From add09b4149b17afa30aa042dbb718b9d9ec741c3 Mon Sep 17 00:00:00 2001 From: Tim Peters Date: Wed, 12 Nov 2003 20:43:28 +0000 Subject: SF bug 840829: weakref callbacks and gc corrupt memory. subtype_dealloc(): This left the dying object exposed to gc, so that if cyclic gc triggered during the weakref callback, gc tried to delete the dying object a second time. That's a disaster. subtype_dealloc() had a (I hope!) unique problem here, as every normal dealloc routine untracks the object (from gc) before fiddling with weakrefs etc. But subtype_dealloc has obscure technical reasons for re-registering the dying object with gc (already explained in a large comment block at the bottom of the function). The fix amounts to simply refraining from reregistering the dying object with gc until after the weakref callback (if any) has been called. This is a critical bug (hard to predict, and causes seemingly random memory corruption when it occurs). I'll backport it to 2.3 later. --- Lib/test/test_weakref.py | 20 ++++++++++++++++++++ Misc/NEWS | 6 ++++++ Objects/typeobject.c | 7 ++++++- 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py index 35ab77f..9149318 100644 --- a/Lib/test/test_weakref.py +++ b/Lib/test/test_weakref.py @@ -298,6 +298,26 @@ class ReferencesTestCase(TestBase): else: self.fail("exception not properly restored") + def test_sf_bug_840829(self): + # "weakref callbacks and gc corrupt memory" + # subtype_dealloc erroneously exposed a new-style instance + # already in the process of getting deallocated to gc, + # causing double-deallocation if the instance had a weakref + # callback that triggered gc. + # If the bug exists, there probably won't be an obvious symptom + # in a release build. In a debug build, a segfault will occur + # when the second attempt to remove the instance from the "list + # of all objects" occurs. + + import gc + + class C(object): + pass + + c = C() + wr = weakref.ref(c, lambda ignore: gc.collect()) + del c + class Object: def __init__(self, arg): diff --git a/Misc/NEWS b/Misc/NEWS index ff1e426..17e7098 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -12,6 +12,12 @@ What's New in Python 2.4 alpha 1? Core and builtins ----------------- +- Critical bugfix, for SF bug 840829: if cyclic garbage collection + happened to occur during a weakref callback for a new-style class + instance, subtle memory corruption was the result (in a release build; + in a debug build, a segfault occurred reliably very soon after). + This has been repaired. + - Added a reversed() builtin function that returns a reverse iterator over a sequence. diff --git a/Objects/typeobject.c b/Objects/typeobject.c index b1c822f..bdbabf4 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -639,7 +639,11 @@ subtype_dealloc(PyObject *self) ++_PyTrash_delete_nesting; Py_TRASHCAN_SAFE_BEGIN(self); --_PyTrash_delete_nesting; - _PyObject_GC_TRACK(self); /* We'll untrack for real later */ + /* DO NOT restore GC tracking at this point. The weakref callback + * (if any) may trigger GC, and if self is tracked at that point, + * it will look like trash to GC and GC will try to delete it + * again. Double-deallocation is a subtle disaster. + */ /* Find the nearest base with a different tp_dealloc */ base = type; @@ -654,6 +658,7 @@ subtype_dealloc(PyObject *self) if (type->tp_weaklistoffset && !base->tp_weaklistoffset) PyObject_ClearWeakRefs(self); + _PyObject_GC_TRACK(self); /* We'll untrack for real later */ /* Maybe call finalizer; exit early if resurrected */ if (type->tp_del) { -- cgit v0.12