From ce558e69d4087dd3653207de78345fbb8a2c7835 Mon Sep 17 00:00:00 2001 From: chgnrdv <52372310+chgnrdv@users.noreply.github.com> Date: Sun, 4 Jun 2023 07:06:45 +0300 Subject: [PATCH] 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 --- Doc/library/atexit.rst | 10 +++++ Lib/test/test_os.py | 16 +++++++ Lib/test/test_subprocess.py | 19 ++++++++ Lib/test/test_threading.py | 44 +++++++------------ ...-05-24-12-10-54.gh-issue-104690.HX3Jou.rst | 6 +++ Modules/_posixsubprocess.c | 6 +++ Modules/_threadmodule.c | 5 +++ Modules/posixmodule.c | 21 ++++++++- 8 files changed, 97 insertions(+), 30 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-05-24-12-10-54.gh-issue-104690.HX3Jou.rst diff --git a/Doc/library/atexit.rst b/Doc/library/atexit.rst index a2bd85b31c9..3dbef69580d 100644 --- 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 c6810c07ae0..8de4ef7270b 100644 --- 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 92f81eaafb1..51ba423a0f1 100644 --- 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 97165264b34..9e4972ecb64 100644 --- 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 index 00000000000..7934dd23b10 --- /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 36470804c6a..2d88f5e9ba1 100644 --- 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 5d753b4a0eb..b6f878e0752 100644 --- 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 1960c6377c2..6f01b70d528 100644 --- 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; }