Skip to content

Commit c17f28a

Browse files
committed
gh-158195: Defer critical section restoration after STW
Restore critical sections *after* releasing the stop-the-world mutex, rather than before. This prevents deadlocks if there is a pinned mutex, e.g. as done by type_lock_prevent_release().
1 parent 1b015e6 commit c17f28a

4 files changed

Lines changed: 263 additions & 2 deletions

File tree

‎Include/internal/pycore_interp_structs.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,10 @@ struct _stoptheworld_state {
421421
Py_ssize_t thread_countdown; // Number of threads that must pause.
422422

423423
PyThreadState *requester; // Thread that requested the pause (may be NULL).
424+
425+
// Critical sections suspended by a requester waiting for mutex. They are
426+
// restored after both stop-the-world locks have been released.
427+
uintptr_t requester_critical_section;
424428
};
425429

426430
/* Tracks some rare events per-interpreter, used by the optimizer to turn on/off
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix a deadlock in the :term:`free-threaded build` when a thread holding a
2+
critical section requested a stop-the-world pause while another thread was
3+
waiting for the pause with the same lock held. The waiting thread no longer
4+
re-acquires its critical sections until the pause ends.

‎Modules/_testinternalcapi/test_critical_sections.c‎

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
#include "parts.h"
66
#include "pycore_critical_section.h"
7+
#include "pycore_interp_structs.h" // _stoptheworld_state
8+
#include "pycore_lock.h" // _Py_HAS_PARKED
79
#include "pycore_pystate.h"
810
#include "pycore_pythread.h"
911

@@ -464,6 +466,209 @@ test_critical_sections_stw(PyObject *self, PyObject *Py_UNUSED(args))
464466

465467
#endif // Py_CAN_START_THREADS
466468

469+
#if defined(Py_GIL_DISABLED) && defined(Py_CAN_START_THREADS)
470+
471+
// gh-158195: Test that a thread waiting to stop the world does not try to
472+
// re-acquire its critical sections while it owns the stop-the-world mutex.
473+
//
474+
// The main thread holds the interpreter's stop-the-world mutex so that both
475+
// worker threads must wait for it:
476+
//
477+
// 1. The "waiter" thread holds a critical section on `obj` and requests
478+
// stop-the-world. It parks on the STW mutex and, because it detaches
479+
// while waiting, releases the mutex of `obj`.
480+
// 2. The "pinned" thread locks `obj->ob_mutex` directly, which mimics a
481+
// pinned critical section: the mutex is not released when the thread
482+
// detaches. It then requests stop-the-world and parks behind the waiter.
483+
// 3. The main thread releases the STW mutex. The waiter acquires it.
484+
//
485+
// Before the fix, the waiter re-acquired `obj` when it re-attached while
486+
// owning the STW mutex, and blocked forever: the pinned thread holds `obj`
487+
// and waits for the STW mutex. This test deadlocks on builds without the fix.
488+
489+
enum {
490+
STW_HANDOFF_ONE_MUTEX,
491+
STW_HANDOFF_TWO_MUTEXES,
492+
STW_HANDOFF_NESTED,
493+
};
494+
495+
struct test_data_stw_handoff {
496+
PyObject *obj;
497+
PyObject *other;
498+
PyObject *inner;
499+
int mode;
500+
PyMutex *stw_mutex;
501+
PyThreadState *pinned_tstate;
502+
};
503+
504+
static void
505+
detached_sleep(int ms)
506+
{
507+
Py_BEGIN_ALLOW_THREADS
508+
pysleep(ms);
509+
Py_END_ALLOW_THREADS
510+
}
511+
512+
static void
513+
stw_handoff_stop_start(struct test_data_stw_handoff *test_data)
514+
{
515+
PyInterpreterState *interp = PyInterpreterState_Get();
516+
_PyEval_StopTheWorld(interp);
517+
// A critical section begun and ended while the world is stopped must not
518+
// resume the critical sections suspended by the STW request.
519+
Py_BEGIN_CRITICAL_SECTION(test_data->inner);
520+
Py_END_CRITICAL_SECTION();
521+
_PyEval_StartTheWorld(interp);
522+
523+
// The critical section is restored by the time StartTheWorld returns.
524+
PyThreadState *tstate = PyThreadState_GET();
525+
assert(_PyCriticalSection_IsActive(tstate->critical_section));
526+
assert(PyMutex_IsLocked(&test_data->obj->ob_mutex));
527+
(void)tstate;
528+
}
529+
530+
static void
531+
thread_stw_handoff_waiter(void *arg)
532+
{
533+
struct test_data_stw_handoff *test_data = arg;
534+
PyGILState_STATE gil = PyGILState_Ensure();
535+
536+
switch (test_data->mode) {
537+
case STW_HANDOFF_ONE_MUTEX:
538+
Py_BEGIN_CRITICAL_SECTION(test_data->obj);
539+
stw_handoff_stop_start(test_data);
540+
Py_END_CRITICAL_SECTION();
541+
break;
542+
case STW_HANDOFF_TWO_MUTEXES:
543+
Py_BEGIN_CRITICAL_SECTION2(test_data->obj, test_data->other);
544+
stw_handoff_stop_start(test_data);
545+
assert(PyMutex_IsLocked(&test_data->other->ob_mutex));
546+
Py_END_CRITICAL_SECTION2();
547+
break;
548+
case STW_HANDOFF_NESTED:
549+
Py_BEGIN_CRITICAL_SECTION(test_data->other);
550+
Py_BEGIN_CRITICAL_SECTION(test_data->obj);
551+
stw_handoff_stop_start(test_data);
552+
Py_END_CRITICAL_SECTION();
553+
// Ending the inner section resumes the outer one.
554+
assert(PyMutex_IsLocked(&test_data->other->ob_mutex));
555+
Py_END_CRITICAL_SECTION();
556+
break;
557+
}
558+
559+
PyGILState_Release(gil);
560+
}
561+
562+
static void
563+
thread_stw_handoff_pinned(void *arg)
564+
{
565+
struct test_data_stw_handoff *test_data = arg;
566+
PyGILState_STATE gil = PyGILState_Ensure();
567+
568+
// Wait for the waiter thread to park on the STW mutex.
569+
while (!(_Py_atomic_load_uint8(&test_data->stw_mutex->_bits) &
570+
_Py_HAS_PARKED)) {
571+
detached_sleep(1);
572+
}
573+
574+
// Lock the mutex directly so that it stays locked while this thread is
575+
// detached, like a critical section pinned by type_lock_prevent_release().
576+
PyMutex_Lock(&test_data->obj->ob_mutex);
577+
_Py_atomic_store_ptr(&test_data->pinned_tstate, PyThreadState_GET());
578+
579+
PyInterpreterState *interp = PyInterpreterState_Get();
580+
_PyEval_StopTheWorld(interp);
581+
_PyEval_StartTheWorld(interp);
582+
PyMutex_Unlock(&test_data->obj->ob_mutex);
583+
584+
PyGILState_Release(gil);
585+
}
586+
587+
static PyObject *
588+
stw_handoff_run(int mode)
589+
{
590+
struct test_data_stw_handoff test_data = {
591+
.obj = PyDict_New(),
592+
.other = PyDict_New(),
593+
.inner = PyDict_New(),
594+
.mode = mode,
595+
.stw_mutex = &PyInterpreterState_Get()->stoptheworld.mutex,
596+
};
597+
if (test_data.obj == NULL || test_data.other == NULL ||
598+
test_data.inner == NULL)
599+
{
600+
goto error;
601+
}
602+
603+
// Hold the STW mutex so that both threads must wait for it. Avoid
604+
// anything that could stop the world (e.g., allocation triggering a GC)
605+
// until it is released.
606+
PyMutex_Lock(test_data.stw_mutex);
607+
608+
PyThread_handle_t handles[2];
609+
PyThread_ident_t idents[2];
610+
if (PyThread_start_joinable_thread(&thread_stw_handoff_waiter, &test_data,
611+
&idents[0], &handles[0]) != 0) {
612+
PyMutex_Unlock(test_data.stw_mutex);
613+
PyErr_SetString(PyExc_RuntimeError, "could not start thread");
614+
goto error;
615+
}
616+
if (PyThread_start_joinable_thread(&thread_stw_handoff_pinned, &test_data,
617+
&idents[1], &handles[1]) != 0) {
618+
Py_FatalError("could not start thread");
619+
}
620+
621+
// Wait for the pinned thread to park on the STW mutex. It parks before
622+
// detaching, so once it is detached it is queued behind the waiter.
623+
for (;;) {
624+
PyThreadState *t = _Py_atomic_load_ptr(&test_data.pinned_tstate);
625+
if (t != NULL &&
626+
_Py_atomic_load_int(&t->state) == _Py_THREAD_DETACHED) {
627+
break;
628+
}
629+
detached_sleep(1);
630+
}
631+
632+
PyMutex_Unlock(test_data.stw_mutex);
633+
634+
Py_BEGIN_ALLOW_THREADS
635+
PyThread_join_thread(handles[0]);
636+
PyThread_join_thread(handles[1]);
637+
Py_END_ALLOW_THREADS
638+
639+
Py_DECREF(test_data.obj);
640+
Py_DECREF(test_data.other);
641+
Py_DECREF(test_data.inner);
642+
Py_RETURN_NONE;
643+
644+
error:
645+
Py_XDECREF(test_data.obj);
646+
Py_XDECREF(test_data.other);
647+
Py_XDECREF(test_data.inner);
648+
return NULL;
649+
}
650+
651+
static PyObject *
652+
test_critical_section_stw_handoff(PyObject *self, PyObject *Py_UNUSED(args))
653+
{
654+
return stw_handoff_run(STW_HANDOFF_ONE_MUTEX);
655+
}
656+
657+
static PyObject *
658+
test_critical_section2_stw_handoff(PyObject *self, PyObject *Py_UNUSED(args))
659+
{
660+
return stw_handoff_run(STW_HANDOFF_TWO_MUTEXES);
661+
}
662+
663+
static PyObject *
664+
test_critical_section_nested_stw_handoff(PyObject *self,
665+
PyObject *Py_UNUSED(args))
666+
{
667+
return stw_handoff_run(STW_HANDOFF_NESTED);
668+
}
669+
670+
#endif // Py_GIL_DISABLED && Py_CAN_START_THREADS
671+
467672
static PyMethodDef test_methods[] = {
468673
{"test_critical_sections", test_critical_sections, METH_NOARGS},
469674
{"test_critical_sections_nest", test_critical_sections_nest, METH_NOARGS},
@@ -476,6 +681,14 @@ static PyMethodDef test_methods[] = {
476681
{"test_critical_sections_threads", test_critical_sections_threads, METH_NOARGS},
477682
{"test_critical_sections_gc", test_critical_sections_gc, METH_NOARGS},
478683
{"test_critical_sections_stw", test_critical_sections_stw, METH_NOARGS},
684+
#endif
685+
#if defined(Py_GIL_DISABLED) && defined(Py_CAN_START_THREADS)
686+
{"test_critical_section_stw_handoff",
687+
test_critical_section_stw_handoff, METH_NOARGS},
688+
{"test_critical_section2_stw_handoff",
689+
test_critical_section2_stw_handoff, METH_NOARGS},
690+
{"test_critical_section_nested_stw_handoff",
691+
test_critical_section_nested_stw_handoff, METH_NOARGS},
479692
#endif
480693
{NULL, NULL} /* sentinel */
481694
};

