From 24a47e2c6f5de430a15f99c5bdc1d1c8750edcb5 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 29 May 2024 21:11:04 +0000 Subject: [PATCH 1/2] gh-119585: Fix crash involving `PyGILState_Release()` and `PyThreadState_Clear()` Don't decrement `gilstate_counter` in `PyGILState_Release()` until after `PyThreadState_Clear()` is called. A destructor called from `PyThreadState_Clear()` may call back into `PyGILState_Ensure()` and PyGILState_Release()` and if `gilstate_counter` is zero, it will try to create a new thread state before the current active thread state is destroyed. --- Lib/test/test_capi/test_misc.py | 16 ++++++++++++++++ ...024-05-29-21-05-59.gh-issue-119585.Sn7JL3.rst | 5 +++++ Modules/_testcapimodule.c | 9 +++++++++ Python/pystate.c | 16 +++++++++++----- 4 files changed, 41 insertions(+), 5 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2024-05-29-21-05-59.gh-issue-119585.Sn7JL3.rst diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index ed42d7b64302f9..f3d16e4a2fc92a 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -2888,6 +2888,22 @@ def callback(): t.start() t.join() + @threading_helper.reap_threads + @threading_helper.requires_working_threading() + def test_thread_gilstate_in_clear(self): + # See https://github.com/python/cpython/issues/119585 + class C: + def __del__(self): + _testcapi.gilstate_ensure_release() + + # Thread-local variables are destroyed in `PyThreadState_Clear()`. + local_var = threading.local() + + def callback(): + local_var.x = C() + + _testcapi._test_thread_state(callback) + @threading_helper.reap_threads @threading_helper.requires_working_threading() def test_gilstate_ensure_no_deadlock(self): diff --git a/Misc/NEWS.d/next/C API/2024-05-29-21-05-59.gh-issue-119585.Sn7JL3.rst b/Misc/NEWS.d/next/C API/2024-05-29-21-05-59.gh-issue-119585.Sn7JL3.rst new file mode 100644 index 00000000000000..038dec2dbf90d1 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2024-05-29-21-05-59.gh-issue-119585.Sn7JL3.rst @@ -0,0 +1,5 @@ +Fix crash when a thread state that was created by :c:func:`PyGILState_Ensure` +calls a destructor that during :c:func:`PyThreadState_Clear` that +calls back into :c:func:`PyGILState_Ensure` and :c:func:`PyGILState_Release`. +This might occur when in the free-threaded build or when using thread-local +variables whose destructors call :c:func:`PyGILState_Ensure`. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index f99ebf0dde4f9e..b58c17260626c2 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -764,6 +764,14 @@ test_thread_state(PyObject *self, PyObject *args) Py_RETURN_NONE; } +static PyObject * +gilstate_ensure_release(PyObject *module, PyObject *Py_UNUSED(ignored)) +{ + PyGILState_STATE state = PyGILState_Ensure(); + PyGILState_Release(state); + Py_RETURN_NONE; +} + #ifndef MS_WINDOWS static PyThread_type_lock wait_done = NULL; @@ -3351,6 +3359,7 @@ static PyMethodDef TestMethods[] = { {"test_get_type_dict", test_get_type_dict, METH_NOARGS}, {"test_reftracer", test_reftracer, METH_NOARGS}, {"_test_thread_state", test_thread_state, METH_VARARGS}, + {"gilstate_ensure_release", gilstate_ensure_release, METH_NOARGS}, #ifndef MS_WINDOWS {"_spawn_pthread_waiter", spawn_pthread_waiter, METH_NOARGS}, {"_end_spawned_pthread", end_spawned_pthread, METH_NOARGS}, diff --git a/Python/pystate.c b/Python/pystate.c index 1ea1ad982a0ec9..bba529141d4989 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2798,17 +2798,20 @@ PyGILState_Release(PyGILState_STATE oldstate) tstate); } assert(holds_gil(tstate)); - --tstate->gilstate_counter; - assert(tstate->gilstate_counter >= 0); /* illegal counter value */ + assert(tstate->gilstate_counter >= 1); /* illegal counter value */ /* If we're going to destroy this thread-state, we must * clear it while the GIL is held, as destructors may run. */ - if (tstate->gilstate_counter == 0) { + if (tstate->gilstate_counter == 1) { /* can't have been locked when we created it */ assert(oldstate == PyGILState_UNLOCKED); // XXX Unbind tstate here. PyThreadState_Clear(tstate); + // gh-119585: decrement gilstate_counter after `PyThreadState_Clear()` + // because it may call destructors that themselves use + // PyGILState_Ensure and PyGILState_Release. + --tstate->gilstate_counter; /* Delete the thread-state. Note this releases the GIL too! * It's vital that the GIL be held here, to avoid shutdown * races; see bugs 225673 and 1061968 (that nasty bug has a @@ -2818,8 +2821,11 @@ PyGILState_Release(PyGILState_STATE oldstate) _PyThreadState_DeleteCurrent(tstate); } /* Release the lock if necessary */ - else if (oldstate == PyGILState_UNLOCKED) { - PyEval_SaveThread(); + else { + --tstate->gilstate_counter; + if (oldstate == PyGILState_UNLOCKED) { + PyEval_SaveThread(); + } } } From 5aa6a0f4ff79a68f292f3efced0424233aeb47f2 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 29 May 2024 21:22:08 +0000 Subject: [PATCH 2/2] Adjust PyGILState_Release modifications. Make them more similar to our patterns with refcounts around finalizers. --- Python/pystate.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index bba529141d4989..ad7e082ce0d37e 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -2798,34 +2798,34 @@ PyGILState_Release(PyGILState_STATE oldstate) tstate); } assert(holds_gil(tstate)); - assert(tstate->gilstate_counter >= 1); /* illegal counter value */ + --tstate->gilstate_counter; + assert(tstate->gilstate_counter >= 0); /* illegal counter value */ /* If we're going to destroy this thread-state, we must * clear it while the GIL is held, as destructors may run. */ - if (tstate->gilstate_counter == 1) { + if (tstate->gilstate_counter == 0) { /* can't have been locked when we created it */ assert(oldstate == PyGILState_UNLOCKED); // XXX Unbind tstate here. + // gh-119585: `PyThreadState_Clear()` may call destructors that + // themselves use PyGILState_Ensure and PyGILState_Release, so make + // sure that gilstate_counter is not zero when calling it. + ++tstate->gilstate_counter; PyThreadState_Clear(tstate); - // gh-119585: decrement gilstate_counter after `PyThreadState_Clear()` - // because it may call destructors that themselves use - // PyGILState_Ensure and PyGILState_Release. --tstate->gilstate_counter; /* Delete the thread-state. Note this releases the GIL too! * It's vital that the GIL be held here, to avoid shutdown * races; see bugs 225673 and 1061968 (that nasty bug has a * habit of coming back). */ + assert(tstate->gilstate_counter == 0); assert(current_fast_get() == tstate); _PyThreadState_DeleteCurrent(tstate); } /* Release the lock if necessary */ - else { - --tstate->gilstate_counter; - if (oldstate == PyGILState_UNLOCKED) { - PyEval_SaveThread(); - } + else if (oldstate == PyGILState_UNLOCKED) { + PyEval_SaveThread(); } }