From 115e4d271f8d0b627da1910bda7320c846ba6d4d Mon Sep 17 00:00:00 2001 From: Shamil Abdulaev Date: Thu, 16 Oct 2025 19:54:41 +0300 Subject: [PATCH 01/10] fix tsan --- Include/internal/pycore_interp_structs.h | 1 + Python/pystate.c | 29 ++++++++++++++++++++---- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/Include/internal/pycore_interp_structs.h b/Include/internal/pycore_interp_structs.h index 2124e76514f1af..4a6456d865c577 100644 --- a/Include/internal/pycore_interp_structs.h +++ b/Include/internal/pycore_interp_structs.h @@ -793,6 +793,7 @@ struct _is { int finalizing; uintptr_t last_restart_version; + Py_ssize_t _owners; struct pythreads { uint64_t next_unique_id; /* The linked list of threads, newest first. */ diff --git a/Python/pystate.c b/Python/pystate.c index dbed609f29aa07..a0ad26ab6cf7b8 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -485,6 +485,23 @@ free_interpreter(PyInterpreterState *interp) } } +static void +_finalize_and_free_interpreter(PyInterpreterState *interp) +{ + _Py_qsbr_fini(interp); + _PyObject_FiniState(interp); + free_interpreter(interp); +} + +static inline void +_interp_release_owner(PyInterpreterState *interp) +{ + Py_ssize_t prev = _Py_atomic_add_ssize(&interp->_owners, -1); + if (prev == 1) { + _finalize_and_free_interpreter(interp); + } +} + #ifndef NDEBUG static inline int check_interpreter_whence(long); #endif @@ -534,6 +551,7 @@ init_interpreter(PyInterpreterState *interp, interp->id = id; interp->id_refcount = 0; + interp->_owners = 1; assert(runtime->interpreters.head == interp); assert(next != NULL || (interp == runtime->interpreters.main)); @@ -955,11 +973,9 @@ PyInterpreterState_Delete(PyInterpreterState *interp) } HEAD_UNLOCK(runtime); - _Py_qsbr_fini(interp); - - _PyObject_FiniState(interp); + interp->finalizing = 1; - free_interpreter(interp); + _interp_release_owner(interp); } @@ -1412,6 +1428,7 @@ 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 == &interp->_initial_thread) { @@ -1423,6 +1440,8 @@ free_threadstate(_PyThreadStateImpl *tstate) else { PyMem_RawFree(tstate); } + + _interp_release_owner(interp); } static void @@ -1549,6 +1568,8 @@ new_threadstate(PyInterpreterState *interp, int whence) uint64_t id = interp->threads.next_unique_id; init_threadstate(tstate, interp, id, whence); + _Py_atomic_add_ssize(&interp->_owners, 1); + // Add the new thread state to the interpreter. PyThreadState *old_head = interp->threads.head; add_threadstate(interp, (PyThreadState *)tstate, old_head); From b58d4a165ad5467c551217b3ea1f9c7f6626fdab Mon Sep 17 00:00:00 2001 From: Shamil Abdulaev Date: Thu, 16 Oct 2025 20:06:40 +0300 Subject: [PATCH 02/10] add blurb --- .../2025-10-16-20-06-33.gh-issue-140138.a_fhrh.rst | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2025-10-16-20-06-33.gh-issue-140138.a_fhrh.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-10-16-20-06-33.gh-issue-140138.a_fhrh.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-10-16-20-06-33.gh-issue-140138.a_fhrh.rst new file mode 100644 index 00000000000000..1e88a6cbd62c1f --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-10-16-20-06-33.gh-issue-140138.a_fhrh.rst @@ -0,0 +1,7 @@ +Fix a TSAN-reported data race and potential use-after-free when shutting +down a subinterpreter with running daemon threads in free-threaded (NOGIL) +builds. The interpreter state could be freed while daemon threads still +accessed tstate->interp (e.g. during time.sleep). PyInterpreterState now +keeps an atomic owner refcount and is only finalized/freed when the last +PyThreadState is destroyed. This fixes +test_threading.test_daemon_threads_fatal_error. From 55b5fe4a0f04b90c31ee091bc0c0d1c5b84009c2 Mon Sep 17 00:00:00 2001 From: Shamil Abdulaev Date: Thu, 16 Oct 2025 21:41:46 +0300 Subject: [PATCH 03/10] Reorder and add `_owners` field Move the `_owners` field to be after `finalizing` and add it back to the struct. It was removed in a previous commit and is needed. --- Include/internal/pycore_interp_structs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_interp_structs.h b/Include/internal/pycore_interp_structs.h index 4a6456d865c577..3222ece1604a18 100644 --- a/Include/internal/pycore_interp_structs.h +++ b/Include/internal/pycore_interp_structs.h @@ -793,7 +793,6 @@ struct _is { int finalizing; uintptr_t last_restart_version; - Py_ssize_t _owners; struct pythreads { uint64_t next_unique_id; /* The linked list of threads, newest first. */ @@ -972,6 +971,7 @@ struct _is { # endif #endif + Py_ssize_t _owners; /* the initial PyInterpreterState.threads.head */ _PyThreadStateImpl _initial_thread; // _initial_thread should be the last field of PyInterpreterState. From 2aba8ba5d4ba798b47e192c62add15a8e69d646d Mon Sep 17 00:00:00 2001 From: Shamil Abdulaev Date: Thu, 16 Oct 2025 21:42:41 +0300 Subject: [PATCH 04/10] Fix typo in interpreter owner atomic variable Fixes a typo where the atomic variable `_owners` was incorrectly named and should have been `owners`. --- Python/pystate.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index a0ad26ab6cf7b8..ec811f627c2a3c 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -496,7 +496,7 @@ _finalize_and_free_interpreter(PyInterpreterState *interp) static inline void _interp_release_owner(PyInterpreterState *interp) { - Py_ssize_t prev = _Py_atomic_add_ssize(&interp->_owners, -1); + Py_ssize_t prev = _Py_atomic_add_ssize(&interp->owners, -1); if (prev == 1) { _finalize_and_free_interpreter(interp); } @@ -551,7 +551,7 @@ init_interpreter(PyInterpreterState *interp, interp->id = id; interp->id_refcount = 0; - interp->_owners = 1; + interp->owners = 1; assert(runtime->interpreters.head == interp); assert(next != NULL || (interp == runtime->interpreters.main)); @@ -1568,7 +1568,7 @@ new_threadstate(PyInterpreterState *interp, int whence) uint64_t id = interp->threads.next_unique_id; init_threadstate(tstate, interp, id, whence); - _Py_atomic_add_ssize(&interp->_owners, 1); + _Py_atomic_add_ssize(&interp->owners, 1); // Add the new thread state to the interpreter. PyThreadState *old_head = interp->threads.head; From 2690fac2eff6ee54fba9832c0d7dadcc885579cb Mon Sep 17 00:00:00 2001 From: Shamil Abdulaev Date: Thu, 16 Oct 2025 21:44:21 +0300 Subject: [PATCH 05/10] Fix news --- .../2025-10-16-20-06-33.gh-issue-140138.a_fhrh.rst | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-10-16-20-06-33.gh-issue-140138.a_fhrh.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-10-16-20-06-33.gh-issue-140138.a_fhrh.rst index 1e88a6cbd62c1f..42a7ceec06c412 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2025-10-16-20-06-33.gh-issue-140138.a_fhrh.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-10-16-20-06-33.gh-issue-140138.a_fhrh.rst @@ -1,7 +1 @@ -Fix a TSAN-reported data race and potential use-after-free when shutting -down a subinterpreter with running daemon threads in free-threaded (NOGIL) -builds. The interpreter state could be freed while daemon threads still -accessed tstate->interp (e.g. during time.sleep). PyInterpreterState now -keeps an atomic owner refcount and is only finalized/freed when the last -PyThreadState is destroyed. This fixes -test_threading.test_daemon_threads_fatal_error. +Fix a TSAN-reported data race and potential use-after-free when shutting down a subinterpreter with running daemon threads in free-threaded (NOGIL) builds. From e6ff33e158b67741bd196ec122aa2af0f5cb21fc Mon Sep 17 00:00:00 2001 From: Shamil Abdulaev Date: Thu, 16 Oct 2025 21:47:39 +0300 Subject: [PATCH 06/10] Rename interpreter cleanup functions Rename functions to be more descriptive of their actions. --- Python/pystate.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index ec811f627c2a3c..8132b97878857b 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -486,7 +486,7 @@ free_interpreter(PyInterpreterState *interp) } static void -_finalize_and_free_interpreter(PyInterpreterState *interp) +cleanup_and_free_interpreter(PyInterpreterState *interp) { _Py_qsbr_fini(interp); _PyObject_FiniState(interp); @@ -494,11 +494,11 @@ _finalize_and_free_interpreter(PyInterpreterState *interp) } static inline void -_interp_release_owner(PyInterpreterState *interp) +release_interp_owner(PyInterpreterState *interp) { Py_ssize_t prev = _Py_atomic_add_ssize(&interp->owners, -1); if (prev == 1) { - _finalize_and_free_interpreter(interp); + cleanup_and_free_interpreter(interp); } } @@ -975,7 +975,7 @@ PyInterpreterState_Delete(PyInterpreterState *interp) interp->finalizing = 1; - _interp_release_owner(interp); + release_interp_owner(interp); } @@ -1441,7 +1441,7 @@ free_threadstate(_PyThreadStateImpl *tstate) PyMem_RawFree(tstate); } - _interp_release_owner(interp); + release_interp_owner(interp); } static void From 47e51d7055c763bae58e87177acb5c1adecb14fd Mon Sep 17 00:00:00 2001 From: Shamil Abdulaev Date: Thu, 16 Oct 2025 21:57:51 +0300 Subject: [PATCH 07/10] Refactor interpreter cleanup Move the cleanup logic into PyInterpreterState_Delete and remove the now-unused helper function. --- Python/pystate.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index 8132b97878857b..d664c028b01752 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -485,20 +485,12 @@ free_interpreter(PyInterpreterState *interp) } } -static void -cleanup_and_free_interpreter(PyInterpreterState *interp) -{ - _Py_qsbr_fini(interp); - _PyObject_FiniState(interp); - free_interpreter(interp); -} - static inline void release_interp_owner(PyInterpreterState *interp) { Py_ssize_t prev = _Py_atomic_add_ssize(&interp->owners, -1); if (prev == 1) { - cleanup_and_free_interpreter(interp); + free_interpreter(interp); } } @@ -974,7 +966,8 @@ PyInterpreterState_Delete(PyInterpreterState *interp) HEAD_UNLOCK(runtime); interp->finalizing = 1; - + _Py_qsbr_fini(interp); + _PyObject_FiniState(interp); release_interp_owner(interp); } From 37ccb722a03bc2a3c201e28612e873c5a6ea4ee7 Mon Sep 17 00:00:00 2001 From: Shamil Abdulaev Date: Thu, 16 Oct 2025 22:01:06 +0300 Subject: [PATCH 08/10] Rename _owners to owners in interp struct The field name was inconsistent with other usages. --- Include/internal/pycore_interp_structs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_interp_structs.h b/Include/internal/pycore_interp_structs.h index 3222ece1604a18..3e4c8ecdba11a0 100644 --- a/Include/internal/pycore_interp_structs.h +++ b/Include/internal/pycore_interp_structs.h @@ -971,7 +971,7 @@ struct _is { # endif #endif - Py_ssize_t _owners; + Py_ssize_t owners; /* the initial PyInterpreterState.threads.head */ _PyThreadStateImpl _initial_thread; // _initial_thread should be the last field of PyInterpreterState. From e5c1336682da0cd8e98ed0ca6fb85ab7cdb541bb Mon Sep 17 00:00:00 2001 From: Shamil Abdulaev Date: Thu, 16 Oct 2025 22:11:11 +0300 Subject: [PATCH 09/10] fix --- Python/pystate.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Python/pystate.c b/Python/pystate.c index d664c028b01752..9b241c084e02a6 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1421,7 +1421,6 @@ 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 == &interp->_initial_thread) { From d08a0937476f866a05279fef27eebe1faeeee002 Mon Sep 17 00:00:00 2001 From: Shamil Abdulaev Date: Thu, 16 Oct 2025 22:15:43 +0300 Subject: [PATCH 10/10] Stop setting `interp->finalizing` --- Python/pystate.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Python/pystate.c b/Python/pystate.c index 9b241c084e02a6..11b21efb7ab40b 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -965,7 +965,6 @@ PyInterpreterState_Delete(PyInterpreterState *interp) } HEAD_UNLOCK(runtime); - interp->finalizing = 1; _Py_qsbr_fini(interp); _PyObject_FiniState(interp); release_interp_owner(interp);