gh-131185: Use a proper thread-local for cached thread states #132510
Conversation
Misc/NEWS.d/next/C_API/2025-04-14-07-41-28.gh-issue-131185.ZCjMHD.rst
Outdated
Show resolved
Hide resolved
Python/pystate.c
Outdated
| } | ||
| #endif | ||
| //--------------------------------------------- | ||
| // the GIL state bound to the current OS thread |
There was a problem hiding this comment.
The previous comment here made more sense to me.
There was a problem hiding this comment.
Yeah, but it wasn't really correct if it was referring to the gilstate. I went with your suggestion below and referred to it as a "thread state used by PyGILState_Ensure()," is that better?
…MHD.rst Co-authored-by: Sam Gross <colesbury@gmail.com>
…ity/cpython into gilstate-thread-local
| if (!_PyEval_ThreadsInitialized() || runtime->gilstate.autoInterpreterState == NULL) { | ||
| PyThread_hang_thread(); | ||
| } |
There was a problem hiding this comment.
This doesn't look right to me. There's no synchronization here with the thread finalizing the process. Additionally, it has time-of-check to time-of-use issues because runtime->gilstate.autoInterpreterState is reloaded below.
There was a problem hiding this comment.
Yeah, I'm planning on fixing this in a follow-up. For now, I'm ignoring cases where threads concurrently call PyGILState_Ensure while the interpreter is finalizing, because there's more to fix than just the gilstate pointer.
For example, zapthreads in PyInterpreterState_Delete performs UAF when there are non-main threads, because it accesses the next of the thread state it just freed. I think there are probably more cases like this. Anyways, for this PR, I'd like to focus on calling PyGILState_Ensure after the interpreter is long dead. Is that alright?
There was a problem hiding this comment.
Ok, that's fine. Can you leave a comment here about the limitations?
There was a problem hiding this comment.
LGTM
@ericsnowcurrently - would you like to review this as well?
|
Bump @ericsnowcurrently @colesbury, I'd like to get all the finalization races fixed before the beta freeze, and we're running out of time. We need this one as a first step. |
|
I haven't had time to follow this, unfortunately. I'll take a look as soon as I can. In the meantime, what's the motivation for the change? |
|
It fixes a crash with |
|
I do think we should first make a change that does the how is-finalizing dance (likely with a lock). The switch to a thread-local, while nice, should be orthogonal to actually fixing the bug. (We probably don't need to backport the thread-local change.) |
I don't think we should backport this change, this code is old so this isn't a new bug. I believe having different implementation for this across branches isn't worth it, it can introduce new bugs. How about keeping this change for 3.15 only? |
|
@ericsnowcurrently Do you have any opposition to landing this on main? #134307 will be used for 3.14. I'd like to get this one merged so I don't have to deal with conflicts. |
|
Nope. Thanks for taking care of this. I'll merge it. |
…ythongh-132510) Switches over to a _Py_thread_local in place of autoTssKey, and also fixes a few other checks regarding PyGILState_Ensure after finalization. Note that this doesn't fix concurrent use of PyGILState_Ensure with Py_Finalize; I'm pretty sure zapthreads doesn't work at all, and that needs to be fixed seperately.
Switches over to a
_Py_thread_localin place ofautoTssKey, and also fixes a few other checks regardingPyGILState_Ensureafter finalization.Note that this doesn't fix concurrent use of
PyGILState_EnsurewithPy_Finalize; I'm pretty surezapthreadsdoesn't work at all, and that needs to be fixed seperately.