From 4590f72259ecbcea66e12a28af14d867255d2e66 Mon Sep 17 00:00:00 2001 From: Eddie Elizondo Date: Tue, 4 Feb 2020 02:29:25 -0800 Subject: bpo-38076 Clear the interpreter state only after clearing module globals (GH-18039) Currently, during runtime destruction, `_PyImport_Cleanup` is clearing the interpreter state before clearing out the modules themselves. This leads to a segfault on modules that rely on the module state to clear themselves up. For example, let's take the small snippet added in the issue by @DinoV : ``` import _struct class C: def __init__(self): self.pack = _struct.pack def __del__(self): self.pack('I', -42) _struct.x = C() ``` The module `_struct` uses the module state to run `pack`. Therefore, the module state has to be alive until after the module has been cleared out to successfully run `C.__del__`. This happens at line 606, when `_PyImport_Cleanup` calls `_PyModule_Clear`. In fact, the loop that calls `_PyModule_Clear` has in its comments: > Now, if there are any modules left alive, clear their globals to minimize potential leaks. All C extension modules actually end up here, since they are kept alive in the interpreter state. That means that we can't clear the module state (which is used by C Extensions) before we run that loop. Moving `_PyInterpreterState_ClearModules` until after it, fixes the segfault in the code snippet. Finally, this updates a test in `io` to correctly assert the error that it now throws (since it now finds the io module state). The test that uses this is: `test_create_at_shutdown_without_encoding`. Given this test is now working is a proof that the module state now stays alive even when `__del__` is called at module destruction time. Thus, I didn't add a new tests for this. https://bugs.python.org/issue38076 --- Lib/test/test_io.py | 2 +- Lib/test/test_struct.py | 18 ++++++++++++++++++ Lib/test/test_sys.py | 17 +++++++++++++++++ .../C API/2020-01-17-11-37-05.bpo-38076.cxfw2x.rst | 2 ++ Python/import.c | 5 +++-- 5 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2020-01-17-11-37-05.bpo-38076.cxfw2x.rst diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index 8a123fa..a66726e 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -3683,7 +3683,7 @@ def _to_memoryview(buf): class CTextIOWrapperTest(TextIOWrapperTest): io = io - shutdown_error = "RuntimeError: could not find io module state" + shutdown_error = "LookupError: unknown encoding: ascii" def test_initialization(self): r = self.BytesIO(b"\xc3\xa9\n\n") diff --git a/Lib/test/test_struct.py b/Lib/test/test_struct.py index 157efa1..4829fbe 100644 --- a/Lib/test/test_struct.py +++ b/Lib/test/test_struct.py @@ -7,6 +7,7 @@ import struct import sys from test import support +from test.support.script_helper import assert_python_ok ISBIGENDIAN = sys.byteorder == "big" @@ -652,6 +653,23 @@ class StructTest(unittest.TestCase): s2 = struct.Struct(s.format.encode()) self.assertEqual(s2.format, s.format) + def test_struct_cleans_up_at_runtime_shutdown(self): + code = """if 1: + import struct + + class C: + def __init__(self): + self.pack = struct.pack + def __del__(self): + self.pack('I', -42) + + struct.x = C() + """ + rc, stdout, stderr = assert_python_ok("-c", code) + self.assertEqual(rc, 0) + self.assertEqual(stdout.rstrip(), b"") + self.assertIn(b"Exception ignored in:", stderr) + self.assertIn(b"C.__del__", stderr) class UnpackIteratorTest(unittest.TestCase): """ diff --git a/Lib/test/test_sys.py b/Lib/test/test_sys.py index 58701a1..c5bd8a4 100644 --- a/Lib/test/test_sys.py +++ b/Lib/test/test_sys.py @@ -855,6 +855,23 @@ class SysModuleTest(unittest.TestCase): self.assertIn(b'sys.flags', out[0]) self.assertIn(b'sys.float_info', out[1]) + def test_sys_ignores_cleaning_up_user_data(self): + code = """if 1: + import struct, sys + + class C: + def __init__(self): + self.pack = struct.pack + def __del__(self): + self.pack('I', -42) + + sys.x = C() + """ + rc, stdout, stderr = assert_python_ok('-c', code) + self.assertEqual(rc, 0) + self.assertEqual(stdout.rstrip(), b"") + self.assertEqual(stderr.rstrip(), b"") + @unittest.skipUnless(hasattr(sys, 'getandroidapilevel'), 'need sys.getandroidapilevel()') def test_getandroidapilevel(self): diff --git a/Misc/NEWS.d/next/C API/2020-01-17-11-37-05.bpo-38076.cxfw2x.rst b/Misc/NEWS.d/next/C API/2020-01-17-11-37-05.bpo-38076.cxfw2x.rst new file mode 100644 index 0000000..d9f6dc3 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2020-01-17-11-37-05.bpo-38076.cxfw2x.rst @@ -0,0 +1,2 @@ +Fix to clear the interpreter state only after clearing module globals to +guarantee module state access from C Extensions during runtime destruction diff --git a/Python/import.c b/Python/import.c index 9838c3f..8bf0448 100644 --- a/Python/import.c +++ b/Python/import.c @@ -568,8 +568,6 @@ _PyImport_Cleanup(PyThreadState *tstate) _PyErr_Clear(tstate); } Py_XDECREF(dict); - /* Clear module dict copies stored in the interpreter state */ - _PyInterpreterState_ClearModules(interp); /* Collect references */ _PyGC_CollectNoFail(); /* Dump GC stats before it's too late, since it uses the warnings @@ -621,6 +619,9 @@ _PyImport_Cleanup(PyThreadState *tstate) } _PyModule_ClearDict(interp->builtins); + /* Clear module dict copies stored in the interpreter state */ + _PyInterpreterState_ClearModules(interp); + /* Clear and delete the modules directory. Actual modules will still be there only if imported during the execution of some destructor. */ -- cgit v0.12