From 7a67cf5c302cb44956d47d5cd75bc6a58b953dde Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sat, 26 Oct 2024 22:43:11 -0400 Subject: [PATCH 01/11] Force thread states to get cleaned up. --- Include/internal/pycore_pystate.h | 3 +++ Python/crossinterp.c | 34 +++++++++++++++++++++++++++++++ Python/pystate.c | 13 ++++++++++++ 3 files changed, 50 insertions(+) diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index fade55945b7dbf..f9dc61a1ababe8 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -244,6 +244,9 @@ extern PyThreadState * _PyThreadState_Swap( extern PyStatus _PyInterpreterState_Enable(_PyRuntimeState *runtime); +extern PyThreadState * +_PyThreadState_SwapAttached(PyThreadState *tstate); + #ifdef HAVE_FORK extern PyStatus _PyInterpreterState_DeleteExceptMain(_PyRuntimeState *runtime); extern void _PySignal_AfterFork(void); diff --git a/Python/crossinterp.c b/Python/crossinterp.c index 0aca322d987dba..4456e6e5cb31f2 100644 --- a/Python/crossinterp.c +++ b/Python/crossinterp.c @@ -1692,6 +1692,39 @@ _PyXI_HasCapturedException(_PyXI_session *session) return session->error != NULL; } +static int +_PyXI_ThreadStateRecovery(void *ptr) +{ + if ((&_PyRuntime)->_main_interpreter.finalizing != 1) + { + // If the interpreter isn't finalizing, then + // it's not our job to clean anything up. + return 0; + } + + PyThreadState *interp_tstate = (PyThreadState *)ptr; + if (interp_tstate->interp == NULL) + { + // Interpreter was cleaned up, do nothing. + return 0; + } + + if (!_PyInterpreterState_IsRunningMain(interp_tstate->interp)) + { + // Main thread was cleaned up, nothing to fix. + return 0; + } + + // Subinterpreter is in a thread that suspended early! + PyThreadState *return_tstate = _PyThreadState_SwapAttached(interp_tstate); + _PyInterpreterState_SetNotRunningMain(interp_tstate->interp); + + PyThreadState_Clear(interp_tstate); + PyThreadState_Swap(return_tstate); + PyThreadState_Delete(interp_tstate); + return 0; +} + int _PyXI_Enter(_PyXI_session *session, PyInterpreterState *interp, PyObject *nsupdates) @@ -1718,6 +1751,7 @@ _PyXI_Enter(_PyXI_session *session, errcode = _PyXI_ERR_ALREADY_RUNNING; goto error; } + Py_AddPendingCall(_PyXI_ThreadStateRecovery, _PyThreadState_GET()); session->running = 1; // Cache __main__.__dict__. diff --git a/Python/pystate.c b/Python/pystate.c index 36b31f3b9e4200..db9b607bbfaec8 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2406,6 +2406,19 @@ PyThreadState_Swap(PyThreadState *newts) return _PyThreadState_Swap(&_PyRuntime, newts); } +/* + * Calls PyThreadState_Swap() on the a bound thread state. + * This breaks the GIL, so it should only be used if certain that + * it's impossible for the thread to be running code. + */ +PyThreadState * +_PyThreadState_SwapAttached(PyThreadState *tstate) +{ + tstate->_status.bound_gilstate = 0; + tstate->_status.holds_gil = 0; + tstate->_status.active = 0; + return PyThreadState_Swap(tstate); +} void _PyThreadState_Bind(PyThreadState *tstate) From 30e654d1291fa9fa74f548f70c5d65d7e2c32dce Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sat, 26 Oct 2024 22:45:12 -0400 Subject: [PATCH 02/11] Add some comments. --- Python/crossinterp.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Python/crossinterp.c b/Python/crossinterp.c index 4456e6e5cb31f2..f815df695dac0a 100644 --- a/Python/crossinterp.c +++ b/Python/crossinterp.c @@ -1695,6 +1695,12 @@ _PyXI_HasCapturedException(_PyXI_session *session) static int _PyXI_ThreadStateRecovery(void *ptr) { + /* + * GH-126016: If the subinterpreter was running in a + * thread, and joining that thread was interrupted (namely with CTRL+C), then + * the thread state isn't cleaned up. This forces it to get cleaned up + * upon finalization. + */ if ((&_PyRuntime)->_main_interpreter.finalizing != 1) { // If the interpreter isn't finalizing, then @@ -1716,6 +1722,8 @@ _PyXI_ThreadStateRecovery(void *ptr) } // Subinterpreter is in a thread that suspended early! + // Get rid of this thread state or else finalize_subinterpreters() won't be + // happy. PyThreadState *return_tstate = _PyThreadState_SwapAttached(interp_tstate); _PyInterpreterState_SetNotRunningMain(interp_tstate->interp); From d4b09086759241a0b79f5c428912310f4b385e2c Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sat, 26 Oct 2024 22:46:56 -0400 Subject: [PATCH 03/11] Add blurb --- .../next/Library/2024-10-26-22-46-50.gh-issue-126016.gUWIIK.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-10-26-22-46-50.gh-issue-126016.gUWIIK.rst diff --git a/Misc/NEWS.d/next/Library/2024-10-26-22-46-50.gh-issue-126016.gUWIIK.rst b/Misc/NEWS.d/next/Library/2024-10-26-22-46-50.gh-issue-126016.gUWIIK.rst new file mode 100644 index 00000000000000..e387787458309e --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-26-22-46-50.gh-issue-126016.gUWIIK.rst @@ -0,0 +1,2 @@ +Fixed crash upon using CTRL+C while a thread containing a subinterpreter was +joining. From f7b7eff8bbda932e5f75cf07050252b91e3e05ad Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sat, 26 Oct 2024 22:51:25 -0400 Subject: [PATCH 04/11] Make the blurb entry prettier --- .../Library/2024-10-26-22-46-50.gh-issue-126016.gUWIIK.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2024-10-26-22-46-50.gh-issue-126016.gUWIIK.rst b/Misc/NEWS.d/next/Library/2024-10-26-22-46-50.gh-issue-126016.gUWIIK.rst index e387787458309e..43a0bdc30d52ac 100644 --- a/Misc/NEWS.d/next/Library/2024-10-26-22-46-50.gh-issue-126016.gUWIIK.rst +++ b/Misc/NEWS.d/next/Library/2024-10-26-22-46-50.gh-issue-126016.gUWIIK.rst @@ -1,2 +1 @@ -Fixed crash upon using CTRL+C while a thread containing a subinterpreter was -joining. +Fixed crash upon using CTRL+C while joining a thread containing a subinterpreter. From 4dff63ca32d03ee5901796f9d98f09036c6621f3 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 27 Oct 2024 09:06:36 -0400 Subject: [PATCH 05/11] Add TODO. --- Python/crossinterp.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/Python/crossinterp.c b/Python/crossinterp.c index f815df695dac0a..dc132108c98c60 100644 --- a/Python/crossinterp.c +++ b/Python/crossinterp.c @@ -1721,12 +1721,22 @@ _PyXI_ThreadStateRecovery(void *ptr) return 0; } - // Subinterpreter is in a thread that suspended early! - // Get rid of this thread state or else finalize_subinterpreters() won't be - // happy. + /* Subinterpreter is in a thread that suspended early! + * Get rid of this thread state or else finalize_subinterpreters() won't be + * happy. + */ + + // We have to use SwapAttached because the thread state is technically active. + // Though, we're certain that nothing is using it, because the thread got + // interrupted. PyThreadState *return_tstate = _PyThreadState_SwapAttached(interp_tstate); _PyInterpreterState_SetNotRunningMain(interp_tstate->interp); + // TODO: While this cleans up the thread state and lets the interpreter + // finalize gracefully, it doesn't deal with any reference on the + // _PyXI_session, because that's a stack reference which might be dead + // by now. So, those references get leaked right now--that needs to get + // fixed. PyThreadState_Clear(interp_tstate); PyThreadState_Swap(return_tstate); PyThreadState_Delete(interp_tstate); From 48150be96cf2ca1d81f5697b8b974fca8a35499a Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 27 Oct 2024 15:37:38 -0400 Subject: [PATCH 06/11] Simplify fix. --- Python/crossinterp.c | 52 -------------------------------------------- Python/pylifecycle.c | 23 +++++++++++++++++++- 2 files changed, 22 insertions(+), 53 deletions(-) diff --git a/Python/crossinterp.c b/Python/crossinterp.c index dc132108c98c60..0aca322d987dba 100644 --- a/Python/crossinterp.c +++ b/Python/crossinterp.c @@ -1692,57 +1692,6 @@ _PyXI_HasCapturedException(_PyXI_session *session) return session->error != NULL; } -static int -_PyXI_ThreadStateRecovery(void *ptr) -{ - /* - * GH-126016: If the subinterpreter was running in a - * thread, and joining that thread was interrupted (namely with CTRL+C), then - * the thread state isn't cleaned up. This forces it to get cleaned up - * upon finalization. - */ - if ((&_PyRuntime)->_main_interpreter.finalizing != 1) - { - // If the interpreter isn't finalizing, then - // it's not our job to clean anything up. - return 0; - } - - PyThreadState *interp_tstate = (PyThreadState *)ptr; - if (interp_tstate->interp == NULL) - { - // Interpreter was cleaned up, do nothing. - return 0; - } - - if (!_PyInterpreterState_IsRunningMain(interp_tstate->interp)) - { - // Main thread was cleaned up, nothing to fix. - return 0; - } - - /* Subinterpreter is in a thread that suspended early! - * Get rid of this thread state or else finalize_subinterpreters() won't be - * happy. - */ - - // We have to use SwapAttached because the thread state is technically active. - // Though, we're certain that nothing is using it, because the thread got - // interrupted. - PyThreadState *return_tstate = _PyThreadState_SwapAttached(interp_tstate); - _PyInterpreterState_SetNotRunningMain(interp_tstate->interp); - - // TODO: While this cleans up the thread state and lets the interpreter - // finalize gracefully, it doesn't deal with any reference on the - // _PyXI_session, because that's a stack reference which might be dead - // by now. So, those references get leaked right now--that needs to get - // fixed. - PyThreadState_Clear(interp_tstate); - PyThreadState_Swap(return_tstate); - PyThreadState_Delete(interp_tstate); - return 0; -} - int _PyXI_Enter(_PyXI_session *session, PyInterpreterState *interp, PyObject *nsupdates) @@ -1769,7 +1718,6 @@ _PyXI_Enter(_PyXI_session *session, errcode = _PyXI_ERR_ALREADY_RUNNING; goto error; } - Py_AddPendingCall(_PyXI_ThreadStateRecovery, _PyThreadState_GET()); session->running = 1; // Cache __main__.__dict__. diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 8f38fbedae9842..bbf9fa56f881bf 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2477,7 +2477,28 @@ finalize_subinterpreters(void) /* Clean up all remaining subinterpreters. */ while (interp != NULL) { - assert(!_PyInterpreterState_IsRunningMain(interp)); + if (_PyInterpreterState_IsRunningMain(interp)) + { + /* + * GH-126016: If a subinterpreter was running in another + * thread, and the user interrupted the joining of that thread + * with CTRL+C, then a thread state for this interpreter is still + * active. We need to destroy it before proceeding. + */ + // XXX Are there more threads we have to worry about? + PyThreadState *to_destroy = interp->threads.main; + + // We have to use SwapAttached to attach to an attached thread state. + // If the thread were really running, this would be a thread safety headache. + // Luckily, we know it isn't. + _PyThreadState_SwapAttached(to_destroy); + _PyInterpreterState_SetNotRunningMain(interp); + PyThreadState_Clear(to_destroy); + + // Back to a NULL tstate + PyThreadState_Swap(NULL); + PyThreadState_Delete(to_destroy); + } /* Find the tstate to use for fini. We assume the interpreter will have at most one tstate at this point. */ From 4012aa61ac3e2e8c92fc2539a3954d22fe269466 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 27 Oct 2024 15:57:58 -0400 Subject: [PATCH 07/11] Add a test. --- Lib/test/test_interpreters/test_lifecycle.py | 22 ++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/Lib/test/test_interpreters/test_lifecycle.py b/Lib/test/test_interpreters/test_lifecycle.py index ac24f6568acd95..374318017eeb42 100644 --- a/Lib/test/test_interpreters/test_lifecycle.py +++ b/Lib/test/test_interpreters/test_lifecycle.py @@ -184,6 +184,28 @@ def test_gh_109793(self): print("------") self.assertEqual(proc.returncode, 1) + @support.requires_subprocess() + def test_interrupt_thread_with_interpreter(self): + # See GH-126016: Subinterpreters that are created + # in another thread might not have their thread states + # cleaned up if the user interrupted joining with CTRL+C. + import subprocess + import signal + + with subprocess.Popen([ + sys.executable, "-c", + "import threading, _interpreters\n" + "def run_interp(): _interpreters.run_string(_interpreters.create(), 'import time; print(1, flush=True); time.sleep(5)')\n" + "threading.Thread(target=run_interp).start()" + ], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) as proc: + with proc.stdout: + self.assertEqual(proc.stdout.read(1), "1") + + proc.send_signal(signal.SIGINT) + with proc.stderr: + self.assertIn("KeyboardInterrupt", proc.stderr.read()) + proc.wait() + self.assertEqual(proc.returncode, 0) if __name__ == '__main__': # Test needs to be a package, so we can do relative imports. From 94b6d09879e40de621ba3280e6359778a366e837 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 27 Oct 2024 16:01:24 -0400 Subject: [PATCH 08/11] Add some comments. --- Lib/test/test_interpreters/test_lifecycle.py | 6 +++++- Python/pystate.c | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_interpreters/test_lifecycle.py b/Lib/test/test_interpreters/test_lifecycle.py index 374318017eeb42..24ca0084a7363b 100644 --- a/Lib/test/test_interpreters/test_lifecycle.py +++ b/Lib/test/test_interpreters/test_lifecycle.py @@ -195,16 +195,20 @@ def test_interrupt_thread_with_interpreter(self): with subprocess.Popen([ sys.executable, "-c", "import threading, _interpreters\n" - "def run_interp(): _interpreters.run_string(_interpreters.create(), 'import time; print(1, flush=True); time.sleep(5)')\n" + "def run_interp(): _interpreters.run_string(_interpreters.create()," + "'import time; print(1, flush=True); time.sleep(5)')\n" "threading.Thread(target=run_interp).start()" ], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) as proc: with proc.stdout: + # Make sure that the thread has actually started self.assertEqual(proc.stdout.read(1), "1") + # Send a KeyboardInterrupt to the process proc.send_signal(signal.SIGINT) with proc.stderr: self.assertIn("KeyboardInterrupt", proc.stderr.read()) proc.wait() + # Make sure it didn't segfault self.assertEqual(proc.returncode, 0) if __name__ == '__main__': diff --git a/Python/pystate.c b/Python/pystate.c index db9b607bbfaec8..09708e64eadb97 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2414,6 +2414,9 @@ PyThreadState_Swap(PyThreadState *newts) PyThreadState * _PyThreadState_SwapAttached(PyThreadState *tstate) { + // Evil thread state magic: we manually + // mark it as unbound so we can re-attach it + // to current thread. tstate->_status.bound_gilstate = 0; tstate->_status.holds_gil = 0; tstate->_status.active = 0; From bd9f75470a2899e56d4a90d4fa98efd8aad00dbf Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 27 Oct 2024 16:26:38 -0400 Subject: [PATCH 09/11] Skip test on Windows because it doesn't support SIGINT. --- Lib/test/test_interpreters/test_lifecycle.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_interpreters/test_lifecycle.py b/Lib/test/test_interpreters/test_lifecycle.py index 24ca0084a7363b..7f511cdebce4a4 100644 --- a/Lib/test/test_interpreters/test_lifecycle.py +++ b/Lib/test/test_interpreters/test_lifecycle.py @@ -185,6 +185,7 @@ def test_gh_109793(self): self.assertEqual(proc.returncode, 1) @support.requires_subprocess() + @unittest.skipIf(sys.platform == "win32", "SIGINT does not work on Windows") def test_interrupt_thread_with_interpreter(self): # See GH-126016: Subinterpreters that are created # in another thread might not have their thread states From 75e93dd46d3c6830c36a4582a375b65bc7cb60b9 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 27 Oct 2024 16:31:54 -0400 Subject: [PATCH 10/11] Fix it for Windows (I hope) --- Lib/test/test_interpreters/test_lifecycle.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_interpreters/test_lifecycle.py b/Lib/test/test_interpreters/test_lifecycle.py index 7f511cdebce4a4..66461d896f90d3 100644 --- a/Lib/test/test_interpreters/test_lifecycle.py +++ b/Lib/test/test_interpreters/test_lifecycle.py @@ -185,7 +185,6 @@ def test_gh_109793(self): self.assertEqual(proc.returncode, 1) @support.requires_subprocess() - @unittest.skipIf(sys.platform == "win32", "SIGINT does not work on Windows") def test_interrupt_thread_with_interpreter(self): # See GH-126016: Subinterpreters that are created # in another thread might not have their thread states @@ -193,19 +192,23 @@ def test_interrupt_thread_with_interpreter(self): import subprocess import signal + flags = subprocess.CREATE_NEW_PROCESS_GROUP if sys.platform == "win32" else 0 with subprocess.Popen([ sys.executable, "-c", "import threading, _interpreters\n" "def run_interp(): _interpreters.run_string(_interpreters.create()," "'import time; print(1, flush=True); time.sleep(5)')\n" "threading.Thread(target=run_interp).start()" - ], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) as proc: + ], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, creationflags=flags) as proc: with proc.stdout: # Make sure that the thread has actually started self.assertEqual(proc.stdout.read(1), "1") # Send a KeyboardInterrupt to the process - proc.send_signal(signal.SIGINT) + if sys.platform == "win32": + proc.send_signal(signal.CTRL_C_EVENT) + else: + proc.send_signal(signal.SIGINT) with proc.stderr: self.assertIn("KeyboardInterrupt", proc.stderr.read()) proc.wait() From 61c9299ffb531c491f546fd29ee3ad8b860c613f Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 27 Oct 2024 17:02:48 -0400 Subject: [PATCH 11/11] Revert "Fix it for Windows (I hope)" This reverts commit 75e93dd46d3c6830c36a4582a375b65bc7cb60b9. --- Lib/test/test_interpreters/test_lifecycle.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_interpreters/test_lifecycle.py b/Lib/test/test_interpreters/test_lifecycle.py index 66461d896f90d3..7f511cdebce4a4 100644 --- a/Lib/test/test_interpreters/test_lifecycle.py +++ b/Lib/test/test_interpreters/test_lifecycle.py @@ -185,6 +185,7 @@ def test_gh_109793(self): self.assertEqual(proc.returncode, 1) @support.requires_subprocess() + @unittest.skipIf(sys.platform == "win32", "SIGINT does not work on Windows") def test_interrupt_thread_with_interpreter(self): # See GH-126016: Subinterpreters that are created # in another thread might not have their thread states @@ -192,23 +193,19 @@ def test_interrupt_thread_with_interpreter(self): import subprocess import signal - flags = subprocess.CREATE_NEW_PROCESS_GROUP if sys.platform == "win32" else 0 with subprocess.Popen([ sys.executable, "-c", "import threading, _interpreters\n" "def run_interp(): _interpreters.run_string(_interpreters.create()," "'import time; print(1, flush=True); time.sleep(5)')\n" "threading.Thread(target=run_interp).start()" - ], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, creationflags=flags) as proc: + ], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) as proc: with proc.stdout: # Make sure that the thread has actually started self.assertEqual(proc.stdout.read(1), "1") # Send a KeyboardInterrupt to the process - if sys.platform == "win32": - proc.send_signal(signal.CTRL_C_EVENT) - else: - proc.send_signal(signal.SIGINT) + proc.send_signal(signal.SIGINT) with proc.stderr: self.assertIn("KeyboardInterrupt", proc.stderr.read()) proc.wait()