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 |
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.