bpo-33330: PyImport_Cleanup check for exc leaks #7068

vstinner · 2018-05-23T09:15:45Z

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.

vstinner · 2018-05-23T09:17:24Z

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

vstinner · 2018-05-23T09:31:56Z

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.

serhiy-storchaka · 2018-05-23T10:45:29Z

Python/import.c

@@ -396,15 +396,18 @@ static const char * const sys_files[] = {
 void
 PyImport_Cleanup(void)
 {
+    assert(!PyErr_Occurred());


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.

serhiy-storchaka · 2018-05-23T10:56:05Z

Modules/gcmodule.c

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


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.

serhiy-storchaka · 2018-05-23T11:03:33Z

Python/import.c

@@ -477,6 +481,7 @@ PyImport_Cleanup(void)
    if (PyDict_CheckExact(modules)) {
        pos = 0;
        while (PyDict_Next(modules, &pos, &key, &value)) {
+            assert(!PyErr_Occurred());


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.

serhiy-storchaka · 2018-05-23T11:05:36Z

Python/import.c

@@ -492,6 +497,7 @@ PyImport_Cleanup(void)
                    PyErr_WriteUnraisable(NULL);
                    continue;
                }
+                assert(!PyErr_Occurred());


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

serhiy-storchaka · 2018-05-23T11:07:54Z

Python/import.c

@@ -502,6 +508,7 @@ PyImport_Cleanup(void)
            Py_DECREF(iterator);
        }
    }
+    assert(!PyErr_Occurred());


This never fails.

serhiy-storchaka · 2018-05-23T11:08:50Z

Python/import.c

@@ -524,6 +531,8 @@ PyImport_Cleanup(void)
        PyErr_Clear();
    }
    Py_XDECREF(dict);
+    assert(!PyErr_Occurred());


This never fails.

serhiy-storchaka · 2018-05-23T11:12:00Z

Python/import.c

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


This never fails.

serhiy-storchaka · 2018-05-23T11:13:43Z

Python/import.c

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


This never fails.

serhiy-storchaka · 2018-05-23T11:21:32Z

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.

vstinner · 2018-05-23T12:03:57Z

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.

ericsnowcurrently · 2018-05-23T15:19:03Z

Python/import.c

    PyObject *weaklist = NULL;
    const char * const *p;

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


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

serhiy-storchaka · 2018-05-26T10:08:31Z

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-33330: PyImport_Cleanup check for exc leaks #7068

bpo-33330: PyImport_Cleanup check for exc leaks #7068

vstinner commented May 23, 2018 •

edited by bedevere-bot

Loading

vstinner commented May 23, 2018

vstinner commented May 23, 2018

serhiy-storchaka May 23, 2018

vstinner May 23, 2018

serhiy-storchaka May 23, 2018

serhiy-storchaka May 23, 2018

vstinner May 23, 2018

serhiy-storchaka May 23, 2018

serhiy-storchaka May 23, 2018

serhiy-storchaka May 23, 2018

serhiy-storchaka May 23, 2018

serhiy-storchaka May 23, 2018

serhiy-storchaka May 23, 2018

serhiy-storchaka commented May 23, 2018

vstinner commented May 23, 2018

ericsnowcurrently May 23, 2018

vstinner May 23, 2018

ericsnowcurrently May 23, 2018

serhiy-storchaka commented May 26, 2018

serhiy-storchaka left a comment

vstinner commented May 28, 2018

serhiy-storchaka commented Jul 16, 2018

vstinner commented Jul 16, 2018

bpo-33330: PyImport_Cleanup check for exc leaks #7068

bpo-33330: PyImport_Cleanup check for exc leaks #7068

Conversation

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

vstinner commented May 23, 2018

vstinner commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka commented May 23, 2018

vstinner commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka commented May 26, 2018

serhiy-storchaka left a comment

Choose a reason for hiding this comment

vstinner commented May 28, 2018

serhiy-storchaka commented Jul 16, 2018

vstinner commented Jul 16, 2018

vstinner commented May 23, 2018 •

edited by bedevere-bot

Loading