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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:39 | admin | set | github: 60806 |
2012-12-10 16:02:23 | pitrou | set | messages:
+ msg177297 |
2012-12-10 10:13:13 | amaury.forgeotdarc | set | nosy:
+ amaury.forgeotdarc messages:
+ msg177272
|
2012-12-08 21:00:37 | pitrou | set | messages:
+ msg177184 |
2012-12-08 20:58:03 | pitrou | set | messages:
+ msg177183 |
2012-12-08 20:53:21 | eltoder | set | messages:
+ msg177182 |
2012-12-08 20:46:09 | pitrou | set | messages:
+ msg177181 |
2012-12-08 20:42:00 | eltoder | set | messages:
+ msg177180 |
2012-12-08 20:24:31 | pitrou | set | status: open -> closed resolution: fixed messages:
+ msg177179
stage: needs patch -> resolved |
2012-12-08 20:23:57 | python-dev | set | messages:
+ msg177178 |
2012-12-08 20:20:28 | python-dev | set | nosy:
+ python-dev messages:
+ msg177177
|
2012-12-05 17:20:57 | jcea | set | nosy:
+ jcea
|
2012-12-05 11:00:24 | asvetlov | set | nosy:
+ asvetlov
|
2012-12-04 18:07:15 | daniel.urban | set | nosy:
+ daniel.urban
|
2012-12-04 14:20:26 | eltoder | set | messages:
+ msg176914 |
2012-12-04 10:34:34 | pitrou | set | messages:
+ msg176892 |
2012-12-04 07:11:14 | christian.heimes | set | versions:
+ Python 3.4, - Python 2.6, Python 3.1 nosy:
+ christian.heimes
messages:
+ msg176875
stage: needs patch |
2012-12-04 06:50:28 | eltoder | create | |