From aa21b07c2f3480b8be442773e9860ab2c7e632fe Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 22 Oct 2024 21:03:54 +0000 Subject: [PATCH 1/2] 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 bb7df1f5cfa7f78..cc2b4fac05b48b2 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 000000000000000..d36aa8fbe7482f4 --- /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 8558d4555a9a3af..aa8912d721ba701 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 From b6b9b7a97008c54a1a825e24069efcd9a868bf2d Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 23 Oct 2024 19:00:55 +0000 Subject: [PATCH 2/2] Ensure objects found by gc.get_objects() are kept alive. After the `_PyEval_StartTheWorld()` call, other threads may be running and mutating objects. Ensure that the objects are kept alive by incref'ing them when they're added to the `_PyObjectStack`. --- Include/internal/pycore_object_stack.h | 10 ++++ Lib/test/test_free_threading/test_gc.py | 61 ++++++++++++++++++++ Python/gc_free_threading.c | 74 ++++++++++++------------- 3 files changed, 105 insertions(+), 40 deletions(-) create mode 100644 Lib/test/test_free_threading/test_gc.py diff --git a/Include/internal/pycore_object_stack.h b/Include/internal/pycore_object_stack.h index c607ea8bc525458..39e69b7cde52a18 100644 --- a/Include/internal/pycore_object_stack.h +++ b/Include/internal/pycore_object_stack.h @@ -71,6 +71,16 @@ _PyObjectStack_Pop(_PyObjectStack *stack) return obj; } +static inline Py_ssize_t +_PyObjectStack_Size(_PyObjectStack *stack) +{ + Py_ssize_t size = 0; + for (_PyObjectStackChunk *buf = stack->head; buf != NULL; buf = buf->prev) { + size += buf->n; + } + return size; +} + // Merge src into dst, leaving src empty extern void _PyObjectStack_Merge(_PyObjectStack *dst, _PyObjectStack *src); diff --git a/Lib/test/test_free_threading/test_gc.py b/Lib/test/test_free_threading/test_gc.py new file mode 100644 index 000000000000000..401067fe9c612c5 --- /dev/null +++ b/Lib/test/test_free_threading/test_gc.py @@ -0,0 +1,61 @@ +import unittest + +import threading +from threading import Thread +from unittest import TestCase +import gc + +from test.support import threading_helper + + +class MyObj: + pass + + +@threading_helper.requires_working_threading() +class TestGC(TestCase): + def test_get_objects(self): + event = threading.Event() + + def gc_thread(): + for i in range(100): + o = gc.get_objects() + event.set() + + def mutator_thread(): + while not event.is_set(): + o1 = MyObj() + o2 = MyObj() + o3 = MyObj() + o4 = MyObj() + + gcs = [Thread(target=gc_thread)] + mutators = [Thread(target=mutator_thread) for _ in range(4)] + with threading_helper.start_threads(gcs + mutators): + pass + + def test_get_referrers(self): + event = threading.Event() + + obj = MyObj() + + def gc_thread(): + for i in range(100): + o = gc.get_referrers(obj) + event.set() + + def mutator_thread(): + while not event.is_set(): + d1 = { "key": obj } + d2 = { "key": obj } + d3 = { "key": obj } + d4 = { "key": obj } + + gcs = [Thread(target=gc_thread) for _ in range(2)] + mutators = [Thread(target=mutator_thread) for _ in range(4)] + with threading_helper.start_threads(gcs + mutators): + pass + + +if __name__ == "__main__": + unittest.main() diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index aa8912d721ba701..1969ed608ea524c 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -1401,6 +1401,28 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason) return n + m; } +static PyObject * +list_from_object_stack(_PyObjectStack *stack) +{ + PyObject *list = PyList_New(_PyObjectStack_Size(stack)); + if (list == NULL) { + PyObject *op; + while ((op = _PyObjectStack_Pop(stack)) != NULL) { + Py_DECREF(op); + } + return NULL; + } + + PyObject *op; + Py_ssize_t idx = 0; + while ((op = _PyObjectStack_Pop(stack)) != NULL) { + assert(idx < PyList_GET_SIZE(list)); + PyList_SET_ITEM(list, idx++, op); + } + assert(idx == PyList_GET_SIZE(list)); + return list; +} + struct get_referrers_args { struct visitor_args base; PyObject *objs; @@ -1435,8 +1457,12 @@ visit_get_referrers(const mi_heap_t *heap, const mi_heap_area_t *area, } struct get_referrers_args *arg = (struct get_referrers_args *)args; + if (op == arg->objs) { + // Don't include the tuple itself in the referrers list. + return true; + } if (Py_TYPE(op)->tp_traverse(op, referrersvisit, arg->objs)) { - if (_PyObjectStack_Push(&arg->results, op) < 0) { + if (_PyObjectStack_Push(&arg->results, Py_NewRef(op)) < 0) { return false; } } @@ -1447,11 +1473,6 @@ visit_get_referrers(const mi_heap_t *heap, const mi_heap_area_t *area, PyObject * _PyGC_GetReferrers(PyInterpreterState *interp, PyObject *objs) { - PyObject *result = PyList_New(0); - if (!result) { - 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. @@ -1460,23 +1481,12 @@ _PyGC_GetReferrers(PyInterpreterState *interp, PyObject *objs) int err = gc_visit_heaps(interp, &visit_get_referrers, &args.base); _PyEval_StartTheWorld(interp); + PyObject *list = list_from_object_stack(&args.results); if (err < 0) { PyErr_NoMemory(); - goto error; + Py_CLEAR(list); } - - PyObject *op; - while ((op = _PyObjectStack_Pop(&args.results)) != NULL) { - if (op != objs && PyList_Append(result, op) < 0) { - goto error; - } - } - return result; - -error: - Py_DECREF(result); - _PyObjectStack_Clear(&args.results); - return NULL; + return list; } struct get_objects_args { @@ -1499,7 +1509,7 @@ visit_get_objects(const mi_heap_t *heap, const mi_heap_area_t *area, } struct get_objects_args *arg = (struct get_objects_args *)args; - if (_PyObjectStack_Push(&arg->objects, op) < 0) { + if (_PyObjectStack_Push(&arg->objects, Py_NewRef(op)) < 0) { return false; } return true; @@ -1508,11 +1518,6 @@ visit_get_objects(const mi_heap_t *heap, const mi_heap_area_t *area, PyObject * _PyGC_GetObjects(PyInterpreterState *interp, int generation) { - PyObject *result = PyList_New(0); - if (!result) { - 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. @@ -1521,23 +1526,12 @@ _PyGC_GetObjects(PyInterpreterState *interp, int generation) int err = gc_visit_heaps(interp, &visit_get_objects, &args.base); _PyEval_StartTheWorld(interp); + PyObject *list = list_from_object_stack(&args.objects); if (err < 0) { PyErr_NoMemory(); - goto error; + Py_CLEAR(list); } - - PyObject *op; - while ((op = _PyObjectStack_Pop(&args.objects)) != NULL) { - if (op != result && PyList_Append(result, op) < 0) { - goto error; - } - } - return result; - -error: - Py_DECREF(result); - _PyObjectStack_Clear(&args.objects); - return NULL; + return list; } static bool