gh-104690 Disallow thread creation and fork at interpreter finalization (#104826)

Disallow thread creation and fork at interpreter finalization.

in the following functions, check if interpreter is finalizing and raise `RuntimeError` with appropriate message:
* `_thread.start_new_thread` and thus `threading`
* `posix.fork`
* `posix.fork1`
* `posix.forkpty`
* `_posixsubprocess.fork_exec` when a `preexec_fn=` is supplied.

---------

Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Gregory P. Smith <greg@krypto.org>

diff --git a/Doc/library/atexit.rst b/Doc/library/atexit.rst
index a2bd85b31c9a8dc765bf117ff8fa10228c7d2ed4..3dbef69580d9b300eba240cb60741c0b9cb7f5c0 100644 (file)
--- a/Doc/library/atexit.rst
+++ b/Doc/library/atexit.rst
@@ -48,6 +48,16 @@ a cleanup function is undefined.
    This function returns *func*, which makes it possible to use it as a
    decorator.
 
+   .. warning::
+       Starting new threads or calling :func:`os.fork` from a registered
+       function can lead to race condition between the main Python
+       runtime thread freeing thread states while internal :mod:`threading`
+       routines or the new process try to use that state. This can lead to
+       crashes rather than clean shutdown.
+
+   .. versionchanged:: 3.12
+       Attempts to start a new thread or :func:`os.fork` a new process
+       in a registered function now leads to :exc:`RuntimeError`.
 
 .. function:: unregister(func)
 

diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py
index c6810c07ae09d8573cc70306a45e9fa97700bfe3..8de4ef7270b754b38c909a7f9782caaac67d5372 100644 (file)
--- a/Lib/test/test_os.py
+++ b/Lib/test/test_os.py
@@ -4706,6 +4706,22 @@ class ForkTests(unittest.TestCase):
         self.assertEqual(err.decode("utf-8"), "")
         self.assertEqual(out.decode("utf-8"), "")
 
+    def test_fork_at_exit(self):
+        code = """if 1:
+            import atexit
+            import os
+
+            def exit_handler():
+                pid = os.fork()
+                if pid != 0:
+                    print("shouldn't be printed")
+
+            atexit.register(exit_handler)
+        """
+        _, out, err = assert_python_ok("-c", code)
+        self.assertEqual(b"", out)
+        self.assertIn(b"can't fork at interpreter shutdown", err)
+
 
 # Only test if the C version is provided, otherwise TestPEP519 already tested
 # the pure Python implementation.

diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
index 92f81eaafb1c93f69f8cf16a72267fad91da8208..51ba423a0f1c923b2babb0f5192bdc8a3d20c49c 100644 (file)
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -5,6 +5,7 @@ from test.support import check_sanitizer
 from test.support import import_helper
 from test.support import os_helper
 from test.support import warnings_helper
+from test.support.script_helper import assert_python_ok
 import subprocess
 import sys
 import signal
@@ -3329,6 +3330,24 @@ class POSIXProcessTestCase(BaseTestCase):
             except subprocess.TimeoutExpired:
                 pass
 
+    def test_preexec_at_exit(self):
+        code = f"""if 1:
+        import atexit
+        import subprocess
+
+        def dummy():
+            pass
+
+        def exit_handler():
+            subprocess.Popen({ZERO_RETURN_CMD}, preexec_fn=dummy)
+            print("shouldn't be printed")
+
+        atexit.register(exit_handler)
+        """
+        _, out, err = assert_python_ok("-c", code)
+        self.assertEqual(out, b'')
+        self.assertIn(b"preexec_fn not supported at interpreter shutdown", err)
+
 
 @unittest.skipUnless(mswindows, "Windows specific tests")
 class Win32ProcessTestCase(BaseTestCase):

diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py
index 97165264b34bbe5b7f1f3d815783b7396b195d43..9e4972ecb640df8e93504c942c23e04775836a2a 100644 (file)
--- a/Lib/test/test_threading.py
+++ b/Lib/test/test_threading.py
@@ -531,34 +531,6 @@ class ThreadTests(BaseTestCase):
         t = threading.Thread(daemon=True)
         self.assertTrue(t.daemon)
 
-    @support.requires_fork()
-    def test_fork_at_exit(self):
-        # bpo-42350: Calling os.fork() after threading._shutdown() must
-        # not log an error.
-        code = textwrap.dedent("""
-            import atexit
-            import os
-            import sys
-            from test.support import wait_process
-
-            # Import the threading module to register its "at fork" callback
-            import threading
-
-            def exit_handler():
-                pid = os.fork()
-                if not pid:
-                    print("child process ok", file=sys.stderr, flush=True)
-                    # child process
-                else:
-                    wait_process(pid, exitcode=0)
-
-            # exit_handler() will be called after threading._shutdown()
-            atexit.register(exit_handler)
-        """)
-        _, out, err = assert_python_ok("-c", code)
-        self.assertEqual(out, b'')
-        self.assertEqual(err.rstrip(), b'child process ok')
-
     @support.requires_fork()
     def test_dummy_thread_after_fork(self):
         # Issue #14308: a dummy thread in the active list doesn't mess up
@@ -1048,6 +1020,22 @@ class ThreadTests(BaseTestCase):
         self.assertEqual(out, b'')
         self.assertEqual(err, b'')
 
+    def test_start_new_thread_at_exit(self):
+        code = """if 1:
+            import atexit
+            import _thread
+
+            def f():
+                print("shouldn't be printed")
+
+            def exit_handler():
+                _thread.start_new_thread(f, ())
+
+            atexit.register(exit_handler)
+        """
+        _, out, err = assert_python_ok("-c", code)
+        self.assertEqual(out, b'')
+        self.assertIn(b"can't create new thread at interpreter shutdown", err)
 
 class ThreadJoinOnShutdown(BaseTestCase):
 

diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-05-24-12-10-54.gh-issue-104690.HX3Jou.rst b/Misc/NEWS.d/next/Core and Builtins/2023-05-24-12-10-54.gh-issue-104690.HX3Jou.rst
new file mode 100644 (file)
index 0000000..7934dd2
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2023-05-24-12-10-54.gh-issue-104690.HX3Jou.rst
@@ -0,0 +1,6 @@
+Starting new threads and process creation through :func:`os.fork` during interpreter
+shutdown (such as from :mod:`atexit` handlers) is no longer supported.  It can lead
+to race condition between the main Python runtime thread freeing thread states while
+internal :mod:`threading` routines are trying to allocate and use the state of just
+created threads. Or forked children trying to use the mid-shutdown runtime and thread
+state in the child process.

diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
index 36470804c6a1653a656425b88af287d0ff8534c6..2d88f5e9ba1601bf28e0a58d5ba8dda8c68b32aa 100644 (file)
--- a/Modules/_posixsubprocess.c
+++ b/Modules/_posixsubprocess.c
@@ -5,6 +5,7 @@
 
 #include "Python.h"
 #include "pycore_fileutils.h"
+#include "pycore_pystate.h"
 #if defined(HAVE_PIPE2) && !defined(_GNU_SOURCE)
 # define _GNU_SOURCE
 #endif
@@ -943,6 +944,11 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args,
     Py_ssize_t fds_to_keep_len = PyTuple_GET_SIZE(py_fds_to_keep);
 
     PyInterpreterState *interp = PyInterpreterState_Get();
+    if ((preexec_fn != Py_None) && interp->finalizing) {
+        PyErr_SetString(PyExc_RuntimeError,
+                        "preexec_fn not supported at interpreter shutdown");
+        return NULL;
+    }
     if ((preexec_fn != Py_None) && (interp != PyInterpreterState_Main())) {
         PyErr_SetString(PyExc_RuntimeError,
                         "preexec_fn not supported within subinterpreters");

diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c
index 5d753b4a0ebc5e9df52b4292e3f1153f7b36db21..b6f878e07526dbca3d00bb59b41b7e5507f3faaa 100644 (file)
--- a/Modules/_threadmodule.c
+++ b/Modules/_threadmodule.c
@@ -1155,6 +1155,11 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs)
                         "thread is not supported for isolated subinterpreters");
         return NULL;
     }
