From 20a9bd64d0d6c5221f93e54e4dc51d4e1e77bfdb Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 21 Nov 2024 13:38:15 -0700 Subject: [PATCH 1/6] Hold a global lock while forking. --- Include/internal/pycore_runtime.h | 6 ++++++ Modules/posixmodule.c | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 2f2cec22cf1589..aad71465adcadb 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -164,6 +164,12 @@ typedef struct pyruntimestate { _Py_AuditHookEntry *head; } audit_hooks; +#ifdef HAVE_FORK + struct { + PyMutex mutex; + } os_fork; +#endif + struct _py_object_runtime_state object_state; struct _Py_float_runtime_state float_state; struct _Py_unicode_runtime_state unicode_state; diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 6eb7054b566e3f..a66badf9610025 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -619,8 +619,11 @@ void PyOS_BeforeFork(void) { PyInterpreterState *interp = _PyInterpreterState_GET(); + assert(interp->runtime == &_PyRuntime); run_at_forkers(interp->before_forkers, 1); + PyMutex_Lock(&_PyRuntime.os_fork.mutex); + _PyImport_AcquireLock(interp); _PyEval_StopTheWorldAll(&_PyRuntime); HEAD_LOCK(&_PyRuntime); @@ -633,7 +636,11 @@ PyOS_AfterFork_Parent(void) _PyEval_StartTheWorldAll(&_PyRuntime); PyInterpreterState *interp = _PyInterpreterState_GET(); + assert(interp->runtime == &_PyRuntime); _PyImport_ReleaseLock(interp); + + PyMutex_Unlock(&_PyRuntime.os_fork.mutex); + run_at_forkers(interp->after_forkers_parent, 0); } @@ -651,6 +658,7 @@ PyOS_AfterFork_Child(void) PyThreadState *tstate = _PyThreadState_GET(); _Py_EnsureTstateNotNULL(tstate); + assert(tstate->interp->runtime == &_PyRuntime); assert(tstate->thread_id == PyThread_get_thread_ident()); #ifdef PY_HAVE_THREAD_NATIVE_ID @@ -694,6 +702,8 @@ PyOS_AfterFork_Child(void) goto fatal_error; } + PyMutex_Unlock(&_PyRuntime.os_fork.mutex); + run_at_forkers(tstate->interp->after_forkers_child, 0); return; From 259e3c672acc97a7a023f9123bc721dd5ea7879c Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 21 Nov 2024 13:40:04 -0700 Subject: [PATCH 2/6] Keep track of the parent thread ID. --- Include/internal/pycore_runtime.h | 3 +++ Modules/posixmodule.c | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index aad71465adcadb..30937d29f72e1b 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -167,6 +167,9 @@ typedef struct pyruntimestate { #ifdef HAVE_FORK struct { PyMutex mutex; + struct _os_fork_parent { + PyThread_ident_t tid; + } parent; } os_fork; #endif diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index a66badf9610025..18dc7305bce1db 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -623,6 +623,7 @@ PyOS_BeforeFork(void) run_at_forkers(interp->before_forkers, 1); PyMutex_Lock(&_PyRuntime.os_fork.mutex); + _PyRuntime.os_fork.parent.tid = PyThread_get_thread_ident_ex(); _PyImport_AcquireLock(interp); _PyEval_StopTheWorldAll(&_PyRuntime); @@ -632,6 +633,8 @@ PyOS_BeforeFork(void) void PyOS_AfterFork_Parent(void) { + _PyRuntime.os_fork.parent = (struct _os_fork_parent){0}; + HEAD_UNLOCK(&_PyRuntime); _PyEval_StartTheWorldAll(&_PyRuntime); @@ -660,6 +663,10 @@ PyOS_AfterFork_Child(void) _Py_EnsureTstateNotNULL(tstate); assert(tstate->interp->runtime == &_PyRuntime); + // XXX We are assuming that the parent and child always have + // the same thread ID. That isn't necessarily true. + // See https://github.com/python/cpython/issues/126688. + assert(_PyRuntime.os_fork.parent.tid == PyThread_get_thread_ident_ex()); assert(tstate->thread_id == PyThread_get_thread_ident()); #ifdef PY_HAVE_THREAD_NATIVE_ID tstate->native_thread_id = PyThread_get_thread_native_id(); @@ -702,6 +709,7 @@ PyOS_AfterFork_Child(void) goto fatal_error; } + _PyRuntime.os_fork.parent = (struct _os_fork_parent){0}; PyMutex_Unlock(&_PyRuntime.os_fork.mutex); run_at_forkers(tstate->interp->after_forkers_child, 0); From 8fbd4fc668bca6e1a92e698b8460d34ecb9e40d7 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 21 Nov 2024 13:55:47 -0700 Subject: [PATCH 3/6] Update the tstate thread ID if necessary. --- Modules/posixmodule.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 18dc7305bce1db..f0653594ebea83 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -663,11 +663,15 @@ PyOS_AfterFork_Child(void) _Py_EnsureTstateNotNULL(tstate); assert(tstate->interp->runtime == &_PyRuntime); - // XXX We are assuming that the parent and child always have - // the same thread ID. That isn't necessarily true. + // We cannot assume that the parent and child always have + // the same thread ID. // See https://github.com/python/cpython/issues/126688. - assert(_PyRuntime.os_fork.parent.tid == PyThread_get_thread_ident_ex()); - assert(tstate->thread_id == PyThread_get_thread_ident()); + if (_PyRuntime.os_fork.parent.tid != PyThread_get_thread_ident_ex()) { + tstate->thread_id = PyThread_get_thread_ident(); + } + else { + assert(tstate->thread_id == PyThread_get_thread_ident()); + } #ifdef PY_HAVE_THREAD_NATIVE_ID tstate->native_thread_id = PyThread_get_thread_native_id(); #endif From db5271aca354960073e95acef02ba5806e40563f Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 21 Nov 2024 13:42:07 -0700 Subject: [PATCH 4/6] Add _PyRecursiveMutex_at_fork_reinit(). --- Include/internal/pycore_lock.h | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/Include/internal/pycore_lock.h b/Include/internal/pycore_lock.h index 57cbce8f126aca..ca1579fe62b8d4 100644 --- a/Include/internal/pycore_lock.h +++ b/Include/internal/pycore_lock.h @@ -151,10 +151,12 @@ _PyOnceFlag_CallOnce(_PyOnceFlag *flag, _Py_once_fn_t *fn, void *arg) return _PyOnceFlag_CallOnceSlow(flag, fn, arg); } +#define _PyThread_ident_t unsigned long long + // A recursive mutex. The mutex should zero-initialized. typedef struct { PyMutex mutex; - unsigned long long thread; // i.e., PyThread_get_thread_ident_ex() + _PyThread_ident_t thread; // i.e., PyThread_get_thread_ident_ex() size_t level; } _PyRecursiveMutex; @@ -164,6 +166,20 @@ extern PyLockStatus _PyRecursiveMutex_LockTimed(_PyRecursiveMutex *m, PyTime_t t PyAPI_FUNC(void) _PyRecursiveMutex_Unlock(_PyRecursiveMutex *m); extern int _PyRecursiveMutex_TryUnlock(_PyRecursiveMutex *m); +PyAPI_FUNC(_PyThread_ident_t) PyThread_get_thread_ident_ex(void); + +static inline void +_PyRecursiveMutex_at_fork_reinit(_PyRecursiveMutex *m, + _PyThread_ident_t parent) +{ + if (m->thread == parent) { + m->thread = PyThread_get_thread_ident_ex(); + } + else { + memset(m, 0, sizeof(*m)); + } +} + // A readers-writer (RW) lock. The lock supports multiple concurrent readers or // a single writer. The lock is write-preferring: if a writer is waiting while // the lock is read-locked then, new readers will be blocked. This avoids From 9e015f8dd425a7ebf0ffc4736359ac3ff16762aa Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 21 Nov 2024 14:10:06 -0700 Subject: [PATCH 5/6] Use _PyRecursiveMutex_at_fork_reinit() in _PyImport_ReInitLock(). --- Include/internal/pycore_import.h | 7 ++++++- Modules/posixmodule.c | 2 +- Python/import.c | 9 ++++++--- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/Include/internal/pycore_import.h b/Include/internal/pycore_import.h index 318c712bdfa174..320ab025f797a2 100644 --- a/Include/internal/pycore_import.h +++ b/Include/internal/pycore_import.h @@ -9,6 +9,7 @@ extern "C" { # error "this header requires Py_BUILD_CORE define" #endif +#include "pycore_pythread.h" // PyThread_ident_t #include "pycore_lock.h" // PyMutex #include "pycore_hashtable.h" // _Py_hashtable_t @@ -21,7 +22,11 @@ extern int _PyImport_SetModuleString(const char *name, PyObject* module); extern void _PyImport_AcquireLock(PyInterpreterState *interp); extern void _PyImport_ReleaseLock(PyInterpreterState *interp); -extern void _PyImport_ReInitLock(PyInterpreterState *interp); +#ifdef HAVE_FORK +extern void _PyImport_ReInitLock( + PyInterpreterState *interp, + PyThread_ident_t parent); +#endif // This is used exclusively for the sys and builtins modules: extern int _PyImport_FixupBuiltin( diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index f0653594ebea83..51e3aac5e6e766 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -697,7 +697,7 @@ PyOS_AfterFork_Child(void) _PyEval_StartTheWorldAll(&_PyRuntime); _PyThreadState_DeleteList(list); - _PyImport_ReInitLock(tstate->interp); + _PyImport_ReInitLock(tstate->interp, _PyRuntime.os_fork.parent.tid); _PyImport_ReleaseLock(tstate->interp); _PySignal_AfterFork(); diff --git a/Python/import.c b/Python/import.c index 09fe95fa1fb647..c95299a893e329 100644 --- a/Python/import.c +++ b/Python/import.c @@ -122,12 +122,15 @@ _PyImport_ReleaseLock(PyInterpreterState *interp) _PyRecursiveMutex_Unlock(&IMPORT_LOCK(interp)); } +#ifdef HAVE_FORK void -_PyImport_ReInitLock(PyInterpreterState *interp) +_PyImport_ReInitLock(PyInterpreterState *interp, PyThread_ident_t parent) { - // gh-126688: Thread id may change after fork() on some operating systems. - IMPORT_LOCK(interp).thread = PyThread_get_thread_ident_ex(); + /* The forking thread always holds the import lock. */ + assert(IMPORT_LOCK(interp).thread == parent); + _PyRecursiveMutex_at_fork_reinit(&IMPORT_LOCK(interp), parent); } +#endif /***************/ From 2aad2d5db78d0e45574b3a571804b79b12154880 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Thu, 21 Nov 2024 14:29:42 -0700 Subject: [PATCH 6/6] Call _PyImport_ReInitLock() in _PyRuntimeState_ReInitThreads(). --- Include/internal/pycore_runtime.h | 4 +++- Modules/posixmodule.c | 4 ++-- Python/pystate.c | 4 +++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 30937d29f72e1b..4970bbcb64dc38 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -219,7 +219,9 @@ extern PyStatus _PyRuntimeState_Init(_PyRuntimeState *runtime); extern void _PyRuntimeState_Fini(_PyRuntimeState *runtime); #ifdef HAVE_FORK -extern PyStatus _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime); +extern PyStatus _PyRuntimeState_ReInitThreads( + _PyRuntimeState *runtime, + PyThread_ident_t parent); #endif /* Initialize _PyRuntimeState. diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 51e3aac5e6e766..1abb8787f4ffb8 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -654,7 +654,8 @@ PyOS_AfterFork_Child(void) _PyRuntimeState *runtime = &_PyRuntime; // re-creates runtime->interpreters.mutex (HEAD_UNLOCK) - status = _PyRuntimeState_ReInitThreads(runtime); + status = _PyRuntimeState_ReInitThreads( + runtime, runtime->os_fork.parent.tid); if (_PyStatus_EXCEPTION(status)) { goto fatal_error; } @@ -697,7 +698,6 @@ PyOS_AfterFork_Child(void) _PyEval_StartTheWorldAll(&_PyRuntime); _PyThreadState_DeleteList(list); - _PyImport_ReInitLock(tstate->interp, _PyRuntime.os_fork.parent.tid); _PyImport_ReleaseLock(tstate->interp); _PySignal_AfterFork(); diff --git a/Python/pystate.c b/Python/pystate.c index 975eb6d4fbd0f2..2495dfecec9d68 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -497,7 +497,8 @@ _PyRuntimeState_Fini(_PyRuntimeState *runtime) /* This function is called from PyOS_AfterFork_Child to ensure that newly created child processes do not share locks with the parent. */ PyStatus -_PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) +_PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime, + PyThread_ident_t parent) { // This was initially set in _PyRuntimeState_Init(). runtime->main_thread = PyThread_get_thread_ident(); @@ -515,6 +516,7 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) for (PyInterpreterState *interp = runtime->interpreters.head; interp != NULL; interp = interp->next) { + _PyImport_ReInitLock(interp, parent); for (int i = 0; i < NUM_WEAKREF_LIST_LOCKS; i++) { _PyMutex_at_fork_reinit(&interp->weakref_locks[i]); }