subtype_dealloc(): A more complete fix for critical bug 840829 +

expanded the test case with a piece that needs the more-complete fix.

I don't intend to backport this beyond 2.3 maint.  It's a critical
bugfix, and should be backported to 2.2, 2.1, ..., if more releases in
those lines get made.

diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py
index 9149318081f2cc18311f019f32a89d0cd5965cc3..093133eb67b8c5d4c88f1250c4cf654bd1c2301d 100644 (file)
--- a/Lib/test/test_weakref.py
+++ b/Lib/test/test_weakref.py
@@ -318,6 +318,25 @@ class ReferencesTestCase(TestBase):
         wr = weakref.ref(c, lambda ignore: gc.collect())
         del c
 
+        # There endeth the first part.  It gets worse.
+        del wr
+
+        c1 = C()
+        c1.i = C()
+        wr = weakref.ref(c1.i, lambda ignore: gc.collect())
+
+        c2 = C()
+        c2.c1 = c1
+        del c1  # still alive because c2 points to it
+
+        # Now when subtype_dealloc gets called on c2, it's not enough just
+        # that c2 is immune from gc while the weakref callbacks associated
+        # with c2 execute (there are none in this 2nd half of the test, btw).
+        # subtype_dealloc goes on to call the base classes' deallocs too,
+        # so any gc triggered by weakref callbacks associated with anything
+        # torn down by a base class dealloc can also trigger double
+        # deallocation of c2.
+        del c2
 
 class Object:
     def __init__(self, arg):

diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index dbd2bc8db8e5ca85e966552a01fcc17da13fba71..6c649433240e9a85f69b0c77af7fcc7e141d4a05 100644 (file)
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -639,10 +639,10 @@ subtype_dealloc(PyObject *self)
        ++_PyTrash_delete_nesting;
        Py_TRASHCAN_SAFE_BEGIN(self);
        --_PyTrash_delete_nesting;
-       /* 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.
+       /* DO NOT restore GC tracking at this point.  weakref callbacks
+        * (if any, and whether directly here or indirectly in something we
+        * call) may trigger GC, and if self is tracked at that point, it
+        * will look like trash to GC and GC will try to delete self again.
         */
 
        /* Find the nearest base with a different tp_dealloc */
@@ -658,13 +658,15 @@ 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) {
+               _PyObject_GC_TRACK(self);
                type->tp_del(self);
                if (self->ob_refcnt > 0)
-                       goto endlabel;
+                       goto endlabel;  /* resurrected */
+               else
+                       _PyObject_GC_UNTRACK(self);
        }
 
        /*  Clear slots up to the nearest base with a different tp_dealloc */
@@ -688,11 +690,11 @@ subtype_dealloc(PyObject *self)
                }
        }
 
-       /* Finalize GC if the base doesn't do GC and we do */
-       if (!PyType_IS_GC(base))
-               _PyObject_GC_UNTRACK(self);
-
-       /* Call the base tp_dealloc() */
+       /* Call the base tp_dealloc(); first retrack self if
+        * basedealloc knows about gc.
+        */
+       if (PyType_IS_GC(base))
+               _PyObject_GC_TRACK(self);
        assert(basedealloc);
        basedealloc(self);
 
@@ -730,6 +732,16 @@ subtype_dealloc(PyObject *self)
                 trashcan begin
                 GC track
 
+           Q. Why did the last question say "immediately GC-track again"?
+              It's nowhere near immediately.
+
+           A. Because the code *used* to re-track immediately.  Bad Idea.
+              self has a refcount of 0, and if gc ever gets its hands on it
+              (which can happen if any weakref callback gets invoked), it
+              looks like trash to gc too, and gc also tries to delete self
+              then.  But we're already deleting self.  Double dealloction is
+              a subtle disaster.
+
           Q. Why the bizarre (net-zero) manipulation of
              _PyTrash_delete_nesting around the trashcan macros?