update_refs(): assert that incoming refcounts aren't 0. The comment

for this function has always claimed that was true, but it wasn't
verified before.  For the latest batch of "double deallocation" bugs
(stemming from weakref callbacks invoked by way of subtype_dealloc),
this assert would have triggered (instead of waiting for
_Py_ForgetReference to die with a segfault later).

diff --git a/Modules/gcmodule.c b/Modules/gcmodule.c
index 6442fe5452a24763138f69a30146191a52258289..e6aabe4b535977c3c38b446cc7bae4e53fcddd8a 100644 (file)
--- a/Modules/gcmodule.c
+++ b/Modules/gcmodule.c
@@ -214,6 +214,25 @@ update_refs(PyGC_Head *containers)
        for (; gc != containers; gc = gc->gc.gc_next) {
                assert(gc->gc.gc_refs == GC_REACHABLE);
                gc->gc.gc_refs = FROM_GC(gc)->ob_refcnt;
+               /* Python's cyclic gc should never see an incoming refcount
+                * of 0:  if something decref'ed to 0, it should have been
+                * deallocated immediately at that time.
+                * Possible cause (if the assert triggers):  a tp_dealloc
+                * routine left a gc-aware object tracked during its teardown
+                * phase, and did something-- or allowed something to happen --
+                * that called back into Python.  gc can trigger then, and may
+                * see the still-tracked dying object.  Before this assert
+                * was added, such mistakes went on to allow gc to try to
+                * delete the object again.  In a debug build, that caused
+                * a mysterious segfault, when _Py_ForgetReference tried
+                * to remove the object from the doubly-linked list of all
+                * objects a second time.  In a release build, an actual
+                * double deallocation occurred, which leads to corruption
+                * of the allocator's internal bookkeeping pointers.  That's
+                * so serious that maybe this should be a release-build
+                * check instead of an assert?
+                */
+               assert(gc->gc.gc_refs != 0);
        }
 }