gh-134144: Fix use-after-free in zapthreads()

[b-pass]

Fixes #134144. This problem was introduced by #127077 which added _Py_FOR_EACH_TSTATE_UNLOCKED.

_Py_FOR_EACH_TSTATE_UNLOCKED reads the tstate at the end of each loop iteration to get the next value. When the loop body frees the tstate, the result is always a use-after-free. It doesn't always crash (seems rare, actually), I guess it just depends what the allocator does with those pages during the rest of the tstate deletion.

• Issue: Cannot safely Py_EndInterpreter in 3.14b1 #134144

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[ZeroIntensity]

I'm pretty sure we can drop zapthreads entirely, because right now the assumption is that the interpreter doesn't have any threads left. Let's hold off on this until we come to a decision on what the actual behavior should be for dangling threads.

[ericsnowcurrently]

LGTM

We just need a NEWS entry.

@ZeroIntensity, regarding gh-128640, I expect we'll do that too since it solves an additional problem. However, this PR is a good idea regardless.

[ZeroIntensity]

We need a test as well, but I'm more worried about the use-case as given in the issue. zapthreads will arbitrarily destroy a thread state (one created by PyGILState_Ensure in the repro), which is a recipe for crashes if the thread is still running.

[b-pass]

We need a test as well, but I'm more worried about the use-case as given in the issue. zapthreads will arbitrarily destroy a thread state (one created by PyGILState_Ensure in the repro), which is a recipe for crashes if the thread is still running.

Sorry, the issue description isn't great. The thread wasn't alive, but Py_EndInterpreter takes a PyThreadState* as the argument (not a PyInterpreterState*), so there must always been one on the interpreter. But the fact is this code ALWAYS does a use-after-free (even if there was only ever one thread state). Here is some pseudo code of what is going on:

for (PyThreadState *tstate = interp->threads.head; 
       tstate != NULL; 
       tstate = tstate->next)
{
   free(tstate);
}

[ZeroIntensity]

Right, I've noticed this before. In most cases, we get lucky because the initial thread is statically allocated on the interpreter state, so it doesn't actually cause UAF. It becomes an issue with something like PyGILState_Ensure because we have more than one thread state, but the problem is that's unsupported anyway.

[b-pass]

It becomes an issue with something like PyGILState_Ensure because we have more than one thread state, but the problem is that's unsupported anyway.

It happens without the Ensure. I've updated the linked issue with a complete program (without an Ensure). So are you saying it's not supported to use a different PyThreadState* to end the interpreter than the one returned from its creation?

[ZeroIntensity]

So are you saying it's not supported to use a different PyThreadState* to end the interpreter than the one returned from its creation?

Yes, and you can't have any additional threads when you call Py_EndInterpreter.

I'm fine with this fix, but my point is that it's not enough. Let's add a test and then I'll approve.

[ericsnowcurrently]

Yeah, a test would be nice.