‎Python/pystate.c‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2524,13 +2524,36 @@ stop_the_world(struct _stoptheworld_state *stw)
25242524
else {
25252525
_PyRWMutex_RLock(&runtime->stoptheworld_mutex);
25262526
}
2527-
PyMutex_Lock(&stw->mutex);
2527+
2528+
uintptr_t requester_critical_section = 0;
2529+
PyThreadState *requester = _PyThreadState_GET();
2530+
if (!PyMutex_LockFast(&stw->mutex)) {
2531+
if (requester != NULL && requester->critical_section != 0) {
2532+
// gh-158195: Do not resume our critical sections while owning the
2533+
// STW mutex. Another thread waiting for the mutex may hold one of
2534+
// their locks without releasing it on detach (see
2535+
// type_lock_prevent_release() in typeobject.c), which would
2536+
// deadlock. Detach (suspending the critical sections) and hide
2537+
// them from _PyThreadState_Attach(). start_the_world() restores
2538+
// them after releasing the STW locks.
2539+
_PyThreadState_Detach(requester);
2540+
requester_critical_section = requester->critical_section;
2541+
assert(requester_critical_section & _Py_CRITICAL_SECTION_INACTIVE);
2542+
requester->critical_section = 0;
2543+
_PyMutex_LockTimed(&stw->mutex, -1, _Py_LOCK_DONT_DETACH);
2544+
_PyThreadState_Attach(requester);
2545+
}
2546+
else {
2547+
PyMutex_Lock(&stw->mutex);
2548+
}
2549+
}
25282550

