Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CPython should not assume that pthread_self returns the same value in fork parent and child #126688

Closed
nico opened this issue Nov 11, 2024 · 2 comments
Closed

CPython should not assume that pthread_self returns the same value in fork parent and child #126688

nico opened this issue Nov 11, 2024 · 2 comments
Assignees
@colesbury
Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes type-bug An unexpected behavior, bug, or error

Comments

@nico
Copy link

nico commented Nov 11, 2024 •
edited by bedevere-app bot
Loading

Summarized from SerenityOS/serenity#25263 by request of @colesbury:

It seems that at least Python 3.13 relies on pthread_self returning the same value in both the parent and the child process.
PyThread_get_thread_ident_ex

POSIX doesn't seem to mention if the value returned by pthread_self should be inherited by the child process.
pthread_self
fork

(It is the case on Linux, Hurd, FreeBSD, OpenBSD, Cygwin. It isn't on SerenityOS, and also not on Solaris 9/HP-UX 11 per https://bugs.python.org/issue7242.

It looks like the code add broke things is from this commit e21057b added in #118523 as part of #117657

Linked PRs
@nico nico mentioned this issue Nov 11, 2024
Closed
@colesbury colesbury self-assigned this Nov 11, 2024
@colesbury colesbury added type-bug An unexpected behavior, bug, or error 3.13 bugs and security fixes 3.14 new features, bugs and security fixes labels Nov 11, 2024
colesbury added a commit to colesbury/cpython that referenced this issue Nov 11, 2024
@colesbury
pythongh-126688: Reinit import lock after fork
fc4bdfb 
The PyMutex implementation supports unlocking after fork because we
clear the list of watiers in parking_lot.c. This doesn't work as well
for _PyRecursiveMutex because on some systems, such as SerenityOS, the
thread id is not preserved across fork().
colesbury added a commit to colesbury/cpython that referenced this issue Nov 11, 2024
@colesbury
pythongh-126688: Reinit import lock after fork
5e86cca 
The PyMutex implementation supports unlocking after fork because we
clear the list of watiers in parking_lot.c. This doesn't work as well
for _PyRecursiveMutex because on some systems, such as SerenityOS, the
thread id is not preserved across fork().
@bedevere-app bedevere-app bot mentioned this issue Nov 11, 2024
Merged
colesbury added a commit to colesbury/cpython that referenced this issue Nov 11, 2024
@colesbury
Merge branch 'main' into pythongh-126688-import-lock
4bf74b8
colesbury added a commit that referenced this issue Nov 12, 2024
@colesbury
gh-126688: Reinit import lock after fork (#126692)
5610860 
The PyMutex implementation supports unlocking after fork because we
clear the list of waiters in parking_lot.c. This doesn't work as well
for _PyRecursiveMutex because on some systems, such as SerenityOS, the
thread id is not preserved across fork().
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 12, 2024
@colesbury @miss-islington
pythongh-126688: Reinit import lock after fork (pythonGH-126692)
6f77b81 
The PyMutex implementation supports unlocking after fork because we
clear the list of waiters in parking_lot.c. This doesn't work as well
for _PyRecursiveMutex because on some systems, such as SerenityOS, the
thread id is not preserved across fork().
(cherry picked from commit 5610860)

Co-authored-by: Sam Gross <[email protected]>
@bedevere-app bedevere-app bot mentioned this issue Nov 12, 2024
Merged
colesbury added a commit that referenced this issue Nov 12, 2024
@miss-islington @colesbury
[3.13] gh-126688: Reinit import lock after fork (GH-126692) (GH-126765)
13fe8f9 
The PyMutex implementation supports unlocking after fork because we
clear the list of waiters in parking_lot.c. This doesn't work as well
for _PyRecursiveMutex because on some systems, such as SerenityOS, the
thread id is not preserved across fork().
(cherry picked from commit 5610860)

Co-authored-by: Sam Gross <[email protected]>
@oskar-skog
Copy link

I think this can be closed now.

@ericsnowcurrently
Copy link
Member

There may be other code that makes the same assumption about the thread ID. For example, in PyOS_AfterFork_Child():

cpython/Modules/posixmodule.c

Lines 655 to 658 in 09c240f

assert(tstate->thread_id == PyThread_get_thread_ident());
#ifdef PY_HAVE_THREAD_NATIVE_ID
tstate->native_thread_id = PyThread_get_thread_native_id();
#endif

This would break only on debug builds (or at least !defined(NDEBUG)).

I'm guessing there are other places too.

@bedevere-app bedevere-app bot mentioned this issue Nov 21, 2024
Draft
@github-actions github-actions bot mentioned this issue Dec 1, 2024
Closed
picnixz pushed a commit to picnixz/cpython that referenced this issue Dec 8, 2024
@colesbury @picnixz
pythongh-126688: Reinit import lock after fork (python#126692)
45320c2 
The PyMutex implementation supports unlocking after fork because we
clear the list of waiters in parking_lot.c. This doesn't work as well
for _PyRecursiveMutex because on some systems, such as SerenityOS, the
thread id is not preserved across fork().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@colesbury colesbury

Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nico @colesbury @ericsnowcurrently @oskar-skog