From: Tim Peters <tim.peters@gmail.com>
Date: Thu, 13 Nov 2003 22:48:42 +0000 (+0000)
Subject: subtype_dealloc():  A more complete fix for critical bug 840829 +
X-Git-Tag: v2.3.3c1~62
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=8e81b7efad82ee2d9f4ad84d5e8ce51a0abb9fa2;p=thirdparty%2FPython%2Fcpython.git

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 9149318081f2..093133eb67b8 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 dbd2bc8db8e5..6c649433240e 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 */
@@ -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?