25292551
HEAD_LOCK(runtime);
25302552
stw->requested = 1;
25312553
stw->thread_countdown = 0;
25322554
stw->stop_event = (PyEvent){0}; // zero-initialize (unset)
2533-
stw->requester = _PyThreadState_GET(); // may be NULL
2555+
stw->requester = requester; // may be NULL
2556+
stw->requester_critical_section = requester_critical_section;
25342557
FT_STAT_WORLD_STOP_INC();
25352558

25362559
_Py_FOR_EACH_STW_INTERP(stw, i) {
@@ -2575,7 +2598,12 @@ start_the_world(struct _stoptheworld_state *stw)
25752598
_PyRuntimeState *runtime = &_PyRuntime;
25762599
assert(PyMutex_IsLocked(&stw->mutex));
25772600

2601+
PyThreadState *requester;
2602+
uintptr_t requester_critical_section;
2603+
25782604
HEAD_LOCK(runtime);
2605+
requester = stw->requester;
2606+
requester_critical_section = stw->requester_critical_section;
25792607
stw->requested = 0;
25802608
stw->world_stopped = 0;
25812609
// Switch threads back to the detached state.
@@ -2587,6 +2615,7 @@ start_the_world(struct _stoptheworld_state *stw)
25872615
}
25882616
}
25892617
stw->requester = NULL;
2618+
stw->requester_critical_section = 0;
25902619
HEAD_UNLOCK(runtime);
25912620
PyMutex_Unlock(&stw->mutex);
25922621
if (stw->is_global) {
@@ -2595,6 +2624,17 @@ start_the_world(struct _stoptheworld_state *stw)
25952624
else {
25962625
_PyRWMutex_RUnlock(&runtime->stoptheworld_mutex);
25972626
}
2627+
2628+
if (requester_critical_section != 0) {
2629+
// Restore the critical sections hidden by stop_the_world(). This may
2630+
// block, so it must happen after releasing the STW locks. Critical
2631+
// sections begun while the world was stopped must have ended.
2632+
assert(requester != NULL);
2633+
assert(requester == _PyThreadState_GET());
2634+
assert(requester->critical_section == 0);
2635+
requester->critical_section = requester_critical_section;
2636+
_PyCriticalSection_Resume(requester);
2637+
}
25982638
}
25992639
#endif // Py_GIL_DISABLED
26002640

0 commit comments

Comments
 (0)