classification
Title: weakref can return an object with 0 refcount
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.4, Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, asvetlov, christian.heimes, daniel.urban, eltoder, jcea, pitrou, python-dev
Priority: normal Keywords:

Created on 2012-12-04 06:50 by eltoder, last changed 2012-12-10 16:02 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
wr2.py eltoder, 2012-12-04 06:50 another test case
Messages (14)
msg176874 - (view) Author: Eugene Toder (eltoder) Date: 2012-12-04 06:50
An interaction between weakrefs and trashcan can cause weakref to return the object it's pointing to after object's refcount is already 0. Given that the object is usually increfed and decrefed, this leads to double dealloc and crashing or hanging.
Tested 2.6.6, 2.7.2 and 3.3.0 on win7.

In more details. The documentation for Py_TRASHCAN_SAFE_BEGIN tells to put it right after PyObject_GC_UnTrack. This means that PyObject_ClearWeakRefs goes after it, and, for example, in subtype_dealloc of Objects/typeobject.c this is indeed the case. This means that if we get into a long chain of deallocations and the trashcan kicks in, we get an object with 0 refcount and without cleared weakrefs lying in trash_delete_later until we go PyTrash_UNWIND_LEVEL levels up. During that time we can execute arbitrary python code, so all we need is some code with an access to the weakref to dereference it.
The current recommendation of clearing weakrefs before clearing attributes makes this less likely to happen, but it's still easy enough:

import weakref

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)

print('deleting')
del root
print('done')

In this case weakref callback in WeakKeyDictionary deletes the reference to the next object, causing the next level of destruction, until trashcan kicks in. Trashcan delays clearing of weakrefs, allowing the second weakref's (wparent) callback to see the dead object via wself that it captured.

Attached is a similar example with less weakrefs using __del__.
msg176875 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-12-04 07:11
Thank you very much for bringing the issue to our attention. I've removed 2.6 and 3.1 because they are in security fix mode and this issue poses no security threat.
msg176892 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-12-04 10:34
The easy fix would probably be to return Py_None when the weakref'ed object has a zero refcount.
msg176914 - (view) Author: Eugene Toder (eltoder) Date: 2012-12-04 14:20
Agreed. That's what I've put in my code as a work around.
msg177177 - (view) Author: Roundup Robot (python-dev) Date: 2012-12-08 20:20
New changeset 0748c22b37e5 by Antoine Pitrou in branch '3.2':
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.
http://hg.python.org/cpython/rev/0748c22b37e5

New changeset 259c1636c884 by Antoine Pitrou in branch '3.3':
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.
http://hg.python.org/cpython/rev/259c1636c884

New changeset 17e5acad302e by Antoine Pitrou in branch 'default':
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.
http://hg.python.org/cpython/rev/17e5acad302e
msg177178 - (view) Author: Roundup Robot (python-dev) Date: 2012-12-08 20:23
New changeset 7e771f0363e2 by Antoine Pitrou in branch '2.7':
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.
http://hg.python.org/cpython/rev/7e771f0363e2
msg177179 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-12-08 20:24
Thank you very much for reporting this issue. I believe it is now fixed.
msg177180 - (view) Author: Eugene Toder (eltoder) Date: 2012-12-08 20:42
Thank you, Antoine.
This change has one effect that's worth highlighting in NEWS at least -- the PyWeakref_GET_OBJECT() macro now evaluates its argument twice. This can break existing code where the argument has side-effects, e.g. o = PyWeakref_GET_OBJECT(p++). I found one such case in our code base, but I don't know how common this is. So this is something to watch out for when upgrading.
I don't think there's a way to write PyWeakref_GET_OBJECT() in standard C90 without double evaluation.
msg177181 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-12-08 20:46
> I don't think there's a way to write PyWeakref_GET_OBJECT() in standard 
> C90 without double evaluation.

Agreed. We generally don't document whether our macros are compatible with side effects in parameters, and I think we'd like to keep it that way. People should simply avoid doing this kind of thing, as it's knowingly fragile, and trivial to avoid anyway.
msg177182 - (view) Author: Eugene Toder (eltoder) Date: 2012-12-08 20:53
> People should simply avoid doing this kind of thing, as it's knowingly fragile, and trivial to avoid anyway.
Is this documented in the C API guide, or somewhere else?
In any case, notifying people so they can quickly check their code seems much nicer than having them figure this out the hard way.
msg177183 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-12-08 20:58
> > People should simply avoid doing this kind of thing, as it's
> > knowingly fragile, and trivial to avoid anyway.
> Is this documented in the C API guide, or somewhere else?

I don't think so, but it's common C wisdom that you shouldn't pass
arguments which have side effects to a macro, except if you are sure the
macro allows it.

> In any case, notifying people so they can quickly check their code
> seems much nicer than having them figure this out the hard way.

I see it as a double-edged sword: if we start adding a warning for this
macro, people will expect us to do it for every other macro, which we
aren't doing right now.
msg177184 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-12-08 21:00
> > In any case, notifying people so they can quickly check their code
> > seems much nicer than having them figure this out the hard way.
> 
> I see it as a double-edged sword: if we start adding a warning for this
> macro, people will expect us to do it for every other macro, which we
> aren't doing right now.

And also, as this issue shows, we may have to change it in a bugfix
release, which means people shouldn't trust the fact that there's no
such warning, anyway.
msg177272 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-12-10 10:13
PyWeakref_GET_OBJECT is also potentially dangerous: since the refcount is not incremented, it's very possible that the GC collects it.

The only safe operation after PyWeakref_GET_OBJECT is to Py_XINCREF the result. Should we provide a PyWeakRef_LockObject()?
msg177297 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-12-10 16:02
> The only safe operation after PyWeakref_GET_OBJECT is to Py_XINCREF the 
> result. Should we provide a PyWeakRef_LockObject()?

There's already a warning in the docs about that. I don't think an additional function is useful here.
History
Date User Action Args
2012-12-10 16:02:23pitrousetmessages: + msg177297
2012-12-10 10:13:13amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg177272
2012-12-08 21:00:37pitrousetmessages: + msg177184
2012-12-08 20:58:03pitrousetmessages: + msg177183
2012-12-08 20:53:21eltodersetmessages: + msg177182
2012-12-08 20:46:09pitrousetmessages: + msg177181
2012-12-08 20:42:00eltodersetmessages: + msg177180
2012-12-08 20:24:31pitrousetstatus: open -> closed
resolution: fixed
messages: + msg177179

stage: needs patch -> resolved
2012-12-08 20:23:57python-devsetmessages: + msg177178
2012-12-08 20:20:28python-devsetnosy: + python-dev
messages: + msg177177
2012-12-05 17:20:57jceasetnosy: + jcea
2012-12-05 11:00:24asvetlovsetnosy: + asvetlov
2012-12-04 18:07:15daniel.urbansetnosy: + daniel.urban
2012-12-04 14:20:26eltodersetmessages: + msg176914
2012-12-04 10:34:34pitrousetmessages: + msg176892
2012-12-04 07:11:14christian.heimessetversions: + Python 3.4, - Python 2.6, Python 3.1
nosy: + christian.heimes

messages: + msg176875

stage: needs patch
2012-12-04 06:50:28eltodercreate