race condition in threading when interpreter finalized while daemon thread runs (thread sanitizer identified) #124878
Comments
|
Quoting a comment from @mpage in a #105805 comment as this is somewhat tstate use after it has been freed related: """ A reference count on the tstate is interesting, presuming that is not something that'd need to change frequently? Mostly I see it as a "if the thread still exists, the PyThreadState should never be freed" marker - do we have a single clear marker of who owns deallocation of thread states? For daemon threads that we're abandoning during finalization we clearly should not be freeing their PyThreadState's today if that is what the above race actually shows as happening. |
|
This bug probably exists on older versions as well, I didn't try testing further back. In general: Friends don't let friends spawn daemon threads. For the health of the process and all code maintainers. |
The race condition with `free_threadstate` and daemon threads exists in both the free threading and default builds. We were missing a suppression in the default build.
|
I'm going to put up a PR soon that I think addresses the race conditions during interpreter finalization. The main ideas are: PyThreadState reference countThe PyThreadState field gains a reference count field to avoid the issues with Those are the only reference count operations. We don't need to ever increment the reference count of the PyThreadState and we don't bother with it during the common
|
The PyThreadState field gains a reference count field to avoid issues with PyThreadState being a dangling pointer to freed memory. The refcount starts with a value of two: one reference is owned by the interpreter's linked list of thread states and one reference is owned by the OS thread. The reference count is decremented when the thread state is removed from the interpreter's linked list and before the OS thread calls `PyThread_hang_thread()`. The thread that decrements it to zero frees the `PyThreadState` memory. The `holds_gil` field is moved out of the `_status` bit field, to avoid a data race where on thread calls `PyThreadState_Clear()`, modifying the `_status` bit field while the OS thread reads `holds_gil` when attempting to acquire the GIL. The `PyThreadState.state` field now has `_Py_THREAD_SHUTTING_DOWN` as a possible value. This corresponds to the `_PyThreadState_MustExit()` check. This avoids race conditions in the free threading build when checking `_PyThreadState_MustExit()`.
The PyThreadState field gains a reference count field to avoid issues with PyThreadState being a dangling pointer to freed memory. The refcount starts with a value of two: one reference is owned by the interpreter's linked list of thread states and one reference is owned by the OS thread. The reference count is decremented when the thread state is removed from the interpreter's linked list and before the OS thread calls `PyThread_hang_thread()`. The thread that decrements it to zero frees the `PyThreadState` memory. The `holds_gil` field is moved out of the `_status` bit field, to avoid a data race where on thread calls `PyThreadState_Clear()`, modifying the `_status` bit field while the OS thread reads `holds_gil` when attempting to acquire the GIL. The `PyThreadState.state` field now has `_Py_THREAD_SHUTTING_DOWN` as a possible value. This corresponds to the `_PyThreadState_MustExit()` check. This avoids race conditions in the free threading build when checking `_PyThreadState_MustExit()`.
The PyThreadState field gains a reference count field to avoid issues with PyThreadState being a dangling pointer to freed memory. The refcount starts with a value of two: one reference is owned by the interpreter's linked list of thread states and one reference is owned by the OS thread. The reference count is decremented when the thread state is removed from the interpreter's linked list and before the OS thread calls `PyThread_hang_thread()`. The thread that decrements it to zero frees the `PyThreadState` memory. The `holds_gil` field is moved out of the `_status` bit field, to avoid a data race where on thread calls `PyThreadState_Clear()`, modifying the `_status` bit field while the OS thread reads `holds_gil` when attempting to acquire the GIL. The `PyThreadState.state` field now has `_Py_THREAD_SHUTTING_DOWN` as a possible value. This corresponds to the `_PyThreadState_MustExit()` check. This avoids race conditions in the free threading build when checking `_PyThreadState_MustExit()`.
The PyThreadState field gains a reference count field to avoid issues with PyThreadState being a dangling pointer to freed memory. The refcount starts with a value of two: one reference is owned by the interpreter's linked list of thread states and one reference is owned by the OS thread. The reference count is decremented when the thread state is removed from the interpreter's linked list and before the OS thread calls `PyThread_hang_thread()`. The thread that decrements it to zero frees the `PyThreadState` memory. The `holds_gil` field is moved out of the `_status` bit field, to avoid a data race where on thread calls `PyThreadState_Clear()`, modifying the `_status` bit field while the OS thread reads `holds_gil` when attempting to acquire the GIL. The `PyThreadState.state` field now has `_Py_THREAD_SHUTTING_DOWN` as a possible value. This corresponds to the `_PyThreadState_MustExit()` check. This avoids race conditions in the free threading build when checking `_PyThreadState_MustExit()`.
…0602) The race condition with `free_threadstate` and daemon threads exists in both the free threading and default builds. We were missing a suppression in the default build.
…dstate (pythongh-130602) The race condition with `free_threadstate` and daemon threads exists in both the free threading and default builds. We were missing a suppression in the default build. (cherry picked from commit cc17307) Co-authored-by: Sam Gross <colesbury@gmail.com>
…gh-130602) (gh-130687) The race condition with `free_threadstate` and daemon threads exists in both the free threading and default builds. We were missing a suppression in the default build. (cherry picked from commit cc17307)
The PyThreadState field gains a reference count field to avoid issues with PyThreadState being a dangling pointer to freed memory. The refcount starts with a value of two: one reference is owned by the interpreter's linked list of thread states and one reference is owned by the OS thread. The reference count is decremented when the thread state is removed from the interpreter's linked list and before the OS thread calls `PyThread_hang_thread()`. The thread that decrements it to zero frees the `PyThreadState` memory. The `holds_gil` field is moved out of the `_status` bit field, to avoid a data race where on thread calls `PyThreadState_Clear()`, modifying the `_status` bit field while the OS thread reads `holds_gil` when attempting to acquire the GIL. The `PyThreadState.state` field now has `_Py_THREAD_SHUTTING_DOWN` as a possible value. This corresponds to the `_PyThreadState_MustExit()` check. This avoids race conditions in the free threading build when checking `_PyThreadState_MustExit()`.
The PyThreadState field gains a reference count field to avoid issues with PyThreadState being a dangling pointer to freed memory. The refcount starts with a value of two: one reference is owned by the interpreter's linked list of thread states and one reference is owned by the OS thread. The reference count is decremented when the thread state is removed from the interpreter's linked list and before the OS thread calls `PyThread_hang_thread()`. The thread that decrements it to zero frees the `PyThreadState` memory. The `holds_gil` field is moved out of the `_status` bit field, to avoid a data race where on thread calls `PyThreadState_Clear()`, modifying the `_status` bit field while the OS thread reads `holds_gil` when attempting to acquire the GIL. The `PyThreadState.state` field now has `_Py_THREAD_SHUTTING_DOWN` as a possible value. This corresponds to the `_PyThreadState_MustExit()` check. This avoids race conditions in the free threading build when checking `_PyThreadState_MustExit()`.
Bug report
Bug description:
Using the code in #105805 with the newly added
test.test_threading.ThreadTests.test_finalize_daemon_thread_hangtest enabled you can reproduce this thread sanitizer crash as follows (I used clang 18):This also happens if I just take the new test and corresponding Modules/_testcapimodule.c change and patch it on top of
main- it's a pre-existing bug not related to my PR adding the new test. (Filing now before I check this in decorated to be skipped under sanitizers so I can reference the issue number in a comment)Examining the code in question where the race occurs... it's this block https://github.com/python/cpython/blob/v3.13.0rc3/Python/ceval_gil.c#L258
looping in @ericsnowcurrently for #104341 context.
The
int final_releasevalue in that call stack is 0 so the next bit tries to load the eval breaker bit but the thread was woken up by python code executing during finalization of the main thread per the test.How'd thread T1 ever obtain the GIL upon waking up in the first place given finalization had started?
CPython versions tested on:
CPython main branch
Operating systems tested on:
Linux
Linked PRs
The text was updated successfully, but these errors were encountered: