From aa21b07c2f3480b8be442773e9860ab2c7e632fe Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 22 Oct 2024 21:03:54 +0000 Subject: [PATCH] gh-125859: Fix crash when `gc.get_objects` is called during GC This fixes a crash when `gc.get_objects()` or `gc.get_referrers()` is called during a GC in the free threading build. Switch to `_PyObjectStack` to avoid corrupting the `struct worklist` linked list maintained by the GC. Also, don't return objects that are frozen (gc.freeze) or in the process of being collected to more closely match the behavior of the default build. --- Lib/test/test_gc.py | 23 ++++ ...-10-23-14-42-27.gh-issue-125859.m3EF9E.rst | 2 + Python/gc_free_threading.c | 107 +++++++++--------- 3 files changed, 77 insertions(+), 55 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-10-23-14-42-27.gh-issue-125859.m3EF9E.rst diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index bb7df1f5cfa7f7..cc2b4fac05b48b 100644 --- a/Lib/test/test_gc.py +++ b/Lib/test/test_gc.py @@ -1065,6 +1065,29 @@ def test_get_referents_on_capsule(self): self.assertEqual(len(gc.get_referents(untracked_capsule)), 0) gc.get_referents(tracked_capsule) + @cpython_only + def test_get_objects_during_gc(self): + # gh-125859: Calling gc.get_objects() or gc.get_referrers() during a + # collection should not crash. + test = self + collected = False + + class GetObjectsOnDel: + def __del__(self): + nonlocal collected + collected = True + objs = gc.get_objects() + # NB: can't use "in" here because some objects override __eq__ + for obj in objs: + test.assertTrue(obj is not self) + test.assertEqual(gc.get_referrers(self), []) + + obj = GetObjectsOnDel() + obj.cycle = obj + del obj + + gc.collect() + self.assertTrue(collected) class IncrementalGCTests(unittest.TestCase): diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-10-23-14-42-27.gh-issue-125859.m3EF9E.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-10-23-14-42-27.gh-issue-125859.m3EF9E.rst new file mode 100644 index 00000000000000..d36aa8fbe7482f --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-10-23-14-42-27.gh-issue-125859.m3EF9E.rst @@ -0,0 +1,2 @@ +Fix a crash in the free threading build when :func:`gc.get_objects` or +:func:`gc.get_referrers` is called during an in-progress garbage collection. diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 8558d4555a9a3a..aa8912d721ba70 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -1404,7 +1404,7 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason) struct get_referrers_args { struct visitor_args base; PyObject *objs; - struct worklist results; + _PyObjectStack results; }; static int @@ -1428,11 +1428,17 @@ visit_get_referrers(const mi_heap_t *heap, const mi_heap_area_t *area, if (op == NULL) { return true; } + if (op->ob_gc_bits & (_PyGC_BITS_UNREACHABLE | _PyGC_BITS_FROZEN)) { + // Exclude unreachable objects (in-progress GC) and frozen + // objects from gc.get_objects() to match the default build. + return true; + } struct get_referrers_args *arg = (struct get_referrers_args *)args; if (Py_TYPE(op)->tp_traverse(op, referrersvisit, arg->objs)) { - op->ob_tid = 0; // we will restore the refcount later - worklist_push(&arg->results, op); + if (_PyObjectStack_Push(&arg->results, op) < 0) { + return false; + } } return true; @@ -1446,43 +1452,36 @@ _PyGC_GetReferrers(PyInterpreterState *interp, PyObject *objs) return NULL; } + // NOTE: We can't append to the PyListObject during gc_visit_heaps() + // because PyList_Append() may reclaim an abandoned mimalloc segments + // while we are traversing them. + struct get_referrers_args args = { .objs = objs }; _PyEval_StopTheWorld(interp); + int err = gc_visit_heaps(interp, &visit_get_referrers, &args.base); + _PyEval_StartTheWorld(interp); - // Append all objects to a worklist. This abuses ob_tid. We will restore - // it later. NOTE: We can't append to the PyListObject during - // gc_visit_heaps() because PyList_Append() may reclaim an abandoned - // mimalloc segments while we are traversing them. - struct get_referrers_args args = { .objs = objs }; - gc_visit_heaps(interp, &visit_get_referrers, &args.base); + if (err < 0) { + PyErr_NoMemory(); + goto error; + } - bool error = false; PyObject *op; - while ((op = worklist_pop(&args.results)) != NULL) { - gc_restore_tid(op); + while ((op = _PyObjectStack_Pop(&args.results)) != NULL) { if (op != objs && PyList_Append(result, op) < 0) { - error = true; - break; + goto error; } } - - // In case of error, clear the remaining worklist - while ((op = worklist_pop(&args.results)) != NULL) { - gc_restore_tid(op); - } - - _PyEval_StartTheWorld(interp); - - if (error) { - Py_DECREF(result); - return NULL; - } - return result; + +error: + Py_DECREF(result); + _PyObjectStack_Clear(&args.results); + return NULL; } struct get_objects_args { struct visitor_args base; - struct worklist objects; + _PyObjectStack objects; }; static bool @@ -1493,11 +1492,16 @@ visit_get_objects(const mi_heap_t *heap, const mi_heap_area_t *area, if (op == NULL) { return true; } + if (op->ob_gc_bits & (_PyGC_BITS_UNREACHABLE | _PyGC_BITS_FROZEN)) { + // Exclude unreachable objects (in-progress GC) and frozen + // objects from gc.get_objects() to match the default build. + return true; + } struct get_objects_args *arg = (struct get_objects_args *)args; - op->ob_tid = 0; // we will restore the refcount later - worklist_push(&arg->objects, op); - + if (_PyObjectStack_Push(&arg->objects, op) < 0) { + return false; + } return true; } @@ -1509,38 +1513,31 @@ _PyGC_GetObjects(PyInterpreterState *interp, int generation) return NULL; } + // NOTE: We can't append to the PyListObject during gc_visit_heaps() + // because PyList_Append() may reclaim an abandoned mimalloc segments + // while we are traversing them. + struct get_objects_args args = { 0 }; _PyEval_StopTheWorld(interp); + int err = gc_visit_heaps(interp, &visit_get_objects, &args.base); + _PyEval_StartTheWorld(interp); - // Append all objects to a worklist. This abuses ob_tid. We will restore - // it later. NOTE: We can't append to the list during gc_visit_heaps() - // because PyList_Append() may reclaim an abandoned mimalloc segment - // while we are traversing it. - struct get_objects_args args = { 0 }; - gc_visit_heaps(interp, &visit_get_objects, &args.base); + if (err < 0) { + PyErr_NoMemory(); + goto error; + } - bool error = false; PyObject *op; - while ((op = worklist_pop(&args.objects)) != NULL) { - gc_restore_tid(op); + while ((op = _PyObjectStack_Pop(&args.objects)) != NULL) { if (op != result && PyList_Append(result, op) < 0) { - error = true; - break; + goto error; } } - - // In case of error, clear the remaining worklist - while ((op = worklist_pop(&args.objects)) != NULL) { - gc_restore_tid(op); - } - - _PyEval_StartTheWorld(interp); - - if (error) { - Py_DECREF(result); - return NULL; - } - return result; + +error: + Py_DECREF(result); + _PyObjectStack_Clear(&args.objects); + return NULL; } static bool