From 9ecb5b50ae9d735ecdfe24f55aa420d01d72dc8d Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 8 Apr 2025 20:36:29 -0400 Subject: [PATCH 1/9] I think this works. --- Include/internal/pycore_pystate.h | 3 +++ Python/ceval.c | 6 ++++++ Python/pystate.c | 16 ++++++++++++++++ 3 files changed, 25 insertions(+) diff --git a/Include/internal/pycore_pystate.h b/Include/internal/pycore_pystate.h index 864e0f5d1db289..d1bdf15ef7eed6 100644 --- a/Include/internal/pycore_pystate.h +++ b/Include/internal/pycore_pystate.h @@ -296,6 +296,9 @@ _Py_AssertHoldsTstateFunc(const char *func) #define _Py_AssertHoldsTstate() #endif +void _PyThreadState_Decref(PyThreadState *tstate); +void _PyThreadState_Incref(PyThreadState *tstate); + #ifdef __cplusplus } #endif diff --git a/Python/ceval.c b/Python/ceval.c index a59b2b7a16866d..c447be2fd99b72 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -2467,6 +2467,10 @@ PyEval_SetTraceAllThreads(Py_tracefunc func, PyObject *arg) _PyRuntimeState *runtime = &_PyRuntime; HEAD_LOCK(runtime); PyThreadState* ts = PyInterpreterState_ThreadHead(interp); + /* gh-132296: We need to prevent the thread state + from getting concurrently deallocated. + We can't stop-the-world because _PyEval_SetTrace() is re-entrant. */ + _PyThreadState_Incref(ts); HEAD_UNLOCK(runtime); while (ts) { @@ -2474,7 +2478,9 @@ PyEval_SetTraceAllThreads(Py_tracefunc func, PyObject *arg) PyErr_FormatUnraisable("Exception ignored in PyEval_SetTraceAllThreads"); } HEAD_LOCK(runtime); + PyThreadState *old = ts; ts = PyThreadState_Next(ts); + _PyThreadState_Decref(old); HEAD_UNLOCK(runtime); } } diff --git a/Python/pystate.c b/Python/pystate.c index ee35f0fa945f8b..6cc690aed9fda0 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1485,6 +1485,22 @@ decref_threadstate(_PyThreadStateImpl *tstate) } } +static void +_PyThreadState_Decref(PyThreadState *tstate) +{ + assert(tstate != NULL); + _PyThreadStateImpl *impl = (_PyThreadStateImpl *)tstate; + decref_threadstate(tstate); +} + +static void +_PyThreadState_Incref(PyThreadState *tstate) +{ + assert(tstate != NULL); + _PyThreadStateImpl *impl = (_PyThreadStateImpl *)tstate; + _Py_atomic_add_ssize(&impl->refcount, 1); +} + /* Get the thread state to a minimal consistent state. Further init happens in pylifecycle.c before it can be used. All fields not initialized here are expected to be zeroed out, From 13694ecae97928e19f80fbd87348fc72fa23a4dc Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 8 Apr 2025 20:37:09 -0400 Subject: [PATCH 2/9] Oops --- Python/pystate.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index 6cc690aed9fda0..724d4856df4a43 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1485,7 +1485,7 @@ decref_threadstate(_PyThreadStateImpl *tstate) } } -static void +void _PyThreadState_Decref(PyThreadState *tstate) { assert(tstate != NULL); @@ -1493,7 +1493,7 @@ _PyThreadState_Decref(PyThreadState *tstate) decref_threadstate(tstate); } -static void +void _PyThreadState_Incref(PyThreadState *tstate) { assert(tstate != NULL); From d820ba2f1817ab3a658c4bd5c07db18a17b9c121 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 8 Apr 2025 20:37:45 -0400 Subject: [PATCH 3/9] Double oops --- Python/pystate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/pystate.c b/Python/pystate.c index 724d4856df4a43..b40ee232f74629 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1490,7 +1490,7 @@ _PyThreadState_Decref(PyThreadState *tstate) { assert(tstate != NULL); _PyThreadStateImpl *impl = (_PyThreadStateImpl *)tstate; - decref_threadstate(tstate); + decref_threadstate(impl); } void From 87e887050f2c7b66433dd3734db883d735b3aa59 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 8 Apr 2025 21:31:24 -0400 Subject: [PATCH 4/9] This approach sort of works. --- Python/ceval.c | 17 ++++++++++++----- Python/pystate.c | 4 ++-- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index c447be2fd99b72..a90afda10eba80 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -2467,21 +2467,28 @@ PyEval_SetTraceAllThreads(Py_tracefunc func, PyObject *arg) _PyRuntimeState *runtime = &_PyRuntime; HEAD_LOCK(runtime); PyThreadState* ts = PyInterpreterState_ThreadHead(interp); - /* gh-132296: We need to prevent the thread state - from getting concurrently deallocated. - We can't stop-the-world because _PyEval_SetTrace() is re-entrant. */ + /* gh-132296: We need to prevent the thread state from being concurrently + deallocated. We can't stop-the-world because _PyEval_SetTrace() + is re-entrant. */ _PyThreadState_Incref(ts); HEAD_UNLOCK(runtime); - while (ts) { + while (ts != NULL) { + /* if (_PyEval_SetTrace(ts, func, arg) < 0) { PyErr_FormatUnraisable("Exception ignored in PyEval_SetTraceAllThreads"); } + */ HEAD_LOCK(runtime); PyThreadState *old = ts; ts = PyThreadState_Next(ts); - _PyThreadState_Decref(old); + /* Drop the reference to the prior thread state + and acquire a reference to the next one. */ + if (ts != NULL) { + _PyThreadState_Incref(ts); + } HEAD_UNLOCK(runtime); + _PyThreadState_Decref(old); } } diff --git a/Python/pystate.c b/Python/pystate.c index b40ee232f74629..d7e0b9e8eedbc0 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1898,7 +1898,7 @@ PyThreadState_Delete(PyThreadState *tstate) _Py_EnsureTstateNotNULL(tstate); tstate_verify_not_active(tstate); tstate_delete_common(tstate, 0); - free_threadstate((_PyThreadStateImpl *)tstate); + decref_threadstate((_PyThreadStateImpl *)tstate); } @@ -1911,7 +1911,7 @@ _PyThreadState_DeleteCurrent(PyThreadState *tstate) #endif current_fast_clear(tstate->interp->runtime); tstate_delete_common(tstate, 1); // release GIL as part of call - free_threadstate((_PyThreadStateImpl *)tstate); + decref_threadstate((_PyThreadStateImpl *)tstate); } void From 3169e82db0b1ab2661b58673d914b7b6f2aa0dfb Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 8 Apr 2025 22:00:08 -0400 Subject: [PATCH 5/9] Use the reference count in PyThreadState_Delete() --- Python/pystate.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index d7e0b9e8eedbc0..b15c3d0398ff90 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1896,9 +1896,15 @@ void PyThreadState_Delete(PyThreadState *tstate) { _Py_EnsureTstateNotNULL(tstate); - tstate_verify_not_active(tstate); - tstate_delete_common(tstate, 0); - decref_threadstate((_PyThreadStateImpl *)tstate); + _PyThreadStateImpl *impl = (_PyThreadStateImpl *)tstate; + if (_Py_atomic_add_ssize(&impl->refcount, -1) == 2) { + /* Treat the interpreter's reference (via the linked list) as weak, + even though it's technically strong. This is because we want + to free the thread state now, rather than wait for finalization. */ + tstate_verify_not_active(tstate); + tstate_delete_common(tstate, 0); + free_threadstate((_PyThreadStateImpl *)tstate); + } } From 0a9bbfa9888ae30c5c3221c48bfbcaa88937ea5b Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 8 Apr 2025 22:00:56 -0400 Subject: [PATCH 6/9] Yay it works --- Python/ceval.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/Python/ceval.c b/Python/ceval.c index a90afda10eba80..02a99690c9a29c 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -2474,11 +2474,9 @@ PyEval_SetTraceAllThreads(Py_tracefunc func, PyObject *arg) HEAD_UNLOCK(runtime); while (ts != NULL) { - /* if (_PyEval_SetTrace(ts, func, arg) < 0) { PyErr_FormatUnraisable("Exception ignored in PyEval_SetTraceAllThreads"); } - */ HEAD_LOCK(runtime); PyThreadState *old = ts; ts = PyThreadState_Next(ts); From 136f4d85122e6b969c050b72c28616eb473d6bd4 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 8 Apr 2025 22:07:11 -0400 Subject: [PATCH 7/9] Add a test. --- Lib/test/test_threading.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index fa666608263e27..2fd5fa4e3fb763 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1039,6 +1039,22 @@ def checker(): self.assertEqual(threading.gettrace(), old_trace) self.assertEqual(sys.gettrace(), old_trace) + @unittest.skipUnless(support.Py_GIL_DISABLED, "only meaningful under free-threading") + def test_settrace_all_threads_race(self): + # GH-132296: settrace_all_threads() could be racy on the free-threaded build + #if the threads were concurrently deleted. + def dummy(*args): + pass + + def do_settrace(): + threading.settrace_all_threads(dummy) + + with threading_helper.catch_threading_exception() as cm: + with threading_helper.start_threads((threading.Thread(target=do_settrace) for _ in range(8))): + pass + + self.assertIsNone(cm.exc_value) + def test_getprofile(self): def fn(*args): pass old_profile = threading.getprofile() From 3a10fe2027672de533ed50e9040939683e42eead Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 8 Apr 2025 22:08:09 -0400 Subject: [PATCH 8/9] Add blurb. --- .../next/C_API/2025-04-08-22-08-05.gh-issue-132296.n0hZYY.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/C_API/2025-04-08-22-08-05.gh-issue-132296.n0hZYY.rst diff --git a/Misc/NEWS.d/next/C_API/2025-04-08-22-08-05.gh-issue-132296.n0hZYY.rst b/Misc/NEWS.d/next/C_API/2025-04-08-22-08-05.gh-issue-132296.n0hZYY.rst new file mode 100644 index 00000000000000..b7f1721fe6474a --- /dev/null +++ b/Misc/NEWS.d/next/C_API/2025-04-08-22-08-05.gh-issue-132296.n0hZYY.rst @@ -0,0 +1,2 @@ +Fix a crash when using :c:func:`PyEval_SetTrace` on the :term:`free threaded +` build. From 8550c2905fcb57ed7c8f8faf330b8b9189b3a0bb Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Tue, 8 Apr 2025 22:27:21 -0400 Subject: [PATCH 9/9] Restore the old trace function at the end to make regrtest happy. --- Lib/test/test_threading.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 2fd5fa4e3fb763..036b12fb37ebcb 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1043,17 +1043,21 @@ def checker(): def test_settrace_all_threads_race(self): # GH-132296: settrace_all_threads() could be racy on the free-threaded build #if the threads were concurrently deleted. + old_trace = threading.gettrace() def dummy(*args): pass def do_settrace(): threading.settrace_all_threads(dummy) - with threading_helper.catch_threading_exception() as cm: - with threading_helper.start_threads((threading.Thread(target=do_settrace) for _ in range(8))): - pass + try: + with threading_helper.catch_threading_exception() as cm: + with threading_helper.start_threads((threading.Thread(target=do_settrace) for _ in range(8))): + pass - self.assertIsNone(cm.exc_value) + self.assertIsNone(cm.exc_value) + finally: + threading.settrace_all_threads(old_trace) def test_getprofile(self): def fn(*args): pass