Skip to content

Deletion of autoTSSkey during runtime finalization is not safe #131185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
colesbury opened this issue Mar 13, 2025 · 5 comments
Closed

Deletion of autoTSSkey during runtime finalization is not safe #131185

colesbury opened this issue Mar 13, 2025 · 5 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@colesbury
Copy link
Contributor

colesbury commented Mar 13, 2025

Bug report

The autoTssKey is deleted during _PyRuntimeState_Fini by gilstate_tss_fini. This isn't safe because other threads may try calling PyGILState_Ensure() or PyGILState_GetThisThreadState() concurrently during shutdown.

cpython/Python/pystate.c

Lines 486 to 501 in e9d210b

void
_PyRuntimeState_Fini(_PyRuntimeState *runtime)
{
#ifdef Py_REF_DEBUG
/* The count is cleared by _Py_FinalizeRefTotal(). */
assert(runtime->object_state.interpreter_leaks == 0);
#endif
if (gilstate_tss_initialized(runtime)) {
gilstate_tss_fini(runtime);
}
if (PyThread_tss_is_created(&runtime->trashTSSkey)) {
PyThread_tss_delete(&runtime->trashTSSkey);
}
}

We can:

  1. Convert autoTssKey to a _Py_thread_local like _Py_tss_tstate, which doesn't require deletion
  2. Don't delete autoTssKey at runtime finalization

My preference is for the first option.

cc @ZeroIntensity @ericsnowcurrently @gpshead

Linked PRs

@colesbury colesbury added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Mar 13, 2025
@ZeroIntensity
Copy link
Member

Making it a thread-local sounds reasonable, but I think we also need to deal with NULL auto interpreter states in order to get PyGILState_Ensure fully working (as in, hanging the thread) post-finalization.

My other concern (which might be an issue regardless of autoTSSkey) is what happens to the thread state when the thread is hung. Does it just get leaked?

@colesbury
Copy link
Contributor Author

...what happens to the thread state when the thread is hung. Does it just get leaked?

No, the thread state gets deleted:

decref_threadstate(tstate_impl);

But I'm not really sure it matters because:

  1. The process is about to exit, so anything you "leak" is going to disappear soon
  2. The native thread will still be leaked
  3. Any Python objects referenced by the thread's stack will be leaked. This is true regardless of whether we kill the thread via pthread_exit() or block it via PyThread_hang_thread().

@ZeroIntensity
Copy link
Member

The process is about to exit, so anything you "leak" is going to disappear soon

I'm thinking about embedders that don't necessarily exit after Py_Finalize, but yeah, I don't think it matters much either. I would assume that most embedders keep the runtime alive for the duration of their app as well.

ericsnowcurrently pushed a commit that referenced this issue May 21, 2025
)

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.
@ericsnowcurrently
Copy link
Member

The broader issue of unprotected runtime state at finalization is handled in gh-134307.

@ericsnowcurrently
Copy link
Member

Thanks for tackling this, @ZeroIntensity!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants