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:

import atexit
import threading

def t0():
    pass

def t1():
    threading.Thread(target=t0).start()

def f():
    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:

static void
bind_tstate(PyThreadState *tstate)
{
    assert(tstate != NULL);
    assert(tstate_is_alive(tstate) && !tstate->_status.bound); /// <=== HERE, line 245
    assert(!tstate->_status.unbound);  // just in case
    assert(!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.