GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-104690: thread_run() checks for tstate dangling pointer by vstinner · Pull Request #109056 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Sep 7, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
pythongh-104690: thread_run() checks for tstate dangling pointer
103970e
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.
@vstinner vstinner added the skip news label Sep 7, 2023
@bedevere-bot bedevere-bot added the awaiting core review label Sep 7, 2023
@bedevere-bot bedevere-bot mentioned this pull request Sep 7, 2023
Threads started in exit handler are still running after their thread states are destroyed #104690
Closed
@vstinner

vstinner commented Sep 7, 2023

Copy link
Copy Markdown
Member Author

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()
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Sep 7, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Sep 7, 2023

Copy link
Copy Markdown
Member Author

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner merged commit f63d378 into python:main Sep 8, 2023
@vstinner
vstinner deleted the tstate_consistency branch September 8, 2023 09:50
@bedevere-bot bedevere-bot removed the awaiting core review label Sep 8, 2023
@vstinner vstinner added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Sep 8, 2023
@miss-islington

miss-islington commented Sep 8, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington

miss-islington commented Sep 8, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington

miss-islington commented Sep 8, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington miss-islington assigned vstinner Sep 8, 2023
@miss-islington

miss-islington commented Sep 8, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner added a commit to vstinner/cpython that referenced this pull request Sep 8, 2023
@vstinner
pythongh-104690: thread_run() checks for tstate dangling pointer (pyt…
97dd3c2
…hon#109056)

thread_run() of _threadmodule.c now calls
_PyThreadState_CheckConsistency() to check if tstate is a dangling
pointer when Python is built in debug mode.

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

(cherry picked from commit f63d378)
@bedevere-bot

bedevere-bot commented Sep 8, 2023

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Sep 8, 2023
@bedevere-bot

bedevere-bot commented Sep 8, 2023

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 8, 2023
vstinner added a commit to vstinner/cpython that referenced this pull request Sep 8, 2023
@vstinner
pythongh-104690: thread_run() checks for tstate dangling pointer (pyt…
cd96310
…hon#109056)

thread_run() of _threadmodule.c now calls
_PyThreadState_CheckConsistency() to check if tstate is a dangling
pointer when Python is built in debug mode.

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

(cherry picked from commit f63d378)
vstinner added a commit that referenced this pull request Sep 8, 2023
@vstinner
[3.11] gh-104690: thread_run() checks for tstate dangling pointer (#1…
b55cf2c
…09056) (#109134)

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

thread_run() of _threadmodule.c now calls
_PyThreadState_CheckConsistency() to check if tstate is a dangling
pointer when Python is built in debug mode.

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

(cherry picked from commit f63d378)
Yhg1s pushed a commit that referenced this pull request Oct 2, 2023
@vstinner
[3.12] gh-104690: thread_run() checks for tstate dangling pointer (#1…
30748d3
…09056) (#109133)

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

thread_run() of _threadmodule.c now calls
_PyThreadState_CheckConsistency() to check if tstate is a dangling
pointer when Python is built in debug mode.

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

(cherry picked from commit f63d378)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@vstinner vstinner

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@vstinner @miss-islington @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.