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.

Sign up for GitHub

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
vstinner wants to merge 1 commit into from
Closed

bpo-33330: PyImport_Cleanup check for exc leaks #7068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
Original file line numberDiff line numberDiff 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

@serhiy-storchakaserhiy-storchakaMay 23, 2018

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
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -396,15 +396,18 @@ static const char * const sys_files[] ={
void
PyImport_Cleanup(void)
{
assert(!PyErr_Occurred());
Copy link
Member

@serhiy-storchakaserhiy-storchakaMay 23, 2018

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
MemberAuthor

@vstinnervstinnerMay 23, 2018

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

@ericsnowcurrentlyericsnowcurrentlyMay 23, 2018

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
MemberAuthor

@vstinnervstinnerMay 23, 2018

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

@ericsnowcurrentlyericsnowcurrentlyMay 23, 2018

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 DownExpand 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 DownExpand 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

@serhiy-storchakaserhiy-storchakaMay 23, 2018

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
MemberAuthor

@vstinnervstinnerMay 23, 2018

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

@serhiy-storchakaserhiy-storchakaMay 23, 2018

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

@serhiy-storchakaserhiy-storchakaMay 23, 2018

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

@serhiy-storchakaserhiy-storchakaMay 23, 2018

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

@serhiy-storchakaserhiy-storchakaMay 23, 2018

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

@serhiy-storchakaserhiy-storchakaMay 23, 2018

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

@serhiy-storchakaserhiy-storchakaMay 23, 2018

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