gh-104690: thread_run() checks for tstate dangling pointer

[vstinner]

thread_run() of _threadmodule.c now calls
_PyThreadState_CheckConsistency() to check if tstate is a dangling pointer.

Rename ceval_gil.c is_tstate_valid() to
_PyThreadState_CheckConsistency() to reuse it in _threadmodule.c.

• Issue: Threads started in exit handler are still running after their thread states are destroyed #104690

[vstinner]

This change provides a better error message rather than a blind crash. It should help debugging complicated crashes.

Example:

$ ./python bug.py python: Python/pystate.c:2904: _PyThreadState_CheckConsistency: Assertion `!_PyMem_IsPtrFreed(tstate->interp)' failed.
Abandon (core dumped)
$ gdb -args ./python bug.py (gdb) run
Starting program: /home/vstinner/python/main/python bug.py
[New Thread 0x7fffea2ff6c0 (LWP 1148671)]
[New Thread 0x7fffe9afe6c0 (LWP 1148672)]
python: Python/pystate.c:2904: _PyThreadState_CheckConsistency: Assertion `!_PyMem_IsPtrFreed(tstate->interp)' failed.
Thread 3 "python" received signal SIGABRT, Aborted.
[Switching to Thread 0x7fffe9afe6c0 (LWP 1148672)]
0x00007ffff7d7a844 in __pthread_kill_implementation () from /lib64/libc.so.6
(gdb) where
#0 0x00007ffff7d7a844 in __pthread_kill_implementation () from /lib64/libc.so.6
#1 0x00007ffff7d29abe in raise () from /lib64/libc.so.6
#2 0x00007ffff7d1287f in abort () from /lib64/libc.so.6
#3 0x00007ffff7d1279b in __assert_fail_base.cold () from /lib64/libc.so.6
#4 0x00007ffff7d22147 in __assert_fail () from /lib64/libc.so.6
#5 0x00000000006e204a in _PyThreadState_CheckConsistency (tstate=0x7fffe4000e50) at Python/pystate.c:2904
#6 0x000000000078f006 in thread_run (boot_raw=0x7fffea4be1b0) at ./Modules/_threadmodule.c:1081
#7 0x00000000006ff043 in pythread_wrapper (arg=0xb86580) at Python/thread_pthread.h:233
#8 0x00007ffff7d78907 in start_thread () from /lib64/libc.so.6
#9 0x00007ffff7dfe870 in clone3 () from /lib64/libc.so.6

Patch to reintroduce the bug:

diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c
index 49f34fcb9f..f5fc209e88 100644
--- a/Modules/_threadmodule.c+++ b/Modules/_threadmodule.c@@ -1158,11 +1161,13 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs)
"thread is not supported for isolated subinterpreters");
return NULL;
}
+#if 0
if (interp->finalizing) {
PyErr_SetString(PyExc_RuntimeError,
"can't create new thread at interpreter shutdown");
return NULL;
}
+#endif
struct bootstate *boot = PyMem_NEW(struct bootstate, 1);
if (boot == NULL) {

Script bug.py:

importatexitimportthreadingdeft0():
passdeft1():
threading.Thread(target=t0).start()
deff():
threading.Thread(target=t1).start()
atexit.register(f)
exit()

[vstinner]

I don't think that it's related, but in a recent job on Windows x86, the following assertion failed while running test_threading() in a test worker process:

Assertion failed: tstate_is_alive(tstate) && !tstate->_status.bound, file D:\\a\\cpython\\cpython\\Python\\pystate.c, line 245

Related code:

staticvoidbind_tstate(PyThreadState*tstate)
{
assert(tstate!=NULL);
assert(tstate_is_alive(tstate) && !tstate->_status.bound); /// <=== HERE, line 245assert(!tstate->_status.unbound); // just in caseassert(!tstate->_status.bound_gilstate);
assert(tstate!=gilstate_tss_get(tstate->interp->runtime));
assert(!tstate->_status.active);
assert(tstate->thread_id==0);
assert(tstate->native_thread_id==0);

I don't know if it would be worth it to repeat the _PyThreadState_CheckConsistency() check in bind_tstate(). I don't think so, since my change already calls it in thread_run().

See: #108987

cc @serhiy-storchaka

[vstinner]

Oh! I managed to reproduce issue #108987 on Windows 32-bit with my PR and I get:

Assertion failed: !_PyMem_IsPtrFreed(tstate->interp), file C:\victor\python\main\Python\pystate.c, line 2911
Fatal Python error: Aborted
Thread 0x00000ef0 (most recent call first):
<no Python frame>

So apparently, thread_run() is called after PyInterpreterState_Delete() has been called!

[miss-islington]

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

[miss-islington]

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[miss-islington]

Sorry, @vstinner, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f63d37877ad166041489a968233b57540f8456e8 3.11

[miss-islington]

Sorry, @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f63d37877ad166041489a968233b57540f8456e8 3.12

[bedevere-bot]

GH-109133 is a backport of this pull request to the 3.12 branch.

[bedevere-bot]

GH-109134 is a backport of this pull request to the 3.11 branch.