From 15ecad5639880950c38efdc775bc48cd91b697a3 Mon Sep 17 00:00:00 2001 From: Radislav Chugunov Date: Wed, 24 May 2023 03:14:29 +0300 Subject: [PATCH 1/9] 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` * `posix.fork` * `posix.fork1` * `posix.forkpty` * `_posixsubprocess.fork_exec` --- Modules/_posixsubprocess.c | 6 ++++++ Modules/_threadmodule.c | 5 +++++ Modules/posixmodule.c | 21 +++++++++++++++++++-- 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 1b7fe71186a163..8939b1bb209fb0 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 @@ -926,6 +927,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 (interp->finalizing) { + PyErr_SetString(PyExc_RuntimeError, + "child process creation is not allowed at interpreter exit"); + 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 5d753b4a0ebc5e..ed21e33723d23a 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, + "creation of new threads is not allowed at interpreter exit"); + return NULL; + } struct bootstate *boot = PyMem_NEW(struct bootstate, 1); if (boot == NULL) { diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 531f26ba8bc86f..750e09ab991833 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -7527,7 +7527,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, + "fork is not allowed at interpreter exit"); + return NULL; + } + if (!_Py_IsMainInterpreter(interp)) { PyErr_SetString(PyExc_RuntimeError, "fork not supported for subinterpreters"); return NULL; } @@ -7563,6 +7569,11 @@ os_fork_impl(PyObject *module) { pid_t pid; PyInterpreterState *interp = _PyInterpreterState_GET(); + if (interp->finalizing) { + PyErr_SetString(PyExc_RuntimeError, + "fork is not allowed at interpreter exit"); + return NULL; + } if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_FORK)) { PyErr_SetString(PyExc_RuntimeError, "fork not supported for isolated subinterpreters"); @@ -8234,7 +8245,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, + "fork is not allowed at interpreter exit"); + return NULL; + } + if (!_Py_IsMainInterpreter(interp)) { PyErr_SetString(PyExc_RuntimeError, "fork not supported for subinterpreters"); return NULL; } From 72095f37efaccd81ac19ce9dd83eacba826efd4d Mon Sep 17 00:00:00 2001 From: Radislav Chugunov Date: Wed, 24 May 2023 12:16:34 +0300 Subject: [PATCH 2/9] improved error messages --- Modules/_posixsubprocess.c | 2 +- Modules/_threadmodule.c | 2 +- Modules/posixmodule.c | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 8939b1bb209fb0..260dd57f466f7e 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -929,7 +929,7 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args, PyInterpreterState *interp = PyInterpreterState_Get(); if (interp->finalizing) { PyErr_SetString(PyExc_RuntimeError, - "child process creation is not allowed at interpreter exit"); + "can't create child process at interpreter shutdown"); return NULL; } if ((preexec_fn != Py_None) && (interp != PyInterpreterState_Main())) { diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index ed21e33723d23a..b6f878e07526db 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1157,7 +1157,7 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs) } if (interp->finalizing) { PyErr_SetString(PyExc_RuntimeError, - "creation of new threads is not allowed at interpreter exit"); + "can't create new thread at interpreter shutdown"); return NULL; } diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 750e09ab991833..88149de4c81eee 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -7530,7 +7530,7 @@ os_fork1_impl(PyObject *module) PyInterpreterState *interp = _PyInterpreterState_GET(); if (interp->finalizing) { PyErr_SetString(PyExc_RuntimeError, - "fork is not allowed at interpreter exit"); + "can't fork at interpreter shutdown"); return NULL; } if (!_Py_IsMainInterpreter(interp)) { @@ -7571,7 +7571,7 @@ os_fork_impl(PyObject *module) PyInterpreterState *interp = _PyInterpreterState_GET(); if (interp->finalizing) { PyErr_SetString(PyExc_RuntimeError, - "fork is not allowed at interpreter exit"); + "can't fork at interpreter shutdown"); return NULL; } if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_FORK)) { @@ -8248,7 +8248,7 @@ os_forkpty_impl(PyObject *module) PyInterpreterState *interp = _PyInterpreterState_GET(); if (interp->finalizing) { PyErr_SetString(PyExc_RuntimeError, - "fork is not allowed at interpreter exit"); + "can't fork at interpreter shutdown"); return NULL; } if (!_Py_IsMainInterpreter(interp)) { From 77443a04886a82fa26d19e8bd9739b08894895f0 Mon Sep 17 00:00:00 2001 From: Radislav Chugunov Date: Wed, 24 May 2023 13:29:23 +0300 Subject: [PATCH 3/9] added tests, changed test_threading.ThreadTests.test_fork_at_exit to fit new behaviour --- Lib/test/test_os.py | 16 ++++++++++++++++ Lib/test/test_subprocess.py | 16 ++++++++++++++++ Lib/test/test_threading.py | 18 +++++++++++++++++- 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 584cc05ca82a55..9453742132667d 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -4700,6 +4700,22 @@ def test_fork_warns_when_non_python_thread_exists(self): 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 92f81eaafb1c93..c178c544ac221f 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -5,6 +5,7 @@ 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,21 @@ def test_communicate_repeated_call_after_stdout_close(self): except subprocess.TimeoutExpired: pass + def test_create_child_process_at_exit(self): + code = f"""if 1: + import atexit + import subprocess + + def exit_handler(): + subprocess.Popen({ZERO_RETURN_CMD}) + print("shouldn't be printed") + + atexit.register(exit_handler) + """ + _, out, err = assert_python_ok("-c", code) + self.assertEqual(out, b'') + self.assertIn(b"can't create child process 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 97165264b34bbe..639c4b8955d7cc 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -557,7 +557,7 @@ def exit_handler(): """) _, out, err = assert_python_ok("-c", code) self.assertEqual(out, b'') - self.assertEqual(err.rstrip(), b'child process ok') + self.assertIn(b"can't fork at interpreter shutdown", err) @support.requires_fork() def test_dummy_thread_after_fork(self): @@ -1048,6 +1048,22 @@ def import_threading(): 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): From 3134fbe42b7d12376638c34a2f3bf1e3a03d82e2 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Wed, 24 May 2023 12:10:57 +0000 Subject: [PATCH 4/9] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2023-05-24-12-10-54.gh-issue-104690.HX3Jou.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-05-24-12-10-54.gh-issue-104690.HX3Jou.rst 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 00000000000000..5fb908752ee749 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-05-24-12-10-54.gh-issue-104690.HX3Jou.rst @@ -0,0 +1 @@ +Starting new threads and process creation through fork/vfork at interpreter exit is no longer supported, as it can lead to race condition between main Python thread freeing existing thread states and :func:`_thread.start_new_thread` trying to allocate and use state of just created thread or fork/vfork trying to use main thread state in child process. From bbc8ca4ae24dc5ba296a2923a355b77aeaac7626 Mon Sep 17 00:00:00 2001 From: Radislav Chugunov Date: Fri, 26 May 2023 12:15:17 +0300 Subject: [PATCH 5/9] Suggested changes: * removed excess test case from `test_threading` * changed NEWS entry to not mention internal function * allowed `subprocess.Popen` at shutdown if `preexec_fn` is `None` --- Lib/test/test_threading.py | 28 ------------------- ...-05-24-12-10-54.gh-issue-104690.HX3Jou.rst | 2 +- Modules/_posixsubprocess.c | 4 +-- 3 files changed, 3 insertions(+), 31 deletions(-) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 639c4b8955d7cc..9e4972ecb640df 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -531,34 +531,6 @@ def test_daemon_param(self): 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.assertIn(b"can't fork at interpreter shutdown", err) - @support.requires_fork() def test_dummy_thread_after_fork(self): # Issue #14308: a dummy thread in the active list doesn't mess up 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 index 5fb908752ee749..abde6a2f372b22 100644 --- 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 @@ -1 +1 @@ -Starting new threads and process creation through fork/vfork at interpreter exit is no longer supported, as it can lead to race condition between main Python thread freeing existing thread states and :func:`_thread.start_new_thread` trying to allocate and use state of just created thread or fork/vfork trying to use main thread state in child process. +Starting new threads and process creation through fork/vfork at interpreter exit is no longer supported, as it can lead to race condition between main Python thread freeing existing thread states and internal :mod:`threading` routines trying to allocate and use state of just created thread or fork/vfork trying to use main thread state in child process. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index bc89dd5bda09c0..2d88f5e9ba1601 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -944,9 +944,9 @@ 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 (interp->finalizing) { + if ((preexec_fn != Py_None) && interp->finalizing) { PyErr_SetString(PyExc_RuntimeError, - "can't create child process at interpreter shutdown"); + "preexec_fn not supported at interpreter shutdown"); return NULL; } if ((preexec_fn != Py_None) && (interp != PyInterpreterState_Main())) { From bcbcb75ea595dee5cddde23b828506bcb4fcbdcd Mon Sep 17 00:00:00 2001 From: Radislav Chugunov Date: Fri, 26 May 2023 12:20:59 +0300 Subject: [PATCH 6/9] fixed test case in test_subprocess --- Lib/test/test_subprocess.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index c178c544ac221f..51ba423a0f1c92 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -3330,20 +3330,23 @@ def test_communicate_repeated_call_after_stdout_close(self): except subprocess.TimeoutExpired: pass - def test_create_child_process_at_exit(self): + 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}) + 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"can't create child process at interpreter shutdown", err) + self.assertIn(b"preexec_fn not supported at interpreter shutdown", err) @unittest.skipUnless(mswindows, "Windows specific tests") From 0926f6f1b14e8529fc74833a82cc24253d3e7772 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Fri, 26 May 2023 10:24:20 -0700 Subject: [PATCH 7/9] Reword and clarify the NEWS entry. --- .../2023-05-24-12-10-54.gh-issue-104690.HX3Jou.rst | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 index abde6a2f372b22..7934dd23b10691 100644 --- 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 @@ -1 +1,6 @@ -Starting new threads and process creation through fork/vfork at interpreter exit is no longer supported, as it can lead to race condition between main Python thread freeing existing thread states and internal :mod:`threading` routines trying to allocate and use state of just created thread or fork/vfork trying to use main thread state in child process. +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. From 9d0a750b4df25deb1ebb7ac0f115ecc7db81c1d1 Mon Sep 17 00:00:00 2001 From: Radislav Chugunov Date: Fri, 26 May 2023 21:31:28 +0300 Subject: [PATCH 8/9] mentioned new behaviour and added warning about old one in atexit docs --- Doc/library/atexit.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Doc/library/atexit.rst b/Doc/library/atexit.rst index a2bd85b31c9a8d..1b1f0f772aa996 100644 --- a/Doc/library/atexit.rst +++ b/Doc/library/atexit.rst @@ -48,6 +48,17 @@ a cleanup function is undefined. This function returns *func*, which makes it possible to use it as a decorator. + .. warning:: + Starting new threads and process creation through :func:`os.fork` + in registered function 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. + + .. versionchanged:: 3.12 + Attempt to start a new thread or create a new process through :func:`os.fork` + in registered function now leads to :exc:`RuntimeError`. .. function:: unregister(func) From deb9204d1bbaf07e2376c3102cc7dc3a769f46eb Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sat, 3 Jun 2023 08:41:35 -0700 Subject: [PATCH 9/9] Cleanup atexit.rst wording. --- Doc/library/atexit.rst | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/Doc/library/atexit.rst b/Doc/library/atexit.rst index 1b1f0f772aa996..3dbef69580d9b3 100644 --- a/Doc/library/atexit.rst +++ b/Doc/library/atexit.rst @@ -49,16 +49,15 @@ a cleanup function is undefined. decorator. .. warning:: - Starting new threads and process creation through :func:`os.fork` - in registered function 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. + 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 - Attempt to start a new thread or create a new process through :func:`os.fork` - in registered function now leads to :exc:`RuntimeError`. + 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)