diff --git a/Include/internal/pycore_interp_structs.h b/Include/internal/pycore_interp_structs.h index fe501be1afc59b1..885012a7121fd05 100644 --- a/Include/internal/pycore_interp_structs.h +++ b/Include/internal/pycore_interp_structs.h @@ -421,6 +421,10 @@ struct _stoptheworld_state { Py_ssize_t thread_countdown; // Number of threads that must pause. PyThreadState *requester; // Thread that requested the pause (may be NULL). + + // Critical sections suspended by a requester waiting for mutex. They are + // restored after both stop-the-world locks have been released. + uintptr_t requester_critical_section; }; /* Tracks some rare events per-interpreter, used by the optimizer to turn on/off diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-10-02-12-00-00.gh-issue-158195.Hn4kTq.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-10-02-12-00-00.gh-issue-158195.Hn4kTq.rst new file mode 100644 index 000000000000000..c628adaf5d012f1 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-10-02-12-00-00.gh-issue-158195.Hn4kTq.rst @@ -0,0 +1,4 @@ +Fix a deadlock in the :term:`free-threaded build` when a thread holding a +critical section requested a stop-the-world pause while another thread was +waiting for the pause with the same lock held. The waiting thread no longer +re-acquires its critical sections until the pause ends. diff --git a/Modules/_testinternalcapi/test_critical_sections.c b/Modules/_testinternalcapi/test_critical_sections.c index 72a1fa2cdc7224d..b803f3de4a72e2e 100644 --- a/Modules/_testinternalcapi/test_critical_sections.c +++ b/Modules/_testinternalcapi/test_critical_sections.c @@ -4,6 +4,8 @@ #include "parts.h" #include "pycore_critical_section.h" +#include "pycore_interp_structs.h" // _stoptheworld_state +#include "pycore_lock.h" // _Py_HAS_PARKED #include "pycore_pystate.h" #include "pycore_pythread.h" @@ -464,6 +466,209 @@ test_critical_sections_stw(PyObject *self, PyObject *Py_UNUSED(args)) #endif // Py_CAN_START_THREADS +#if defined(Py_GIL_DISABLED) && defined(Py_CAN_START_THREADS) + +// gh-158195: Test that a thread waiting to stop the world does not try to +// re-acquire its critical sections while it owns the stop-the-world mutex. +// +// The main thread holds the interpreter's stop-the-world mutex so that both +// worker threads must wait for it: +// +// 1. The "waiter" thread holds a critical section on `obj` and requests +// stop-the-world. It parks on the STW mutex and, because it detaches +// while waiting, releases the mutex of `obj`. +// 2. The "pinned" thread locks `obj->ob_mutex` directly, which mimics a +// pinned critical section: the mutex is not released when the thread +// detaches. It then requests stop-the-world and parks behind the waiter. +// 3. The main thread releases the STW mutex. The waiter acquires it. +// +// Before the fix, the waiter re-acquired `obj` when it re-attached while +// owning the STW mutex, and blocked forever: the pinned thread holds `obj` +// and waits for the STW mutex. This test deadlocks on builds without the fix. + +enum { + STW_HANDOFF_ONE_MUTEX, + STW_HANDOFF_TWO_MUTEXES, + STW_HANDOFF_NESTED, +}; + +struct test_data_stw_handoff { + PyObject *obj; + PyObject *other; + PyObject *inner; + int mode; + PyMutex *stw_mutex; + PyThreadState *pinned_tstate; +}; + +static void +detached_sleep(int ms) +{ + Py_BEGIN_ALLOW_THREADS + pysleep(ms); + Py_END_ALLOW_THREADS +} + +static void +stw_handoff_stop_start(struct test_data_stw_handoff *test_data) +{ + PyInterpreterState *interp = PyInterpreterState_Get(); + _PyEval_StopTheWorld(interp); + // A critical section begun and ended while the world is stopped must not + // resume the critical sections suspended by the STW request. + Py_BEGIN_CRITICAL_SECTION(test_data->inner); + Py_END_CRITICAL_SECTION(); + _PyEval_StartTheWorld(interp); + + // The critical section is restored by the time StartTheWorld returns. + PyThreadState *tstate = PyThreadState_GET(); + assert(_PyCriticalSection_IsActive(tstate->critical_section)); + assert(PyMutex_IsLocked(&test_data->obj->ob_mutex)); + (void)tstate; +} + +static void +thread_stw_handoff_waiter(void *arg) +{ + struct test_data_stw_handoff *test_data = arg; + PyGILState_STATE gil = PyGILState_Ensure(); + + switch (test_data->mode) { + case STW_HANDOFF_ONE_MUTEX: + Py_BEGIN_CRITICAL_SECTION(test_data->obj); + stw_handoff_stop_start(test_data); + Py_END_CRITICAL_SECTION(); + break; + case STW_HANDOFF_TWO_MUTEXES: + Py_BEGIN_CRITICAL_SECTION2(test_data->obj, test_data->other); + stw_handoff_stop_start(test_data); + assert(PyMutex_IsLocked(&test_data->other->ob_mutex)); + Py_END_CRITICAL_SECTION2(); + break; + case STW_HANDOFF_NESTED: + Py_BEGIN_CRITICAL_SECTION(test_data->other); + Py_BEGIN_CRITICAL_SECTION(test_data->obj); + stw_handoff_stop_start(test_data); + Py_END_CRITICAL_SECTION(); + // Ending the inner section resumes the outer one. + assert(PyMutex_IsLocked(&test_data->other->ob_mutex)); + Py_END_CRITICAL_SECTION(); + break; + } + + PyGILState_Release(gil); +} + +static void +thread_stw_handoff_pinned(void *arg) +{ + struct test_data_stw_handoff *test_data = arg; + PyGILState_STATE gil = PyGILState_Ensure(); + + // Wait for the waiter thread to park on the STW mutex. + while (!(_Py_atomic_load_uint8(&test_data->stw_mutex->_bits) & + _Py_HAS_PARKED)) { + detached_sleep(1); + } + + // Lock the mutex directly so that it stays locked while this thread is + // detached, like a critical section pinned by type_lock_prevent_release(). + PyMutex_Lock(&test_data->obj->ob_mutex); + _Py_atomic_store_ptr(&test_data->pinned_tstate, PyThreadState_GET()); + + PyInterpreterState *interp = PyInterpreterState_Get(); + _PyEval_StopTheWorld(interp); + _PyEval_StartTheWorld(interp); + PyMutex_Unlock(&test_data->obj->ob_mutex); + + PyGILState_Release(gil); +} + +static PyObject * +stw_handoff_run(int mode) +{ + struct test_data_stw_handoff test_data = { + .obj = PyDict_New(), + .other = PyDict_New(), + .inner = PyDict_New(), + .mode = mode, + .stw_mutex = &PyInterpreterState_Get()->stoptheworld.mutex, + }; + if (test_data.obj == NULL || test_data.other == NULL || + test_data.inner == NULL) + { + goto error; + } + + // Hold the STW mutex so that both threads must wait for it. Avoid + // anything that could stop the world (e.g., allocation triggering a GC) + // until it is released. + PyMutex_Lock(test_data.stw_mutex); + + PyThread_handle_t handles[2]; + PyThread_ident_t idents[2]; + if (PyThread_start_joinable_thread(&thread_stw_handoff_waiter, &test_data, + &idents[0], &handles[0]) != 0) { + PyMutex_Unlock(test_data.stw_mutex); + PyErr_SetString(PyExc_RuntimeError, "could not start thread"); + goto error; + } + if (PyThread_start_joinable_thread(&thread_stw_handoff_pinned, &test_data, + &idents[1], &handles[1]) != 0) { + Py_FatalError("could not start thread"); + } + + // Wait for the pinned thread to park on the STW mutex. It parks before + // detaching, so once it is detached it is queued behind the waiter. + for (;;) { + PyThreadState *t = _Py_atomic_load_ptr(&test_data.pinned_tstate); + if (t != NULL && + _Py_atomic_load_int(&t->state) == _Py_THREAD_DETACHED) { + break; + } + detached_sleep(1); + } + + PyMutex_Unlock(test_data.stw_mutex); + + Py_BEGIN_ALLOW_THREADS + PyThread_join_thread(handles[0]); + PyThread_join_thread(handles[1]); + Py_END_ALLOW_THREADS + + Py_DECREF(test_data.obj); + Py_DECREF(test_data.other); + Py_DECREF(test_data.inner); + Py_RETURN_NONE; + +error: + Py_XDECREF(test_data.obj); + Py_XDECREF(test_data.other); + Py_XDECREF(test_data.inner); + return NULL; +} + +static PyObject * +test_critical_section_stw_handoff(PyObject *self, PyObject *Py_UNUSED(args)) +{ + return stw_handoff_run(STW_HANDOFF_ONE_MUTEX); +} + +static PyObject * +test_critical_section2_stw_handoff(PyObject *self, PyObject *Py_UNUSED(args)) +{ + return stw_handoff_run(STW_HANDOFF_TWO_MUTEXES); +} + +static PyObject * +test_critical_section_nested_stw_handoff(PyObject *self, + PyObject *Py_UNUSED(args)) +{ + return stw_handoff_run(STW_HANDOFF_NESTED); +} + +#endif // Py_GIL_DISABLED && Py_CAN_START_THREADS + static PyMethodDef test_methods[] = { {"test_critical_sections", test_critical_sections, METH_NOARGS}, {"test_critical_sections_nest", test_critical_sections_nest, METH_NOARGS}, @@ -476,6 +681,14 @@ static PyMethodDef test_methods[] = { {"test_critical_sections_threads", test_critical_sections_threads, METH_NOARGS}, {"test_critical_sections_gc", test_critical_sections_gc, METH_NOARGS}, {"test_critical_sections_stw", test_critical_sections_stw, METH_NOARGS}, +#endif +#if defined(Py_GIL_DISABLED) && defined(Py_CAN_START_THREADS) + {"test_critical_section_stw_handoff", + test_critical_section_stw_handoff, METH_NOARGS}, + {"test_critical_section2_stw_handoff", + test_critical_section2_stw_handoff, METH_NOARGS}, + {"test_critical_section_nested_stw_handoff", + test_critical_section_nested_stw_handoff, METH_NOARGS}, #endif {NULL, NULL} /* sentinel */ }; diff --git a/Python/pystate.c b/Python/pystate.c index 97323ea2ba2bce1..09d3e1d62c8662f 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2524,13 +2524,36 @@ stop_the_world(struct _stoptheworld_state *stw) else { _PyRWMutex_RLock(&runtime->stoptheworld_mutex); } - PyMutex_Lock(&stw->mutex); + + uintptr_t requester_critical_section = 0; + PyThreadState *requester = _PyThreadState_GET(); + if (!PyMutex_LockFast(&stw->mutex)) { + if (requester != NULL && requester->critical_section != 0) { + // gh-158195: Do not resume our critical sections while owning the + // STW mutex. Another thread waiting for the mutex may hold one of + // their locks without releasing it on detach (see + // type_lock_prevent_release() in typeobject.c), which would + // deadlock. Detach (suspending the critical sections) and hide + // them from _PyThreadState_Attach(). start_the_world() restores + // them after releasing the STW locks. + _PyThreadState_Detach(requester); + requester_critical_section = requester->critical_section; + assert(requester_critical_section & _Py_CRITICAL_SECTION_INACTIVE); + requester->critical_section = 0; + _PyMutex_LockTimed(&stw->mutex, -1, _Py_LOCK_DONT_DETACH); + _PyThreadState_Attach(requester); + } + else { + PyMutex_Lock(&stw->mutex); + } + } HEAD_LOCK(runtime); stw->requested = 1; stw->thread_countdown = 0; stw->stop_event = (PyEvent){0}; // zero-initialize (unset) - stw->requester = _PyThreadState_GET(); // may be NULL + stw->requester = requester; // may be NULL + stw->requester_critical_section = requester_critical_section; FT_STAT_WORLD_STOP_INC(); _Py_FOR_EACH_STW_INTERP(stw, i) { @@ -2575,7 +2598,12 @@ start_the_world(struct _stoptheworld_state *stw) _PyRuntimeState *runtime = &_PyRuntime; assert(PyMutex_IsLocked(&stw->mutex)); + PyThreadState *requester; + uintptr_t requester_critical_section; + HEAD_LOCK(runtime); + requester = stw->requester; + requester_critical_section = stw->requester_critical_section; stw->requested = 0; stw->world_stopped = 0; // Switch threads back to the detached state. @@ -2587,6 +2615,7 @@ start_the_world(struct _stoptheworld_state *stw) } } stw->requester = NULL; + stw->requester_critical_section = 0; HEAD_UNLOCK(runtime); PyMutex_Unlock(&stw->mutex); if (stw->is_global) { @@ -2595,6 +2624,17 @@ start_the_world(struct _stoptheworld_state *stw) else { _PyRWMutex_RUnlock(&runtime->stoptheworld_mutex); } + + if (requester_critical_section != 0) { + // Restore the critical sections hidden by stop_the_world(). This may + // block, so it must happen after releasing the STW locks. Critical + // sections begun while the world was stopped must have ended. + assert(requester != NULL); + assert(requester == _PyThreadState_GET()); + assert(requester->critical_section == 0); + requester->critical_section = requester_critical_section; + _PyCriticalSection_Resume(requester); + } } #endif // Py_GIL_DISABLED