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/Lib/test/test_interpreters/test_stress.py b/Lib/test/test_interpreters/test_stress.py index e400535b2a0e4e..6f416d071cf850 100644 --- a/Lib/test/test_interpreters/test_stress.py +++ b/Lib/test/test_interpreters/test_stress.py @@ -32,6 +32,24 @@ def task(): with threading_helper.start_threads(threads): pass + @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(): + 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. 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. diff --git a/Python/pystate.c b/Python/pystate.c index 24ee73c145cbcc..fdd11476a88aac 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1399,13 +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( + &interp->threads.used_initial, + 0); } else { PyMem_RawFree(tstate); @@ -1526,18 +1530,30 @@ 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, we have a dedicated atomic field to make sure that only one + * thread accesses it at a time. + */ + 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 (old_head == NULL) { - // It's the interpreter's initial thread state. + 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; } - // 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.