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

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

I'll backport this to 2.3 maint.

2 files changed, 38 insertions, 6 deletions

diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py
index 9149318..093133e 100644
--- 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 bdbabf4..032fa18 100644
--- 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 */
@@ -689,6 +691,7 @@ subtype_dealloc(PyObject *self)
 	}
 
 	/* Finalize GC if the base doesn't do GC and we do */
+	_PyObject_GC_TRACK(self);
 	if (!PyType_IS_GC(base))
 		_PyObject_GC_UNTRACK(self);
 
@@ -730,6 +733,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?