bpo-33622: Add checks for exceptions leaks in the garbage collector. by serhiy-storchaka · Pull Request #7126 · 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

Conversation12 Commits3 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 }})

Failure in adding to gc.garbage is no longer fatal.

@@ -663,8 +663,10 @@ handle_legacy_finalizers(PyGC_Head finalizers, PyGC_Head old)
PyObject *op = FROM_GC(gc);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

}
else {
if ((clear = Py_TYPE(op)->tp_clear) != NULL) {
Py_INCREF(op);
clear(op);
(void) clear(op);
assert(!PyErr_Occurred());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might put the assertion into clear().

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clear is tp_clear. It is defined in user code.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. In that case, the assertion is correct.

@@ -1127,6 +1137,7 @@ gc_collect_impl(PyObject *module, int generation)
n = 0; /* already collecting, don't do anything */
else {
_PyRuntime.gc.collecting = 1;
assert(!PyErr_Occurred());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to put the assertion inside collect_with_callback()? At the entry.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

My comments are just minor suggestions. The current change is good, if you want to apply it as it is.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Victor! I hoped on your review.

}
else {
if ((clear = Py_TYPE(op)->tp_clear) != NULL) {
Py_INCREF(op);
clear(op);
(void) clear(op);
assert(!PyErr_Occurred());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clear is tp_clear. It is defined in user code.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 tp_clear() sets an exception. Currently this can lead to a crash. Cases in the stdlib should be fixed in separate issues, but there may be third-party code.

I have replaced the assertion with this check because I have found few cases in the stdlib (very unlikely) in which tp_clear() sets an exception. Currently this can lead to a crash.

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 tp_clear.