From b4650baafe995ecb2fd17966752ac2642e6ca45a Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Sun, 19 May 2024 21:25:28 -0400 Subject: [PATCH 1/4] Fix getargs.c to store state in InterpreterState... ...as opposed to storing it in PyRuntime. Storing it in PyRuntime is fundametally wrong, as its state contains references to Python objects. Those objects (tuples and strings) can (and will) be picked by various subinterpreter clean up code, leaving PyRuntime with broken pointers. --- Include/internal/pycore_interp.h | 5 +++++ Include/internal/pycore_pylifecycle.h | 3 ++- Include/internal/pycore_runtime.h | 6 ------ Python/getargs.c | 28 +++++++++++++-------------- Python/pylifecycle.c | 3 ++- Python/pystate.c | 5 +++-- 6 files changed, 26 insertions(+), 24 deletions(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 37cc88ed081b72..5147501c57852c 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -39,6 +39,10 @@ struct _Py_long_state { int max_str_digits; }; +struct _getargs_runtime_state { + struct _PyArg_Parser *static_parsers; +}; + /* cross-interpreter data registry */ @@ -175,6 +179,7 @@ struct _is { PyObject *after_forkers_child; #endif + struct _getargs_runtime_state getargs; struct _warnings_runtime_state warnings; struct atexit_state atexit; diff --git a/Include/internal/pycore_pylifecycle.h b/Include/internal/pycore_pylifecycle.h index 7cd998a704c88d..24e24b36ade8af 100644 --- a/Include/internal/pycore_pylifecycle.h +++ b/Include/internal/pycore_pylifecycle.h @@ -62,7 +62,8 @@ extern void _PyAST_Fini(PyInterpreterState *interp); extern void _PyAtExit_Fini(PyInterpreterState *interp); extern void _PyThread_FiniType(PyInterpreterState *interp); extern void _Py_Deepfreeze_Fini(void); -extern void _PyArg_Fini(void); +extern void _PyArg_InitState(PyInterpreterState *interp); +extern void _PyArg_Fini(PyInterpreterState *interp); extern void _Py_FinalizeAllocatedBlocks(_PyRuntimeState *); extern PyStatus _PyGILState_Init(PyInterpreterState *interp); diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 99c4b0760bfb94..21b8ce6a687559 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -27,11 +27,6 @@ extern "C" { #include "pycore_typeobject.h" // struct types_runtime_state #include "pycore_unicodeobject.h" // struct _Py_unicode_runtime_ids -struct _getargs_runtime_state { - PyThread_type_lock mutex; - struct _PyArg_Parser *static_parsers; -}; - /* GIL state */ struct _gilstate_runtime_state { @@ -135,7 +130,6 @@ typedef struct pyruntimestate { struct _import_runtime_state imports; struct _ceval_runtime_state ceval; struct _gilstate_runtime_state gilstate; - struct _getargs_runtime_state getargs; struct _fileutils_state fileutils; struct _faulthandler_runtime_state faulthandler; struct _tracemalloc_runtime_state tracemalloc; diff --git a/Python/getargs.c b/Python/getargs.c index 5e731cdc23cb5f..f9bab8bb37ef61 100644 --- a/Python/getargs.c +++ b/Python/getargs.c @@ -4,6 +4,8 @@ #include "Python.h" #include "pycore_tuple.h" // _PyTuple_ITEMS() #include "pycore_pylifecycle.h" // _PyArg_Fini +#include "pycore_pyerrors.h" // _Py_CalculateSuggestions() +#include "pycore_interp.h" // _getargs_runtime_state #include #include @@ -2021,8 +2023,9 @@ _parser_init(struct _PyArg_Parser *parser) parser->initialized = owned ? 1 : -1; assert(parser->next == NULL); - parser->next = _PyRuntime.getargs.static_parsers; - _PyRuntime.getargs.static_parsers = parser; + PyInterpreterState *interp = PyInterpreterState_Get(); + parser->next = interp->getargs.static_parsers; + interp->getargs.static_parsers = parser; return 1; } @@ -2035,16 +2038,7 @@ parser_init(struct _PyArg_Parser *parser) assert(parser->kwtuple != NULL); return 1; } - PyThread_acquire_lock(_PyRuntime.getargs.mutex, WAIT_LOCK); - // Check again if another thread initialized the parser - // while we were waiting for the lock. - if (*((volatile int *)&parser->initialized)) { - assert(parser->kwtuple != NULL); - PyThread_release_lock(_PyRuntime.getargs.mutex); - return 1; - } int ret = _parser_init(parser); - PyThread_release_lock(_PyRuntime.getargs.mutex); return ret; } @@ -2943,16 +2937,22 @@ _PyArg_NoKwnames(const char *funcname, PyObject *kwnames) } void -_PyArg_Fini(void) +_PyArg_InitState(PyInterpreterState *interp) +{ + interp->getargs.static_parsers = NULL; +} + +void +_PyArg_Fini(PyInterpreterState *interp) { - struct _PyArg_Parser *tmp, *s = _PyRuntime.getargs.static_parsers; + struct _PyArg_Parser *tmp, *s = interp->getargs.static_parsers; while (s) { tmp = s->next; s->next = NULL; parser_clear(s); s = tmp; } - _PyRuntime.getargs.static_parsers = NULL; + interp->getargs.static_parsers = NULL; } #ifdef __cplusplus diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index a0130fde15d574..ed07173490d6b4 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -848,6 +848,8 @@ pycore_interp_init(PyThreadState *tstate) return _PyStatus_ERR("can't initialize warnings"); } + _PyArg_InitState(interp); + status = _PyAtExit_Init(interp); if (_PyStatus_EXCEPTION(status)) { return status; @@ -1751,7 +1753,6 @@ finalize_interp_clear(PyThreadState *tstate) if (is_main_interp) { _Py_HashRandomization_Fini(); - _PyArg_Fini(); _Py_ClearFileSystemEncoding(); _Py_Deepfreeze_Fini(); _PyPerfTrampoline_Fini(); diff --git a/Python/pystate.c b/Python/pystate.c index 1337516aa59cbc..2b457f12985bb6 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -380,12 +380,11 @@ _Py_COMP_DIAG_IGNORE_DEPR_DECLS static const _PyRuntimeState initial = _PyRuntimeState_INIT(_PyRuntime); _Py_COMP_DIAG_POP -#define NUMLOCKS 9 +#define NUMLOCKS 8 #define LOCKS_INIT(runtime) \ { \ &(runtime)->interpreters.mutex, \ &(runtime)->xidregistry.mutex, \ - &(runtime)->getargs.mutex, \ &(runtime)->unicode_state.ids.lock, \ &(runtime)->imports.extensions.mutex, \ &(runtime)->ceval.pending_mainthread.lock, \ @@ -887,6 +886,8 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate) _PyWarnings_Fini(interp); _PyAtExit_Fini(interp); + _PyArg_Fini(interp); + // All Python types must be destroyed before the last GC collection. Python // types create a reference cycle to themselves in their in their // PyTypeObject.tp_mro member (the tuple contains the type). From 4339e3ef2c5b184b51e677fdc463043394a9d662 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Sun, 19 May 2024 21:40:10 -0400 Subject: [PATCH 2/4] Add the removed struct from PyRuntime back to preserve ABI --- Include/internal/pycore_runtime.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 21b8ce6a687559..2146ab18513bd9 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -27,6 +27,14 @@ extern "C" { #include "pycore_typeobject.h" // struct types_runtime_state #include "pycore_unicodeobject.h" // struct _Py_unicode_runtime_ids +/* The following struct is moved to interpreter state in 3.13+. + We're keeping it here for ABI compat. + See also: https://github.com/python/cpython/pull/119194. */ +struct _UNUSED_getargs_runtime_state { + PyThread_type_lock mutex; + struct _PyArg_Parser *static_parsers; +}; + /* GIL state */ struct _gilstate_runtime_state { @@ -130,6 +138,7 @@ typedef struct pyruntimestate { struct _import_runtime_state imports; struct _ceval_runtime_state ceval; struct _gilstate_runtime_state gilstate; + struct _UNUSED_getargs_runtime_state _UNUSED_gilstate; struct _fileutils_state fileutils; struct _faulthandler_runtime_state faulthandler; struct _tracemalloc_runtime_state tracemalloc; From 2a30e1c2f7617a2123498a8010266892dbd77f29 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Sun, 19 May 2024 21:41:40 -0400 Subject: [PATCH 3/4] Drop unintentionally added #include --- Python/getargs.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Python/getargs.c b/Python/getargs.c index f9bab8bb37ef61..489a62d7cd83ce 100644 --- a/Python/getargs.c +++ b/Python/getargs.c @@ -4,7 +4,6 @@ #include "Python.h" #include "pycore_tuple.h" // _PyTuple_ITEMS() #include "pycore_pylifecycle.h" // _PyArg_Fini -#include "pycore_pyerrors.h" // _Py_CalculateSuggestions() #include "pycore_interp.h" // _getargs_runtime_state #include From 83d05d14fb60fbb4703c4b4713a57b3eadd47e87 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Sun, 19 May 2024 21:47:51 -0400 Subject: [PATCH 4/4] Move the newly added struct to the end of the _is struct --- Include/internal/pycore_interp.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 5147501c57852c..ad71bc64afb3d4 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -179,7 +179,6 @@ struct _is { PyObject *after_forkers_child; #endif - struct _getargs_runtime_state getargs; struct _warnings_runtime_state warnings; struct atexit_state atexit; @@ -235,6 +234,8 @@ struct _is { /* the initial PyInterpreterState.threads.head */ PyThreadState _initial_thread; + + struct _getargs_runtime_state getargs; };