bpo-33622: Fix issues with handling errors in the GC. by serhiy-storchaka · Pull Request #7078 · python/cpython (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- Fixed a leak when the GC fails to add an object with
__del__into thegc.garbagelist. PyGC_Collect()can now be called when an exception is set and preserves it.- Fixed an UB with comparing a dead pointer with NULL.
https://bugs.python.org/issue33622
- Fixed a leak when the GC fails to add an object with del into the gc.garbage list.
- PyGC_Collect() can now be called when an exception is set and preserves it.
- Fixed an UB with comparing a dead pointer with NULL.
Looks good to me! Note: I don't know what "UB" means in the "Fixed an UB with ..." comment means.
UB = undefined behavior. A common abbreviation in the C/C++ word.
In that case, what about the behavior was undefined? Memory-releasing functions don't magically change the value of a pointer passed to them; r == NULL after Py_XDECREF(r) if and only if r == NULL before it. If r isn't NULL and wasn't obtained by a legit memory allocation function to begin with, or the memory referenced by r was already freed, that's UB.
So I found the rewrite slightly clearer, but didn't see a need for it.
In C the value of a pointer becomes invalid after releasing a memory. It can "work" on some platforms with specific versions of compilers, but the Standard doesn't promise anything. The next version of the compiler can generate the code that will crash or remove the code of the whole function. This is what called an undefined behavior.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
- Fixed a leak when the GC fails to add an object with del into the gc.garbage list.
- PyGC_Collect() can now be called when an exception is set and preserves it.
- Fixed an undefined behavior with comparing a dead pointer with NULL. (cherry picked from commit 301e3cc)
Co-authored-by: Serhiy Storchaka storchaka@gmail.com
Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.cherry_picker 301e3cc8a5bc68c5347ab6ac6f83428000d31ab2 3.6
Sorry, @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.cherry_picker 301e3cc8a5bc68c5347ab6ac6f83428000d31ab2 2.7
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request
- Fixed a leak when the GC fails to add an object with del into the gc.garbage list.
- PyGC_Collect() can now be called when an exception is set and preserves it.
- Fixed an undefined behavior with comparing a dead pointer with NULL.. (cherry picked from commit 301e3cc)
Co-authored-by: Serhiy Storchaka storchaka@gmail.com
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request
- Fixed a leak when the GC fails to add an object with del into the gc.garbage list.
- PyGC_Collect() can now be called when an exception is set and preserves it. (cherry picked from commit 301e3cc)
Co-authored-by: Serhiy Storchaka storchaka@gmail.com
miss-islington added a commit that referenced this pull request
- Fixed a leak when the GC fails to add an object with del into the gc.garbage list.
- PyGC_Collect() can now be called when an exception is set and preserves it.
- Fixed an undefined behavior with comparing a dead pointer with NULL. (cherry picked from commit 301e3cc)
Co-authored-by: Serhiy Storchaka storchaka@gmail.com
serhiy-storchaka added a commit that referenced this pull request
- Fixed a leak when the GC fails to add an object with del into the gc.garbage list.
- PyGC_Collect() can now be called when an exception is set and preserves it.
- Fixed an undefined behavior with comparing a dead pointer with NULL. (cherry picked from commit 301e3cc)
In C the value of a pointer becomes invalid after releasing a memory. It can "work" on some platforms with specific versions of compilers, but the Standard doesn't promise anything.
Not true. Dereferencing a freed pointer gives UB. The code here never dereferenced r after Py_XDECREF(r), it merely tested whether r == NULL. Here's one copy of the C standard. The cases it gives for UB after free() are the ones I already typed. Use your head here: no function can change the value of a pointer passed to it - to pull that off it would have to be passed the address of the pointer (or otherwise have access to that address). C passes pointers by value, not by reference.
As I said, I found your rewrite slightly clearer in this case anyway, so I don't care. I just object to calling a thing UB when it isn't, lest we see a slew of needless changes all over the place based on hallucinations 😉.
J.2 Undefined behavior
The behavior is undefined in the following circumstances:
...
— The value of a pointer that refers to space deallocated by a call to the free or
realloc function is used (7.20.3).
From the implementation point of view, the pointer can be not an arbitrary integer, but a more complex object. For example any use of the pointer (including comparing with NULL) may require loading into a segment register, and this can crash if the segment is no longer valid.
Maybe comparing the pointer to a deallocated memory with NULL will not cause any problems on any platform supported by Python. I don't know. But is better to write a code that conforms the Standard (unless practicality beats purity).
Excellent! Thank you for the reference - I stand gratefully corrected 😃
serhiy-storchaka added a commit that referenced this pull request
- Fixed a leak when the GC fails to add an object with del into the gc.garbage list.
- PyGC_Collect() can now be called when an exception is set and preserves it. (cherry picked from commit 301e3cc)
Labels
An unexpected behavior, bug, or error