From 6f8b84fd96ffb4f536d384c3d158db6796cc8789 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 7 Aug 2023 12:11:55 -0600 Subject: [PATCH 1/3] Unrevert "[3.12] gh-107080: Fix Py_TRACE_REFS Crashes Under Isolated Subinterpreters (gh-107567) (#107599)". This reverts commit 6e4eec760648a71e1cd8f8f551997b1823b4bb9f (gh-107648). --- Include/internal/pycore_object.h | 4 +- Include/internal/pycore_object_state.h | 13 +++-- Include/internal/pycore_runtime_init.h | 11 ++++ ...-08-02-12-24-51.gh-issue-107080.PNolFU.rst | 4 ++ Objects/object.c | 51 ++++++++++++------- Python/pylifecycle.c | 8 +-- 6 files changed, 62 insertions(+), 29 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2023-08-02-12-24-51.gh-issue-107080.PNolFU.rst diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 0981d1122fec54..5a9403456903ed 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -271,8 +271,8 @@ extern void _PyDebug_PrintTotalRefs(void); #ifdef Py_TRACE_REFS extern void _Py_AddToAllObjects(PyObject *op, int force); -extern void _Py_PrintReferences(FILE *); -extern void _Py_PrintReferenceAddresses(FILE *); +extern void _Py_PrintReferences(PyInterpreterState *, FILE *); +extern void _Py_PrintReferenceAddresses(PyInterpreterState *, FILE *); #endif diff --git a/Include/internal/pycore_object_state.h b/Include/internal/pycore_object_state.h index 94005d77881432..65feb5af969f8b 100644 --- a/Include/internal/pycore_object_state.h +++ b/Include/internal/pycore_object_state.h @@ -11,17 +11,22 @@ extern "C" { struct _py_object_runtime_state { #ifdef Py_REF_DEBUG Py_ssize_t interpreter_leaks; -#else - int _not_used; #endif + int _not_used; }; struct _py_object_state { #ifdef Py_REF_DEBUG Py_ssize_t reftotal; -#else - int _not_used; #endif +#ifdef Py_TRACE_REFS + /* Head of circular doubly-linked list of all objects. These are linked + * together via the _ob_prev and _ob_next members of a PyObject, which + * exist only in a Py_TRACE_REFS build. + */ + PyObject refchain; +#endif + int _not_used; }; diff --git a/Include/internal/pycore_runtime_init.h b/Include/internal/pycore_runtime_init.h index 4130188079cffa..7aace9f86119c4 100644 --- a/Include/internal/pycore_runtime_init.h +++ b/Include/internal/pycore_runtime_init.h @@ -101,6 +101,7 @@ extern PyTypeObject _PyExc_MemoryError; { .threshold = 10, }, \ }, \ }, \ + .object_state = _py_object_state_INIT(INTERP), \ .dtoa = _dtoa_state_INIT(&(INTERP)), \ .dict_state = _dict_state_INIT, \ .func_state = { \ @@ -130,6 +131,16 @@ extern PyTypeObject _PyExc_MemoryError; .context_ver = 1, \ } +#ifdef Py_TRACE_REFS +# define _py_object_state_INIT(INTERP) \ + { \ + .refchain = {&INTERP.object_state.refchain, &INTERP.object_state.refchain}, \ + } +#else +# define _py_object_state_INIT(INTERP) \ + { 0 } +#endif + // global objects diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-08-02-12-24-51.gh-issue-107080.PNolFU.rst b/Misc/NEWS.d/next/Core and Builtins/2023-08-02-12-24-51.gh-issue-107080.PNolFU.rst new file mode 100644 index 00000000000000..5084c854360e35 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2023-08-02-12-24-51.gh-issue-107080.PNolFU.rst @@ -0,0 +1,4 @@ +Trace refs builds (``--with-trace-refs``) were crashing when used with +isolated subinterpreters. The problematic global state has been isolated to +each interpreter. Other fixing the crashes, this change does not affect +users. diff --git a/Objects/object.c b/Objects/object.c index bd0fa40bd2a851..b4c9416036c44e 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -158,11 +158,8 @@ _PyDebug_PrintTotalRefs(void) { Do not call them otherwise, they do not initialize the object! */ #ifdef Py_TRACE_REFS -/* Head of circular doubly-linked list of all objects. These are linked - * together via the _ob_prev and _ob_next members of a PyObject, which - * exist only in a Py_TRACE_REFS build. - */ -static PyObject refchain = {&refchain, &refchain}; + +#define REFCHAIN(interp) &interp->object_state.refchain /* Insert op at the front of the list of all objects. If force is true, * op is added even if _ob_prev and _ob_next are non-NULL already. If @@ -187,10 +184,11 @@ _Py_AddToAllObjects(PyObject *op, int force) } #endif if (force || op->_ob_prev == NULL) { - op->_ob_next = refchain._ob_next; - op->_ob_prev = &refchain; - refchain._ob_next->_ob_prev = op; - refchain._ob_next = op; + PyObject *refchain = REFCHAIN(_PyInterpreterState_GET()); + op->_ob_next = refchain->_ob_next; + op->_ob_prev = refchain; + refchain->_ob_next->_ob_prev = op; + refchain->_ob_next = op; } } #endif /* Py_TRACE_REFS */ @@ -2206,7 +2204,8 @@ _Py_ForgetReference(PyObject *op) _PyObject_ASSERT_FAILED_MSG(op, "negative refcnt"); } - if (op == &refchain || + PyObject *refchain = REFCHAIN(_PyInterpreterState_GET()); + if (op == refchain || op->_ob_prev->_ob_next != op || op->_ob_next->_ob_prev != op) { _PyObject_ASSERT_FAILED_MSG(op, "invalid object chain"); @@ -2214,12 +2213,12 @@ _Py_ForgetReference(PyObject *op) #ifdef SLOW_UNREF_CHECK PyObject *p; - for (p = refchain._ob_next; p != &refchain; p = p->_ob_next) { + for (p = refchain->_ob_next; p != refchain; p = p->_ob_next) { if (p == op) { break; } } - if (p == &refchain) { + if (p == refchain) { /* Not found */ _PyObject_ASSERT_FAILED_MSG(op, "object not found in the objects list"); @@ -2235,11 +2234,15 @@ _Py_ForgetReference(PyObject *op) * interpreter must be in a healthy state. */ void -_Py_PrintReferences(FILE *fp) +_Py_PrintReferences(PyInterpreterState *interp, FILE *fp) { PyObject *op; + if (interp == NULL) { + interp = _PyInterpreterState_Main(); + } fprintf(fp, "Remaining objects:\n"); - for (op = refchain._ob_next; op != &refchain; op = op->_ob_next) { + PyObject *refchain = REFCHAIN(interp); + for (op = refchain->_ob_next; op != refchain; op = op->_ob_next) { fprintf(fp, "%p [%zd] ", (void *)op, Py_REFCNT(op)); if (PyObject_Print(op, fp, 0) != 0) { PyErr_Clear(); @@ -2251,34 +2254,42 @@ _Py_PrintReferences(FILE *fp) /* Print the addresses of all live objects. Unlike _Py_PrintReferences, this * doesn't make any calls to the Python C API, so is always safe to call. */ +// XXX This function is not safe to use if the interpreter has been +// freed or is in an unhealthy state (e.g. late in finalization). +// The call in Py_FinalizeEx() is okay since the main interpreter +// is statically allocated. void -_Py_PrintReferenceAddresses(FILE *fp) +_Py_PrintReferenceAddresses(PyInterpreterState *interp, FILE *fp) { PyObject *op; + PyObject *refchain = REFCHAIN(interp); fprintf(fp, "Remaining object addresses:\n"); - for (op = refchain._ob_next; op != &refchain; op = op->_ob_next) + for (op = refchain->_ob_next; op != refchain; op = op->_ob_next) fprintf(fp, "%p [%zd] %s\n", (void *)op, Py_REFCNT(op), Py_TYPE(op)->tp_name); } +/* The implementation of sys.getobjects(). */ PyObject * _Py_GetObjects(PyObject *self, PyObject *args) { int i, n; PyObject *t = NULL; PyObject *res, *op; + PyInterpreterState *interp = _PyInterpreterState_GET(); if (!PyArg_ParseTuple(args, "i|O", &n, &t)) return NULL; - op = refchain._ob_next; + PyObject *refchain = REFCHAIN(interp); + op = refchain->_ob_next; res = PyList_New(0); if (res == NULL) return NULL; - for (i = 0; (n == 0 || i < n) && op != &refchain; i++) { + for (i = 0; (n == 0 || i < n) && op != refchain; i++) { while (op == self || op == args || op == res || op == t || (t != NULL && !Py_IS_TYPE(op, (PyTypeObject *) t))) { op = op->_ob_next; - if (op == &refchain) + if (op == refchain) return res; } if (PyList_Append(res, op) < 0) { @@ -2290,6 +2301,8 @@ _Py_GetObjects(PyObject *self, PyObject *args) return res; } +#undef REFCHAIN + #endif diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index a67fa26b372278..5b8c8af179c005 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1920,11 +1920,11 @@ Py_FinalizeEx(void) } if (dump_refs) { - _Py_PrintReferences(stderr); + _Py_PrintReferences(tstate->interp, stderr); } if (dump_refs_fp != NULL) { - _Py_PrintReferences(dump_refs_fp); + _Py_PrintReferences(tstate->interp, dump_refs_fp); } #endif /* Py_TRACE_REFS */ @@ -1960,11 +1960,11 @@ Py_FinalizeEx(void) */ if (dump_refs) { - _Py_PrintReferenceAddresses(stderr); + _Py_PrintReferenceAddresses(tstate->interp, stderr); } if (dump_refs_fp != NULL) { - _Py_PrintReferenceAddresses(dump_refs_fp); + _Py_PrintReferenceAddresses(tstate->interp, dump_refs_fp); fclose(dump_refs_fp); } #endif /* Py_TRACE_REFS */ From 24a455adac3dbab331d804fc675d0461873c9e54 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 7 Aug 2023 12:04:17 -0600 Subject: [PATCH 2/3] Initialize each interpreter's refchain properly. --- Include/internal/pycore_object.h | 1 + Objects/object.c | 22 +++++++++++++++++++++- Python/pylifecycle.c | 2 ++ Python/pystate.c | 1 + 4 files changed, 25 insertions(+), 1 deletion(-) diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 5a9403456903ed..7a2f13a21bda76 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -152,6 +152,7 @@ _PyType_HasFeature(PyTypeObject *type, unsigned long feature) { extern void _PyType_InitCache(PyInterpreterState *interp); +extern void _PyObject_InitState(PyInterpreterState *interp); /* Inline functions trading binary compatibility for speed: _PyObject_Init() is the fast version of PyObject_Init(), and diff --git a/Objects/object.c b/Objects/object.c index b4c9416036c44e..8a4010fb13669b 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -161,6 +161,14 @@ _PyDebug_PrintTotalRefs(void) { #define REFCHAIN(interp) &interp->object_state.refchain +static inline void +init_refchain(PyInterpreterState *interp) +{ + PyObject *refchain = REFCHAIN(interp); + refchain->_ob_prev = refchain; + refchain->_ob_next = refchain; +} + /* Insert op at the front of the list of all objects. If force is true, * op is added even if _ob_prev and _ob_next are non-NULL already. If * force is false amd _ob_prev or _ob_next are non-NULL, do nothing. @@ -1996,6 +2004,18 @@ PyObject _Py_NotImplementedStruct = { &_PyNotImplemented_Type }; + +void +_PyObject_InitState(PyInterpreterState *interp) +{ +#ifdef Py_TRACE_REFS + if (!_Py_IsMainInterpreter(interp)) { + init_refchain(interp); + } +#endif +} + + extern PyTypeObject _Py_GenericAliasIterType; extern PyTypeObject _PyMemoryIter_Type; extern PyTypeObject _PyLineIterator; @@ -2303,7 +2323,7 @@ _Py_GetObjects(PyObject *self, PyObject *args) #undef REFCHAIN -#endif +#endif /* Py_TRACE_REFS */ /* Hack to force loading of abstract.o */ diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 5b8c8af179c005..29771e07ae6a2c 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2074,6 +2074,8 @@ new_interpreter(PyThreadState **tstate_p, const PyInterpreterConfig *config) } has_gil = 1; + /* No objects have been created yet. */ + status = pycore_interp_init(tstate); if (_PyStatus_EXCEPTION(status)) { goto error; diff --git a/Python/pystate.c b/Python/pystate.c index 8097124965cb7c..2ee16e3de25da3 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -673,6 +673,7 @@ init_interpreter(PyInterpreterState *interp, _obmalloc_pools_INIT(interp->obmalloc.pools); memcpy(&interp->obmalloc.pools.used, temp, sizeof(temp)); } + _PyObject_InitState(interp); _PyEval_InitState(interp, pending_lock); _PyGC_InitState(&interp->gc); From 6eb6e60a3ee41790f60f44e370a9ebd500f13877 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 7 Aug 2023 15:16:06 -0600 Subject: [PATCH 3/3] Skip test_basic_multiple_interpreters_deleted_no_reset on tracerefs builds. --- Lib/test/test_import/__init__.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 06adf01f18c0a4..f7df6d7e42319f 100644 --- a/Lib/test/test_import/__init__.py +++ b/Lib/test/test_import/__init__.py @@ -2539,6 +2539,12 @@ def test_basic_multiple_interpreters_main_no_reset(self): def test_basic_multiple_interpreters_deleted_no_reset(self): # without resetting; already loaded in a deleted interpreter + if hasattr(sys, 'getobjects'): + # It's a Py_TRACE_REFS build. + # This test breaks interpreter isolation a little, + # which causes problems on Py_TRACE_REF builds. + raise unittest.SkipTest('crashes on Py_TRACE_REFS builds') + # At this point: # * alive in 0 interpreters # * module def may or may not be loaded already