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

Add "assert(!PyErr_Occurred());" assertions to PyImport_Cleanup()
and _PyGC_CollectNoFail() to make sure that these functions don't
leak exceptions.
Replace also PyImport_GetModuleDict() call with interp->modules to
prevent a fatal error if modules is already NULL.

https://bugs.python.org/issue33330

* Add "assert(!PyErr_Occurred());" assertions to PyImport_Cleanup() and _PyGC_CollectNoFail() to make sure that these functions don't leak exceptions. * Replace also PyImport_GetModuleDict() call with interp->modules to prevent a fatal error if modules is already NULL.

The commit d393c1b replaced interp->modules with PyImport_GetModuleDict(), but this function fails with a fatal error if modules is NULL. I'm not sure that it was a deliberate change. Moreover, this commit has been partially reverted (interp->modules is back!).

PyImport_Cleanup() is part of the public API, but it's documented as "Empty the module table. For internal use only." :-( Why do we have public API for internal use only? Likely a mistake from the past.

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.

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.

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.

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

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.

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.

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

This never fails.

I would left only few asserts after calls that can raise exceptions if the third-party code has bugs. And one at the end of PyImport_Cleanup() just for sure. For other asserts we can guarantee the absence of exceptions.

The last 2 years, I added many "assert(!PyErr_Occurred());" in ceval.c and object.c, and it helped me a lot to detect some very old bugs. The rationale here is the same.

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

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.

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. :)

                      during interpreter shutdown (and then never finish it).
                      See http://bugs.python.org/issue8713#msg195178 for an example.
                      */
+                  assert(!PyErr_Occurred());

                   if (PyDict_CheckExact(modules)) {
                       pos = 0;
                       while (PyDict_Next(modules, &pos, &key, &value)) {
+                          assert(!PyErr_Occurred());

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

                           Py_INCREF(mod);
                           if (Py_VerboseFlag && PyUnicode_Check(name))
                               PySys_FormatStderr("# cleanup[3] wiping %U\n", name);
+                          assert(!PyErr_Occurred());

                                   PyErr_WriteUnraisable(NULL);
                                   continue;
                               }
+                              assert(!PyErr_Occurred());

                       PyErr_Clear();
                   }
                   Py_XDECREF(dict);
+                  assert(!PyErr_Occurred());

                   const char * const *p;
-                  if (modules == NULL)
+                  PyObject *modules = interp->modules;

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

Uh oh!

Uh oh!

bpo-33330: PyImport_Cleanup check for exc leaks - #7068

bpo-33330: PyImport_Cleanup check for exc leaks#7068vstinner wants to merge 1 commit intopython:masterpython/cpython:masterfrom vstinner:pyimport_cleanup_assertCopy head branch name to clipboard

Conversation

vstinner commented May 23, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented May 23, 2018

Uh oh!

vstinner commented May 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented May 23, 2018

Uh oh!

vstinner commented May 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented May 26, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented May 28, 2018

Uh oh!

serhiy-storchaka commented Jul 16, 2018

Uh oh!

vstinner commented Jul 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bpo-33330: PyImport_Cleanup check for exc leaks#7068
vstinner wants to merge 1 commit into
python:masterfrom
vstinner:pyimport_cleanup_assert

vstinner commented May 23, 2018 •
edited by bedevere-bot
Loading