Skip to content

bpo-33330: PyImport_Cleanup check for exc leaks #7068

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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Modules/gcmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -1595,13 +1595,15 @@ _PyGC_CollectNoFail(void)
during interpreter shutdown (and then never finish it).
See http://bugs.python.org/issue8713#msg195178 for an example.
*/
assert(!PyErr_Occurred());
Copy link
Member

Choose a reason for hiding this comment

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

Yesterday I wrote a patch that improves errors handling in the garbage collector. bpo-33612 is the result of testing it. And I have added similar asserts in _PyGC_CollectNoFail() (but the patch is not completed yet, maybe I'll move them).

At least move the second assert inside the "else" block.

if (_PyRuntime.gc.collecting)
n = 0;
else {
_PyRuntime.gc.collecting = 1;
n = collect(NUM_GENERATIONS - 1, NULL, NULL, 1);
_PyRuntime.gc.collecting = 0;
}
assert(!PyErr_Occurred());
return n;
}

Expand Down
15 changes: 13 additions & 2 deletions Python/import.c
Original file line number Diff line number Diff line change
Expand Up @@ -396,15 +396,18 @@ static const char * const sys_files[] = {
void
PyImport_Cleanup(void)
{
assert(!PyErr_Occurred());
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it is worth to call PyErr_WriteUnraisable() here. This will expose some information about the exception raised outside of this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

The contract of this function is that the caller must not hold an exception. If the assertion fails, I would prefer to fix the caller rather than working around the bug using PyErr_WriteUnraisable().

The point is not only to catch bugs in the current code, but also ease debug when the callers code will be modified. Python shutdown process is very fragile and error prone.


Py_ssize_t pos;
PyObject *key, *value, *dict;
PyInterpreterState *interp = PyThreadState_GET()->interp;
PyObject *modules = PyImport_GetModuleDict();
PyObject *weaklist = NULL;
const char * const *p;

if (modules == NULL)
PyObject *modules = interp->modules;
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather avoid this, if possible. Under what conditions does the use of PyImport_GetModuleDict() cause a fatal error?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you call PyImport_Cleanup() twice. In Python 3.6, calling PyImport_Cleanup() a second time does nothing: that's the purpose of the test on the next line.

PyImport_Cleanup() is part of the public API, even if it's documented as "please don't call this function" ;-) I would prefer to restore Python 3.6 behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks for clarifying. LGTM then. I still plan on landing #3606 at some point, but at that point I'll have to deal with recent interp->modules usage anyway. :)

if (modules == NULL) {
return; /* Already done */
}

/* Delete some special variables first. These are common
places where user values hide and people complain when their
Expand Down Expand Up @@ -437,6 +440,7 @@ PyImport_Cleanup(void)
PyErr_WriteUnraisable(NULL);
}
}
assert(!PyErr_Occurred());

/* We prepare a list which will receive (name, weakref) tuples of
modules when they are removed from sys.modules. The name is used
Expand Down Expand Up @@ -477,6 +481,7 @@ PyImport_Cleanup(void)
if (PyDict_CheckExact(modules)) {
pos = 0;
while (PyDict_Next(modules, &pos, &key, &value)) {
assert(!PyErr_Occurred());
Copy link
Member

Choose a reason for hiding this comment

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

Neither PyDict_Next() nor any other calls before can raise an exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the theory. In practice, something called before might leak an exception. It's to catch bugs "just in case". The assertion is cheap. Debugging an "exception leak" can be very painful.

Copy link
Member

Choose a reason for hiding this comment

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

There is no code between this line and previous error check that can raise an exception and don't silence it. I have checked this.

CLEAR_MODULE(key, value);
}
}
Expand All @@ -492,6 +497,7 @@ PyImport_Cleanup(void)
PyErr_WriteUnraisable(NULL);
continue;
}
assert(!PyErr_Occurred());
Copy link
Member

Choose a reason for hiding this comment

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

PyObject_GetIter(), PyIter_Next() and PyObject_GetItem() can raise and exception if corresponding slots are implemented incorrectly in extensions. assert looks good here.

CLEAR_MODULE(key, value);
Py_DECREF(value);
Py_DECREF(key);
Expand All @@ -502,6 +508,7 @@ PyImport_Cleanup(void)
Py_DECREF(iterator);
}
}
assert(!PyErr_Occurred());
Copy link
Member

Choose a reason for hiding this comment

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

This never fails.


/* Clear the modules dict. */
if (PyDict_CheckExact(modules)) {
Expand All @@ -524,6 +531,8 @@ PyImport_Cleanup(void)
PyErr_Clear();
}
Py_XDECREF(dict);
assert(!PyErr_Occurred());
Copy link
Member

Choose a reason for hiding this comment

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

This never fails.


/* Clear module dict copies stored in the interpreter state */
_PyState_ClearModules();
/* Collect references */
Expand All @@ -543,6 +552,7 @@ PyImport_Cleanup(void)
references left to it), we need to delete the "builtins"
module last. Likewise, we don't delete sys until the very
end because it is implicitly referenced (e.g. by print). */
assert(!PyErr_Occurred());
Copy link
Member

Choose a reason for hiding this comment

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

This never fails.

if (weaklist != NULL) {
Py_ssize_t i, n;
n = PyList_GET_SIZE(weaklist);
Expand All @@ -559,6 +569,7 @@ PyImport_Cleanup(void)
Py_INCREF(mod);
if (Py_VerboseFlag && PyUnicode_Check(name))
PySys_FormatStderr("# cleanup[3] wiping %U\n", name);
assert(!PyErr_Occurred());
Copy link
Member

Choose a reason for hiding this comment

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

This never fails.

_PyModule_Clear(mod);
Py_DECREF(mod);
}
Expand Down