From 62a0d6ea402d18f528629c6e84ed6f46e7c0912a Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Sat, 8 Dec 2012 21:15:26 +0100 Subject: [PATCH] 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. Thanks to Eugene Toder for diagnosing and reporting the issue. --- Include/weakrefobject.h | 12 +++++++++++- Lib/test/test_weakref.py | 21 +++++++++++++++++++++ Misc/ACKS | 1 + Misc/NEWS | 4 ++++ Objects/weakrefobject.c | 5 ++--- 5 files changed, 39 insertions(+), 4 deletions(-) diff --git a/Include/weakrefobject.h b/Include/weakrefobject.h index 725838726de..537e7ebf287 100644 --- a/Include/weakrefobject.h +++ b/Include/weakrefobject.h @@ -70,7 +70,17 @@ PyAPI_FUNC(Py_ssize_t) _PyWeakref_GetWeakrefCount(PyWeakReference *head); PyAPI_FUNC(void) _PyWeakref_ClearRef(PyWeakReference *self); #endif -#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 92a47133cbc..571e33f492d 100644 --- a/Lib/test/test_weakref.py +++ b/Lib/test/test_weakref.py @@ -776,6 +776,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: + 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 9e662f6a731..4500d1a6f0c 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -1067,6 +1067,7 @@ Jim Tittsler Frank J. Tobin R Lindsay Todd Bennett Todd +Eugene Toder Erik Tollerud Matias Torchinsky Sandro Tosi diff --git a/Misc/NEWS b/Misc/NEWS index 3c9f382fe3f..5152c03fc8c 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -10,6 +10,10 @@ What's New in Python 3.2.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 #16416: On Mac OS X, operating system data are now always encoded/decoded to/from UTF-8/surrogateescape, instead of the locale encoding (which may be ASCII if no locale environment variable is set), to avoid diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index dae3c24f4a3..8d571e65d16 100644 --- 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)