FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-33330: PyImport_Cleanup check for exc leaks by vstinner · Pull Request #7068 · python/cpython · GitHub

/ cpython Public
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension .c  (2) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
2 changes: 2 additions & 0 deletions Modules/gcmodule.c
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
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
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
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This never fails.

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

Back | FazBrowse Home | New Git URL