Issue #16602: When a weakref's target was part of a long deallocation chain, the...

Thanks to Eugene Toder for diagnosing and reporting the issue.

diff --git a/Include/weakrefobject.h b/Include/weakrefobject.h
index f15c9d9c128365866ae725bbc766f52bf9cfcb2e..e46aecf38587f6361d1d890d326591cb27e6a577 100644 (file)
--- a/Include/weakrefobject.h
+++ b/Include/weakrefobject.h
@@ -66,7 +66,17 @@ PyAPI_FUNC(Py_ssize_t) _PyWeakref_GetWeakrefCount(PyWeakReference *head);
 
 PyAPI_FUNC(void) _PyWeakref_ClearRef(PyWeakReference *self);
 
-#define PyWeakref_GET_OBJECT(ref) (((PyWeakReference *)(ref))->wr_object)
+/* Explanation for the Py_REFCNT() check: when a weakref's target is part
+   of a long chain of deallocations which triggers the trashcan mechanism,
+   clearing the weakrefs can be delayed long after the target's refcount
+   has dropped to zero.  In the meantime, code accessing the weakref will
+   be able to "see" the target object even though it is supposed to be
+   unreachable.  See issue #16602. */
+
+#define PyWeakref_GET_OBJECT(ref)                           \
+    (Py_REFCNT(((PyWeakReference *)(ref))->wr_object) > 0   \
+     ? ((PyWeakReference *)(ref))->wr_object                \
+     : Py_None)
 
 
 #ifdef __cplusplus

diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py
index b7c72bd5e1b063409124f65e4a09d862c295d1e6..3d86cb7eeb565e8c1ab95f93f7e833b12381fd2b 100644 (file)
--- a/Lib/test/test_weakref.py
+++ b/Lib/test/test_weakref.py
@@ -774,6 +774,27 @@ class ReferencesTestCase(TestBase):
         self.assertEqual(hash(a), hash(42))
         self.assertRaises(TypeError, hash, b)
 
+    def test_trashcan_16602(self):
+        # Issue #16602: when a weakref's target was part of a long
+        # deallocation chain, the trashcan mechanism could delay clearing
+        # of the weakref and make the target object visible from outside
+        # code even though its refcount had dropped to 0.  A crash ensued.
+        class C(object):
+            def __init__(self, parent):
+                if not parent:
+                    return
+                wself = weakref.ref(self)
+                def cb(wparent):
+                    o = wself()
+                self.wparent = weakref.ref(parent, cb)
+
+        d = weakref.WeakKeyDictionary()
+        root = c = C(None)
+        for n in range(100):
+            d[c] = c = C(c)
+        del root
+        gc.collect()
+
 
 class SubclassableWeakrefTestCase(TestBase):
 

diff --git a/Misc/ACKS b/Misc/ACKS
index 0a7c8bd2e215ad441f6a01b7c21132306a11a964..5e4c03a04e82fdd4c742b4d1fb8a72ff35c793f8 100644 (file)
--- a/Misc/ACKS
+++ b/Misc/ACKS
@@ -990,6 +990,7 @@ Jim Tittsler
 Frank J. Tobin
 R Lindsay Todd
 Bennett Todd
+Eugene Toder
 Matias Torchinsky
 Sandro Tosi
 Richard Townsend

diff --git a/Misc/NEWS b/Misc/NEWS
index 0c01e8b6ab8e924d475902fcc6040b2827c3d33b..9934212f6c09bbad35b27de82dd82500d08dc292 100644 (file)
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -9,6 +9,10 @@ What's New in Python 2.7.4
 Core and Builtins
 -----------------
 
+- Issue #16602: When a weakref's target was part of a long deallocation
+  chain, the object could remain reachable through its weakref even though
+  its refcount had dropped to zero.
+
 - Issue #9011: Fix hacky AST code that modified the CST when compiling
   a negated numeric literal.
 

diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c
index 7b943b0f00491bde7a2a88a2d6802d92d59dd5f5..99bf42bfece828ff6b0c37f5d104e21988a17640 100644 (file)
--- a/Objects/weakrefobject.c
+++ b/Objects/weakrefobject.c
@@ -52,9 +52,8 @@ clear_weakref(PyWeakReference *self)
 {
     PyObject *callback = self->wr_callback;
 
-    if (PyWeakref_GET_OBJECT(self) != Py_None) {
-        PyWeakReference **list = GET_WEAKREFS_LISTPTR(
-            PyWeakref_GET_OBJECT(self));
+    if (self->wr_object != Py_None) {
+        PyWeakReference **list = GET_WEAKREFS_LISTPTR(self->wr_object);
 
         if (*list == self)
             /* If 'self' is the end of the list (and thus self->wr_next == NULL)