gh-151524: Avoid using instrumentation callback result after Py_DECREF by lpyu001 · Pull Request #151525 · python/cpython
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change lgtm, but I'd create a news entry, like almost all of the fixes for the umbrella issue #146102 did so far.
cc @pablogsal
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
The change lgtm, but I'd create a news entry, like almost all of the fixes for the umbrella issue #146102 did so far.
cc @pablogsal
I’ve submitted the news entry.thanks
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: was it ever tested? Or was just the RC of res higher than 1, so no crash happened?
| @@ -0,0 +1,2 @@ | |||
| Avoid comparing the result of a ``sys.monitoring`` callback after | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a user-facing news entry. Users care about crashes (which could happen here), not about RC :)
Let's rephrase it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't UB. _PyInstrumentation_DISABLE is an immortal object; Py_DECREF operations on it are a no-op.
>>> import sys >>> sys._is_immortal(sys.monitoring.DISABLE) True
That said, I do agree that it's misleading. Let's either remove the Py_DECREF call entirely and/or add an assertion that it's immortal.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically just a refactor, so it's not user-facing. Let's remove this.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i‘ve changed it
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.