[Python-checkins] bpo-38076 Clear the interpreter state only after clearing module globals (GH-18039)

[Eddie Elizondo]

https://github.com/python/cpython/commit/4590f72259ecbcea66e12a28af14d867255d2e66
commit: 4590f72259ecbcea66e12a28af14d867255d2e66
branch: master
author: Eddie Elizondo <eelizondo at fb.com>
committer: GitHub <noreply at github.com>
date: 2020-02-04T02:29:25-08:00
summary:

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

files:
A Misc/NEWS.d/next/C API/2020-01-17-11-37-05.bpo-38076.cxfw2x.rst
M Lib/test/test_io.py
M Lib/test/test_struct.py
M Lib/test/test_sys.py
M Python/import.c

diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py
index 8a123fa1dc07a..a66726e9a790a 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 157efa1347a9b..4829fbe1b975c 100644
--- a/Lib/test/test_struct.py
+++ b/Lib/test/test_struct.py
@@ -7,6 +7,7 @@
 import sys
 
 from test import support
+from test.support.script_helper import assert_python_ok
 
 ISBIGENDIAN = sys.byteorder == "big"
 
@@ -652,6 +653,23 @@ def test_format_attr(self):
         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 58701a11f919c..c5bd8a4b1ff96 100644
--- a/Lib/test/test_sys.py
+++ b/Lib/test/test_sys.py
@@ -855,6 +855,23 @@ def __del__(self, sys=sys):
         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 0000000000000..d9f6dc31efd8d
--- /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 9838c3fa04538..8bf044827c6e9 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. */