From 4899ba796245ecca4a8bf170e637fdca4a2f30f5 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sat, 16 Nov 2024 17:19:13 -0500 Subject: [PATCH 1/8] Lock while freeing the thread state. --- Python/pystate.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index 24ee73c145cbcc..1ac7ce170d5efe 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1746,7 +1746,6 @@ tstate_delete_common(PyThreadState *tstate, int release_gil) } _PyRuntimeState *runtime = interp->runtime; - HEAD_LOCK(runtime); if (tstate->prev) { tstate->prev->next = tstate->next; } @@ -1775,8 +1774,6 @@ tstate_delete_common(PyThreadState *tstate, int release_gil) assert(tstate_impl->refcounts.values == NULL); #endif - HEAD_UNLOCK(runtime); - // XXX Unbind in PyThreadState_Clear(), or earlier // (and assert not-equal here)? if (tstate->_status.bound_gilstate) { @@ -1804,11 +1801,14 @@ zapthreads(PyInterpreterState *interp) PyThreadState *tstate; /* No need to lock the mutex here because this should only happen when the threads are all really dead (XXX famous last words). */ + _PyRuntimeState *runtime = interp->runtime; + HEAD_LOCK(runtime); while ((tstate = interp->threads.head) != NULL) { tstate_verify_not_active(tstate); tstate_delete_common(tstate, 0); free_threadstate((_PyThreadStateImpl *)tstate); } + HEAD_UNLOCK(runtime); } @@ -1816,9 +1816,12 @@ void PyThreadState_Delete(PyThreadState *tstate) { _Py_EnsureTstateNotNULL(tstate); + _PyRuntimeState *runtime = tstate->interp->runtime; + HEAD_LOCK(runtime); tstate_verify_not_active(tstate); tstate_delete_common(tstate, 0); free_threadstate((_PyThreadStateImpl *)tstate); + HEAD_UNLOCK(runtime); } @@ -1830,8 +1833,11 @@ _PyThreadState_DeleteCurrent(PyThreadState *tstate) _Py_qsbr_detach(((_PyThreadStateImpl *)tstate)->qsbr); #endif current_fast_clear(tstate->interp->runtime); + _PyRuntimeState *runtime = tstate->interp->runtime; + HEAD_LOCK(runtime); tstate_delete_common(tstate, 1); // release GIL as part of call free_threadstate((_PyThreadStateImpl *)tstate); + HEAD_UNLOCK(runtime); } void From 9e080365439ba00f1dd0c565558873080cd88dd3 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sat, 16 Nov 2024 17:41:05 -0500 Subject: [PATCH 2/8] Add a test. --- Lib/test/test_interpreters/test_stress.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Lib/test/test_interpreters/test_stress.py b/Lib/test/test_interpreters/test_stress.py index e400535b2a0e4e..756ac165e461fb 100644 --- a/Lib/test/test_interpreters/test_stress.py +++ b/Lib/test/test_interpreters/test_stress.py @@ -32,6 +32,22 @@ def task(): with threading_helper.start_threads(threads): pass + @support.requires_resource('cpu') + def test_many_threads_to_same_interp(self): + interp = interpreters.create() + + def run(): + this_interp = interpreters.create() + this_interp.exec(f"import _interpreters; _interpreters.run_string({interp.id}, '1')") + + + threads = (threading.Thread(target=run) for _ in range(100)) + with threading_helper.catch_threading_exception() as cm: + with threading_helper.start_threads(threads): + pass + + self.assertIsNone(cm.exc_value) + if __name__ == '__main__': # Test needs to be a package, so we can do relative imports. From a095410a5a389cab8349d5e697876e68dc67a418 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sat, 16 Nov 2024 17:42:42 -0500 Subject: [PATCH 3/8] Add blurb --- .../2024-11-16-17-42-36.gh-issue-126914.smTnX0.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-11-16-17-42-36.gh-issue-126914.smTnX0.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-11-16-17-42-36.gh-issue-126914.smTnX0.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-16-17-42-36.gh-issue-126914.smTnX0.rst new file mode 100644 index 00000000000000..1526104f3f1638 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-16-17-42-36.gh-issue-126914.smTnX0.rst @@ -0,0 +1,2 @@ +Fix crash when trying to concurrently create a thread state for a +subinterpreter. From 028889916c73eee4ac5f493519ea991e15d87cfe Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sat, 16 Nov 2024 17:43:53 -0500 Subject: [PATCH 4/8] Add a comment. --- Lib/test/test_interpreters/test_stress.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/test/test_interpreters/test_stress.py b/Lib/test/test_interpreters/test_stress.py index 756ac165e461fb..6f416d071cf850 100644 --- a/Lib/test/test_interpreters/test_stress.py +++ b/Lib/test/test_interpreters/test_stress.py @@ -34,6 +34,8 @@ def task(): @support.requires_resource('cpu') def test_many_threads_to_same_interp(self): + # GH-126914: The initial thread of a subinterpreter could be accessed + # concurrently in new_threadstate() while it was finalizing. interp = interpreters.create() def run(): From 685ebf6f72fa0b847b928ef68b9b5c8f36ba81da Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 17 Nov 2024 14:19:50 -0500 Subject: [PATCH 5/8] Revert "Lock while freeing the thread state." This reverts commit 4899ba796245ecca4a8bf170e637fdca4a2f30f5. --- Python/pystate.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index 1ac7ce170d5efe..24ee73c145cbcc 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1746,6 +1746,7 @@ tstate_delete_common(PyThreadState *tstate, int release_gil) } _PyRuntimeState *runtime = interp->runtime; + HEAD_LOCK(runtime); if (tstate->prev) { tstate->prev->next = tstate->next; } @@ -1774,6 +1775,8 @@ tstate_delete_common(PyThreadState *tstate, int release_gil) assert(tstate_impl->refcounts.values == NULL); #endif + HEAD_UNLOCK(runtime); + // XXX Unbind in PyThreadState_Clear(), or earlier // (and assert not-equal here)? if (tstate->_status.bound_gilstate) { @@ -1801,14 +1804,11 @@ zapthreads(PyInterpreterState *interp) PyThreadState *tstate; /* No need to lock the mutex here because this should only happen when the threads are all really dead (XXX famous last words). */ - _PyRuntimeState *runtime = interp->runtime; - HEAD_LOCK(runtime); while ((tstate = interp->threads.head) != NULL) { tstate_verify_not_active(tstate); tstate_delete_common(tstate, 0); free_threadstate((_PyThreadStateImpl *)tstate); } - HEAD_UNLOCK(runtime); } @@ -1816,12 +1816,9 @@ void PyThreadState_Delete(PyThreadState *tstate) { _Py_EnsureTstateNotNULL(tstate); - _PyRuntimeState *runtime = tstate->interp->runtime; - HEAD_LOCK(runtime); tstate_verify_not_active(tstate); tstate_delete_common(tstate, 0); free_threadstate((_PyThreadStateImpl *)tstate); - HEAD_UNLOCK(runtime); } @@ -1833,11 +1830,8 @@ _PyThreadState_DeleteCurrent(PyThreadState *tstate) _Py_qsbr_detach(((_PyThreadStateImpl *)tstate)->qsbr); #endif current_fast_clear(tstate->interp->runtime); - _PyRuntimeState *runtime = tstate->interp->runtime; - HEAD_LOCK(runtime); tstate_delete_common(tstate, 1); // release GIL as part of call free_threadstate((_PyThreadStateImpl *)tstate); - HEAD_UNLOCK(runtime); } void From f795f6e9c5bbdd2b25ffab9205a6ad381814c909 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 17 Nov 2024 14:32:23 -0500 Subject: [PATCH 6/8] Use an atomic field. --- Include/internal/pycore_interp.h | 5 +++++ Python/pystate.c | 28 ++++++++++++++++++++++++---- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 824b865eda60df..598cb3c71aa5a0 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -139,6 +139,11 @@ struct _is { or the size specified by the THREAD_STACK_SIZE macro. */ /* Used in Python/thread.c. */ size_t stacksize; + /* + * Whether the _initial_thread has been used. + * This should only be accessed atomically. + */ + int used_initial; } threads; /* Reference to the _PyRuntime global variable. This field exists diff --git a/Python/pystate.c b/Python/pystate.c index 24ee73c145cbcc..04b8ff1c5c5f40 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1406,6 +1406,9 @@ free_threadstate(_PyThreadStateImpl *tstate) memcpy(tstate, &initial._main_interpreter._initial_thread, sizeof(*tstate)); + _Py_atomic_store_int_relaxed( + &tstate->base.interp->threads.used_initial, + 0); } else { PyMem_RawFree(tstate); @@ -1526,18 +1529,35 @@ new_threadstate(PyInterpreterState *interp, int whence) interp->threads.next_unique_id += 1; uint64_t id = interp->threads.next_unique_id; + /* + * GH-126914: Using the initial thread is a bit of a headache + * in terms of thread safety. Originally, this function just checked + * if interp->threads.head was NULL, but since that gets set + * before the thread is fully cleaned up, it's possible + * for it to still be in use while interp->threads.head is NULL. + * + * So, an atomic compare-exchange is used here to properly only let one + * thread access the initial thread at a time. + */ + int expected = 0; // Initial is available + int set_initial = _Py_atomic_compare_exchange_int( + &interp->threads.used_initial, + &expected, 1); + // Allocate the thread state and add it to the interpreter. PyThreadState *old_head = interp->threads.head; - if (old_head == NULL) { - // It's the interpreter's initial thread state. + if (set_initial == 1) { + // The initial thread state is not in use, and we successfully + // claimed it! + assert(old_head == NULL); + assert(_Py_atomic_load_int_relaxed(&interp->threads.used_initial) == 1); used_newtstate = 0; tstate = &interp->_initial_thread; } - // XXX Re-use interp->_initial_thread if not in use? else { // Every valid interpreter must have at least one thread. assert(id > 1); - assert(old_head->prev == NULL); + assert(old_head == NULL || old_head->prev == NULL); used_newtstate = 1; tstate = new_tstate; // Set to _PyThreadState_INIT. From 5153dc1a4eaebd9e256d4d82d4702fc628dfb613 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 17 Nov 2024 15:05:42 -0500 Subject: [PATCH 7/8] Copy the interpreter state pointer before overwriting it. --- Python/pystate.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index 04b8ff1c5c5f40..e3038fa626ffc0 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1399,16 +1399,17 @@ alloc_threadstate(void) static void free_threadstate(_PyThreadStateImpl *tstate) { + PyInterpreterState *interp = tstate->base.interp; // The initial thread state of the interpreter is allocated // as part of the interpreter state so should not be freed. - if (tstate == &tstate->base.interp->_initial_thread) { + if (tstate == &interp->_initial_thread) { // Restore to _PyThreadState_INIT. memcpy(tstate, &initial._main_interpreter._initial_thread, sizeof(*tstate)); - _Py_atomic_store_int_relaxed( - &tstate->base.interp->threads.used_initial, - 0); + _Py_atomic_store_int( + &interp->threads.used_initial, + 0); } else { PyMem_RawFree(tstate); @@ -1549,7 +1550,6 @@ new_threadstate(PyInterpreterState *interp, int whence) if (set_initial == 1) { // The initial thread state is not in use, and we successfully // claimed it! - assert(old_head == NULL); assert(_Py_atomic_load_int_relaxed(&interp->threads.used_initial) == 1); used_newtstate = 0; tstate = &interp->_initial_thread; From 931cea85ea9fbc7dcdac6860b015c91627501115 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 17 Nov 2024 15:38:25 -0500 Subject: [PATCH 8/8] No need for a fancy compare-exchange. --- Python/pystate.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index e3038fa626ffc0..fdd11476a88aac 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1537,20 +1537,16 @@ new_threadstate(PyInterpreterState *interp, int whence) * before the thread is fully cleaned up, it's possible * for it to still be in use while interp->threads.head is NULL. * - * So, an atomic compare-exchange is used here to properly only let one - * thread access the initial thread at a time. + * So, we have a dedicated atomic field to make sure that only one + * thread accesses it at a time. */ - int expected = 0; // Initial is available - int set_initial = _Py_atomic_compare_exchange_int( - &interp->threads.used_initial, - &expected, 1); + int used_initial = _Py_atomic_load_int_relaxed(&interp->threads.used_initial); // Allocate the thread state and add it to the interpreter. PyThreadState *old_head = interp->threads.head; - if (set_initial == 1) { - // The initial thread state is not in use, and we successfully - // claimed it! - assert(_Py_atomic_load_int_relaxed(&interp->threads.used_initial) == 1); + if (used_initial == 0) { + // The initial thread state is not in use. + _Py_atomic_store_int_relaxed(&interp->threads.used_initial, 1); used_newtstate = 0; tstate = &interp->_initial_thread; }