bpo-33622: Add checks for exceptions leaks in the garbage collector. #7126
Conversation
Failure in adding to gc.garbage is no longer fatal.
| Py_FatalError("gc couldn't create gc.garbage list"); | ||
| } | ||
| for (; gc != finalizers; gc = gc->gc.gc_next) { | ||
| PyObject *op = FROM_GC(gc); |
There was a problem hiding this comment.
Usually, when I start to add assert(!PyErr_Occurred()), I like to add the assertion at the function entry and exit. Here it would avoid to remove an exception, since you add PyErr_Clear(). Currently, it's non obvious that the function must not be called with an exception set. An assertion would make it obvious ;-)
Modules/gcmodule.c
Outdated
| Py_INCREF(op); | ||
| clear(op); | ||
| (void) clear(op); | ||
| assert(!PyErr_Occurred()); |
There was a problem hiding this comment.
You might put the assertion into clear().
There was a problem hiding this comment.
clear is tp_clear. It is defined in user code.
There was a problem hiding this comment.
Oh I see. In that case, the assertion is correct.
Modules/gcmodule.c
Outdated
| n = 0; /* already collecting, don't do anything */ | ||
| else { | ||
| _PyRuntime.gc.collecting = 1; | ||
| assert(!PyErr_Occurred()); |
There was a problem hiding this comment.
Would it make sense to put the assertion inside collect_with_callback()? At the entry.
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM.
My comments are just minor suggestions. The current change is good, if you want to apply it as it is.
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Thank you Victor! I hoped on your review.
Modules/gcmodule.c
Outdated
| Py_INCREF(op); | ||
| clear(op); | ||
| (void) clear(op); | ||
| assert(!PyErr_Occurred()); |
There was a problem hiding this comment.
clear is tp_clear. It is defined in user code.
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
The PR is now perfect :-D
Exception handling at the C level is hard :-( It's so easy to get it wrong (clear or replace the current exception by mistake). These assertions should help to detect such bugs earlier.
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
Still LGTM even with the latest change ;-)
|
I have replaced the assertion with this check because I have found few cases in the stdlib (very unlikely) in which |
Oh. It's a bug, right? Do you plan to open a new issue or write a fix for these bugs? If the bug is "very unlikely" and the case is now handled properly (error logged into stderr), maybe it's fine. |
|
I have opened three new issues for three possible cases of exceptions in |
4 participants
Failure in adding to gc.garbage is no longer fatal.
https://bugs.python.org/issue33622