+    if (interp->finalizing) {
+        PyErr_SetString(PyExc_RuntimeError,
+                        "can't create new thread at interpreter shutdown");
+        return NULL;
+    }
 
     struct bootstate *boot = PyMem_NEW(struct bootstate, 1);
     if (boot == NULL) {

diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
index 1960c6377c226813a7ca50aca5bb54e6d871cc91..6f01b70d528177a7083f54c96d0b9b57dffb9ede 100644 (file)
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -7620,7 +7620,13 @@ os_fork1_impl(PyObject *module)
 {
     pid_t pid;
 
-    if (!_Py_IsMainInterpreter(_PyInterpreterState_GET())) {
+    PyInterpreterState *interp = _PyInterpreterState_GET();
+    if (interp->finalizing) {
+        PyErr_SetString(PyExc_RuntimeError,
+                        "can't fork at interpreter shutdown");
+        return NULL;
+    }
+    if (!_Py_IsMainInterpreter(interp)) {
         PyErr_SetString(PyExc_RuntimeError, "fork not supported for subinterpreters");
         return NULL;
     }
@@ -7656,6 +7662,11 @@ os_fork_impl(PyObject *module)
 {
     pid_t pid;
     PyInterpreterState *interp = _PyInterpreterState_GET();
+    if (interp->finalizing) {
+        PyErr_SetString(PyExc_RuntimeError,
+                        "can't fork at interpreter shutdown");
+        return NULL;
+    }
     if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_FORK)) {
         PyErr_SetString(PyExc_RuntimeError,
                         "fork not supported for isolated subinterpreters");
@@ -8327,7 +8338,13 @@ os_forkpty_impl(PyObject *module)
     int master_fd = -1;
     pid_t pid;
 
-    if (!_Py_IsMainInterpreter(_PyInterpreterState_GET())) {
+    PyInterpreterState *interp = _PyInterpreterState_GET();
+    if (interp->finalizing) {
+        PyErr_SetString(PyExc_RuntimeError,
+                        "can't fork at interpreter shutdown");
+        return NULL;
+    }
+    if (!_Py_IsMainInterpreter(interp)) {
         PyErr_SetString(PyExc_RuntimeError, "fork not supported for subinterpreters");
         return NULL;
     }