From a5b6574a03233925021f12cea73c5f14e5f11ce2 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 9 Oct 2024 11:32:16 -0600 Subject: [PATCH 1/2] gh-116510: Fix a Crash Due to Shared Immortal Interned Strings (gh-124865) Fix a crash caused by immortal interned strings being shared between sub-interpreters that use basic single-phase init. In that case, the string can be used by an interpreter that outlives the interpreter that created and interned it. For interpreters that share obmalloc state, also share the interned dict with the main interpreter. This is an un-revert of gh-124646 that then addresses the Py_TRACE_REFS failures identified by gh-124785. (cherry picked from commit f2cb39947093feda3ff85b8dc820922cc5e5f954) Co-authored-by: Eric Snow --- ...-09-26-18-21-06.gh-issue-116510.FacUWO.rst | 5 ++ Objects/object.c | 36 ++++++++++++ Objects/unicodeobject.c | 56 +++++++++++++++++-- 3 files changed, 91 insertions(+), 6 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-09-26-18-21-06.gh-issue-116510.FacUWO.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-09-26-18-21-06.gh-issue-116510.FacUWO.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-09-26-18-21-06.gh-issue-116510.FacUWO.rst new file mode 100644 index 00000000000000..e3741321006548 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-09-26-18-21-06.gh-issue-116510.FacUWO.rst @@ -0,0 +1,5 @@ +Fix a crash caused by immortal interned strings being shared between +sub-interpreters that use basic single-phase init. In that case, the string +can be used by an interpreter that outlives the interpreter that created and +interned it. For interpreters that share obmalloc state, also share the +interned dict with the main interpreter. diff --git a/Objects/object.c b/Objects/object.c index cbf576d5e5aee3..c35883e1db2704 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -2481,6 +2481,42 @@ _Py_ResurrectReference(PyObject *op) #ifdef Py_TRACE_REFS +/* Make sure the ref is associated with the right interpreter. + * This only needs special attention for heap-allocated objects + * that have been immortalized, and only when the object might + * outlive the interpreter where it was created. That means the + * object was necessarily created using a global allocator + * (i.e. from the main interpreter). Thus in that specific case + * we move the object over to the main interpreter's refchain. + * + * This was added for the sake of the immortal interned strings, + * where legacy subinterpreters share the main interpreter's + * interned dict (and allocator), and therefore the strings can + * outlive the subinterpreter. + * + * It may make sense to fold this into _Py_SetImmortalUntracked(), + * but that requires further investigation. In the meantime, it is + * up to the caller to know if this is needed. There should be + * very few cases. + */ +void +_Py_NormalizeImmortalReference(PyObject *op) +{ + assert(_Py_IsImmortal(op)); + PyInterpreterState *interp = _PyInterpreterState_GET(); + if (!_PyRefchain_IsTraced(interp, op)) { + return; + } + PyInterpreterState *main_interp = _PyInterpreterState_Main(); + if (interp != main_interp + && interp->feature_flags & Py_RTFLAGS_USE_MAIN_OBMALLOC) + { + assert(!_PyRefchain_IsTraced(main_interp, op)); + _PyRefchain_Remove(interp, op); + _PyRefchain_Trace(main_interp, op); + } +} + void _Py_ForgetReference(PyObject *op) { diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c index 5a6ae78fe23bae..4799872920f584 100644 --- a/Objects/unicodeobject.c +++ b/Objects/unicodeobject.c @@ -277,13 +277,37 @@ hashtable_unicode_compare(const void *key1, const void *key2) } } +/* Return true if this interpreter should share the main interpreter's + intern_dict. That's important for interpreters which load basic + single-phase init extension modules (m_size == -1). There could be interned + immortal strings that are shared between interpreters, due to the + PyDict_Update(mdict, m_copy) call in import_find_extension(). + + It's not safe to deallocate those strings until all interpreters that + potentially use them are freed. By storing them in the main interpreter, we + ensure they get freed after all other interpreters are freed. +*/ +static bool +has_shared_intern_dict(PyInterpreterState *interp) +{ + PyInterpreterState *main_interp = _PyInterpreterState_Main(); + return interp != main_interp && interp->feature_flags & Py_RTFLAGS_USE_MAIN_OBMALLOC; +} + static int init_interned_dict(PyInterpreterState *interp) { assert(get_interned_dict(interp) == NULL); - PyObject *interned = interned = PyDict_New(); - if (interned == NULL) { - return -1; + PyObject *interned; + if (has_shared_intern_dict(interp)) { + interned = get_interned_dict(_PyInterpreterState_Main()); + Py_INCREF(interned); + } + else { + interned = PyDict_New(); + if (interned == NULL) { + return -1; + } } _Py_INTERP_CACHED_OBJECT(interp, interned_strings) = interned; return 0; @@ -294,7 +318,10 @@ clear_interned_dict(PyInterpreterState *interp) { PyObject *interned = get_interned_dict(interp); if (interned != NULL) { - PyDict_Clear(interned); + if (!has_shared_intern_dict(interp)) { + // only clear if the dict belongs to this interpreter + PyDict_Clear(interned); + } Py_DECREF(interned); _Py_INTERP_CACHED_OBJECT(interp, interned_strings) = NULL; } @@ -15098,6 +15125,10 @@ _PyUnicode_InternStatic(PyInterpreterState *interp, PyObject **p) assert(*p); } +#ifdef Py_TRACE_REFS +extern void _Py_NormalizeImmortalReference(PyObject *); +#endif + static void immortalize_interned(PyObject *s) { @@ -15113,6 +15144,10 @@ immortalize_interned(PyObject *s) #endif _PyUnicode_STATE(s).interned = SSTATE_INTERNED_IMMORTAL; _Py_SetImmortal(s); +#ifdef Py_TRACE_REFS + /* Make sure the ref is associated with the right interpreter. */ + _Py_NormalizeImmortalReference(s); +#endif } static /* non-null */ PyObject* @@ -15306,6 +15341,13 @@ _PyUnicode_ClearInterned(PyInterpreterState *interp) } assert(PyDict_CheckExact(interned)); + if (has_shared_intern_dict(interp)) { + // the dict doesn't belong to this interpreter, skip the debug + // checks on it and just clear the pointer to it + clear_interned_dict(interp); + return; + } + #ifdef INTERNED_STATS fprintf(stderr, "releasing %zd interned strings\n", PyDict_GET_SIZE(interned)); @@ -15827,8 +15869,10 @@ _PyUnicode_Fini(PyInterpreterState *interp) { struct _Py_unicode_state *state = &interp->unicode; - // _PyUnicode_ClearInterned() must be called before _PyUnicode_Fini() - assert(get_interned_dict(interp) == NULL); + if (!has_shared_intern_dict(interp)) { + // _PyUnicode_ClearInterned() must be called before _PyUnicode_Fini() + assert(get_interned_dict(interp) == NULL); + } _PyUnicode_FiniEncodings(&state->fs_codec); From 909f70e8c685e7468661ede82e9a1b66c1124954 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 23 Oct 2024 10:10:06 -0600 Subject: [PATCH 2/2] [3.13] gh-125286: Share the Main Refchain With Legacy Interpreters (gh-125709) They used to be shared, before 3.12. Returning to sharing them resolves a failure on Py_TRACE_REFS builds. --- Doc/library/sys.rst | 29 +++++++++++++ Doc/using/configure.rst | 2 +- Doc/whatsnew/3.13.rst | 11 +++++ Objects/object.c | 92 ++++++++++++++++++++--------------------- Objects/unicodeobject.c | 8 ---- Python/pylifecycle.c | 14 +++++++ Python/pystate.c | 6 +-- 7 files changed, 101 insertions(+), 61 deletions(-) diff --git a/Doc/library/sys.rst b/Doc/library/sys.rst index 1e1f65851fd811..76770a9d797036 100644 --- a/Doc/library/sys.rst +++ b/Doc/library/sys.rst @@ -920,6 +920,35 @@ always available. It is not guaranteed to exist in all implementations of Python. +.. function:: getobjects(limit[, type]) + + This function only exists if CPython was built using the + specialized configure option :option:`--with-trace-refs`. + It is intended only for debugging garbage-collection issues. + + Return a list of up to *limit* dynamically allocated Python objects. + If *type* is given, only objects of that exact type (not subtypes) + are included. + + Objects from the list are not safe to use. + Specifically, the result will include objects from all interpreters that + share their object allocator state (that is, ones created with + :c:member:`PyInterpreterConfig.use_main_obmalloc` set to 1 + or using :c:func:`Py_NewInterpreter`, and the + :ref:`main interpreter `). + Mixing objects from different interpreters may lead to crashes + or other unexpected behavior. + + .. impl-detail:: + + This function should be used for specialized purposes only. + It is not guaranteed to exist in all implementations of Python. + + .. versionchanged:: next + + The result may include objects from other interpreters. + + .. function:: getprofile() .. index:: diff --git a/Doc/using/configure.rst b/Doc/using/configure.rst index 052289bd827366..713b2afc91d199 100644 --- a/Doc/using/configure.rst +++ b/Doc/using/configure.rst @@ -708,7 +708,7 @@ Debug options Effects: * Define the ``Py_TRACE_REFS`` macro. - * Add :func:`!sys.getobjects` function. + * Add :func:`sys.getobjects` function. * Add :envvar:`PYTHONDUMPREFS` environment variable. The :envvar:`PYTHONDUMPREFS` environment variable can be used to dump diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index a2897097aaba57..253d97c7e9577b 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -2706,3 +2706,14 @@ Regression Test Changes option. If used, it specifies a module that should be imported early in the lifecycle of the interpreter, before ``site.py`` is executed. (Contributed by Łukasz Langa in :gh:`110769`.) + + +Notable changes in 3.13.1 +========================= + +sys +--- + +* The previously undocumented special function :func:`sys.getobjects`, + which only exists in specialized builds of Python, may now return objects + from other interpreters than the one it's called in. diff --git a/Objects/object.c b/Objects/object.c index c35883e1db2704..43d1761d5161cf 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -169,6 +169,48 @@ _PyDebug_PrintTotalRefs(void) { #define REFCHAIN(interp) interp->object_state.refchain #define REFCHAIN_VALUE ((void*)(uintptr_t)1) +static inline int +has_own_refchain(PyInterpreterState *interp) +{ + if (interp->feature_flags & Py_RTFLAGS_USE_MAIN_OBMALLOC) { + return (_Py_IsMainInterpreter(interp) + || _PyInterpreterState_Main() == NULL); + } + return 1; +} + +static int +refchain_init(PyInterpreterState *interp) +{ + if (!has_own_refchain(interp)) { + // Legacy subinterpreters share a refchain with the main interpreter. + REFCHAIN(interp) = REFCHAIN(_PyInterpreterState_Main()); + return 0; + } + _Py_hashtable_allocator_t alloc = { + // Don't use default PyMem_Malloc() and PyMem_Free() which + // require the caller to hold the GIL. + .malloc = PyMem_RawMalloc, + .free = PyMem_RawFree, + }; + REFCHAIN(interp) = _Py_hashtable_new_full( + _Py_hashtable_hash_ptr, _Py_hashtable_compare_direct, + NULL, NULL, &alloc); + if (REFCHAIN(interp) == NULL) { + return -1; + } + return 0; +} + +static void +refchain_fini(PyInterpreterState *interp) +{ + if (has_own_refchain(interp) && REFCHAIN(interp) != NULL) { + _Py_hashtable_destroy(REFCHAIN(interp)); + } + REFCHAIN(interp) = NULL; +} + bool _PyRefchain_IsTraced(PyInterpreterState *interp, PyObject *obj) { @@ -2171,16 +2213,7 @@ PyStatus _PyObject_InitState(PyInterpreterState *interp) { #ifdef Py_TRACE_REFS - _Py_hashtable_allocator_t alloc = { - // Don't use default PyMem_Malloc() and PyMem_Free() which - // require the caller to hold the GIL. - .malloc = PyMem_RawMalloc, - .free = PyMem_RawFree, - }; - REFCHAIN(interp) = _Py_hashtable_new_full( - _Py_hashtable_hash_ptr, _Py_hashtable_compare_direct, - NULL, NULL, &alloc); - if (REFCHAIN(interp) == NULL) { + if (refchain_init(interp) < 0) { return _PyStatus_NO_MEMORY(); } #endif @@ -2191,8 +2224,7 @@ void _PyObject_FiniState(PyInterpreterState *interp) { #ifdef Py_TRACE_REFS - _Py_hashtable_destroy(REFCHAIN(interp)); - REFCHAIN(interp) = NULL; + refchain_fini(interp); #endif } @@ -2481,42 +2513,6 @@ _Py_ResurrectReference(PyObject *op) #ifdef Py_TRACE_REFS -/* Make sure the ref is associated with the right interpreter. - * This only needs special attention for heap-allocated objects - * that have been immortalized, and only when the object might - * outlive the interpreter where it was created. That means the - * object was necessarily created using a global allocator - * (i.e. from the main interpreter). Thus in that specific case - * we move the object over to the main interpreter's refchain. - * - * This was added for the sake of the immortal interned strings, - * where legacy subinterpreters share the main interpreter's - * interned dict (and allocator), and therefore the strings can - * outlive the subinterpreter. - * - * It may make sense to fold this into _Py_SetImmortalUntracked(), - * but that requires further investigation. In the meantime, it is - * up to the caller to know if this is needed. There should be - * very few cases. - */ -void -_Py_NormalizeImmortalReference(PyObject *op) -{ - assert(_Py_IsImmortal(op)); - PyInterpreterState *interp = _PyInterpreterState_GET(); - if (!_PyRefchain_IsTraced(interp, op)) { - return; - } - PyInterpreterState *main_interp = _PyInterpreterState_Main(); - if (interp != main_interp - && interp->feature_flags & Py_RTFLAGS_USE_MAIN_OBMALLOC) - { - assert(!_PyRefchain_IsTraced(main_interp, op)); - _PyRefchain_Remove(interp, op); - _PyRefchain_Trace(main_interp, op); - } -} - void _Py_ForgetReference(PyObject *op) { diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c index 4799872920f584..c69a64de062baa 100644 --- a/Objects/unicodeobject.c +++ b/Objects/unicodeobject.c @@ -15125,10 +15125,6 @@ _PyUnicode_InternStatic(PyInterpreterState *interp, PyObject **p) assert(*p); } -#ifdef Py_TRACE_REFS -extern void _Py_NormalizeImmortalReference(PyObject *); -#endif - static void immortalize_interned(PyObject *s) { @@ -15144,10 +15140,6 @@ immortalize_interned(PyObject *s) #endif _PyUnicode_STATE(s).interned = SSTATE_INTERNED_IMMORTAL; _Py_SetImmortal(s); -#ifdef Py_TRACE_REFS - /* Make sure the ref is associated with the right interpreter. */ - _Py_NormalizeImmortalReference(s); -#endif } static /* non-null */ PyObject* diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 1701a1cd217440..0cd4fb417636a2 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -670,6 +670,13 @@ pycore_create_interpreter(_PyRuntimeState *runtime, return status; } + // This could be done in init_interpreter() (in pystate.c) if it + // didn't depend on interp->feature_flags being set already. + status = _PyObject_InitState(interp); + if (_PyStatus_EXCEPTION(status)) { + return status; + } + // initialize the interp->obmalloc state. This must be done after // the settings are loaded (so that feature_flags are set) but before // any calls are made to obmalloc functions. @@ -2290,6 +2297,13 @@ new_interpreter(PyThreadState **tstate_p, goto error; } + // This could be done in init_interpreter() (in pystate.c) if it + // didn't depend on interp->feature_flags being set already. + status = _PyObject_InitState(interp); + if (_PyStatus_EXCEPTION(status)) { + return status; + } + // initialize the interp->obmalloc state. This must be done after // the settings are loaded (so that feature_flags are set) but before // any calls are made to obmalloc functions. diff --git a/Python/pystate.c b/Python/pystate.c index 66fd392c3e3ad0..ad3fdce69bf1d5 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -632,10 +632,8 @@ init_interpreter(PyInterpreterState *interp, assert(next != NULL || (interp == runtime->interpreters.main)); interp->next = next; - PyStatus status = _PyObject_InitState(interp); - if (_PyStatus_EXCEPTION(status)) { - return status; - } + // We would call _PyObject_InitState() at this point + // if interp->feature_flags were alredy set. _PyEval_InitState(interp); _PyGC_InitState(&interp->gc);