diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index 7eb104a6b58..c0be53795f0 100644 --- a/Lib/test/test_gc.py +++ b/Lib/test/test_gc.py @@ -580,6 +580,38 @@ class GCTests(unittest.TestCase): # would be damaged, with an empty __dict__. self.assertEqual(x, None) + def test_bug21435(self): + # This is a poor test - its only virtue is that it happened to + # segfault on Tim's Windows box before the patch for 21435 was + # applied. That's a nasty bug relying on specific pieces of cyclic + # trash appearing in exactly the right order in finalize_garbage()'s + # input list. + # But there's no reliable way to force that order from Python code, + # so over time chances are good this test won't really be testing much + # of anything anymore. Still, if it blows up, there's _some_ + # problem ;-) + gc.collect() + + class A: + pass + + class B: + def __init__(self, x): + self.x = x + + def __del__(self): + self.attr = None + + def do_work(): + a = A() + b = B(A()) + + a.attr = b + b.attr = a + + do_work() + gc.collect() # this blows up (bad C pointer) when it fails + @cpython_only def test_garbage_at_shutdown(self): import subprocess diff --git a/Misc/NEWS b/Misc/NEWS index 6fc0a13dc6e..04bf8545b9b 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -10,6 +10,10 @@ Release date: TBA Core and Builtins ----------------- +- Issue #21435: In rare cases, when running finalizers on objects in cyclic + trash a bad pointer dereference could occur due to a subtle flaw in + internal iteration logic. + - Issue #21233: Add new C functions: PyMem_RawCalloc(), PyMem_Calloc(), PyObject_Calloc(), _PyObject_GC_Calloc(). bytes(int) and bytearray(int) are now using ``calloc()`` instead of ``malloc()`` for large objects which diff --git a/Modules/gcmodule.c b/Modules/gcmodule.c index cff5d097aff..e1b693f9dc0 100644 --- a/Modules/gcmodule.c +++ b/Modules/gcmodule.c @@ -776,28 +776,40 @@ handle_legacy_finalizers(PyGC_Head *finalizers, PyGC_Head *old) return 0; } +/* Run first-time finalizers (if any) on all the objects in collectable. + * Note that this may remove some (or even all) of the objects from the + * list, due to refcounts falling to 0. + */ static void -finalize_garbage(PyGC_Head *collectable, PyGC_Head *old) +finalize_garbage(PyGC_Head *collectable) { destructor finalize; - PyGC_Head *gc = collectable->gc.gc_next; + PyGC_Head seen; - for (; gc != collectable; gc = gc->gc.gc_next) { + /* While we're going through the loop, `finalize(op)` may cause op, or + * other objects, to be reclaimed via refcounts falling to zero. So + * there's little we can rely on about the structure of the input + * `collectable` list across iterations. For safety, we always take the + * first object in that list and move it to a temporary `seen` list. + * If objects vanish from the `collectable` and `seen` lists we don't + * care. + */ + gc_list_init(&seen); + + while (!gc_list_is_empty(collectable)) { + PyGC_Head *gc = collectable->gc.gc_next; PyObject *op = FROM_GC(gc); - + gc_list_move(gc, &seen); if (!_PyGCHead_FINALIZED(gc) && - PyType_HasFeature(Py_TYPE(op), Py_TPFLAGS_HAVE_FINALIZE) && - (finalize = Py_TYPE(op)->tp_finalize) != NULL) { + PyType_HasFeature(Py_TYPE(op), Py_TPFLAGS_HAVE_FINALIZE) && + (finalize = Py_TYPE(op)->tp_finalize) != NULL) { _PyGCHead_SET_FINALIZED(gc, 1); Py_INCREF(op); finalize(op); - if (Py_REFCNT(op) == 1) { - /* op will be destroyed */ - gc = gc->gc.gc_prev; - } Py_DECREF(op); } } + gc_list_merge(&seen, collectable); } /* Walk the collectable list and check that they are really unreachable @@ -1006,7 +1018,7 @@ collect(int generation, Py_ssize_t *n_collected, Py_ssize_t *n_uncollectable, m += handle_weakrefs(&unreachable, old); /* Call tp_finalize on objects which have one. */ - finalize_garbage(&unreachable, old); + finalize_garbage(&unreachable); if (check_garbage(&unreachable)) { revive_garbage(&unreachable);