Issue1517
Created on 2007-11-29 01:41 by Rhamphoryncus, last changed 2007-11-29 18:26 by gvanrossum.
| File name |
Uploaded |
Description |
Edit |
Remove |
|
dictbug.py
|
Rhamphoryncus,
2007-11-29 01:41
|
|
|
|
|
fix1517.diff
|
gvanrossum,
2007-11-29 18:22
|
|
|
|
| msg57925 (view) |
Author: Adam Olsen (Rhamphoryncus) |
Date: 2007-11-29 01:41 |
|
(thanks go to my partner in crime, jorendorff, for helping flesh this out.)
lookdict calls PyObject_RichCompareBool without using INCREF/DECREF on
the key passed. It's possible for the comparison to delete the key from
the dict, causing its own argument to be deallocated. This can lead to
bogus results or a crash.
A custom type with its own __eq__ method will implicitly INCREF the key
when passing it as an argument, which prevents incorrect behaviour from
manifesting. There are a couple ways around this though, as shown in my
attachment.
|
| msg57928 (view) |
Author: Christian Heimes (christian.heimes) |
Date: 2007-11-29 10:49 |
|
Two questions:
* Which versions of Python are vulnerable to the problem? You forgot to
fill in the version list.
* How do we fix the problem? Is it enough to incref the key or must
startkey be protected, too?
|
| msg57942 (view) |
Author: Guido van Rossum (gvanrossum) |
Date: 2007-11-29 18:19 |
|
I can reproduce the segfault in 2.2 through 2.4; in 2.5 and 2.6 the
output is this instead:
Test 1, using __eq__(a, b).__nonzero__()
this is never the right answer
*****
Test 2, using tuple's tp_richcompare
New Watch 0xf7f8cbac
New Watch 0xf7f8cc0c
Deleting Watch 0xf7f8cbac
Deleting Watch 0xf7f8cbac
Deleting Watch 0xf7f8cc0c
Traceback (most recent call last):
File "/tmp/db3.py", line 72, in <module>
print(d[(Bar(), Watch())])
TypeError: __eq__() takes exactly 1 argument (2 given)
which suggests it's still there ("this is never the right answer").
In 3.0 the output from the 1st test is "this is an acceptable answer"
suggesting it's no longer there; but I suspect it's there in 3.0 as well
but due to the unicode transition the dict code there is different
enough that the example doesn't trigger it.
The key that needs to be INCREF'ed is actually startkey. Patch attached.
|
| msg57943 (view) |
Author: Guido van Rossum (gvanrossum) |
Date: 2007-11-29 18:22 |
|
Oops, the same code appeared twice. The new fix fixes both places.
|
| msg57944 (view) |
Author: Christian Heimes (christian.heimes) |
Date: 2007-11-29 18:24 |
|
Guido van Rossum wrote:
> The key that needs to be INCREF'ed is actually startkey. Patch attached.
What about the second PyObject_RichCompareBool(startkey, key, Py_EQ) a
few lines below inside the for loop?
Christian
|
| msg57945 (view) |
Author: Guido van Rossum (gvanrossum) |
Date: 2007-11-29 18:26 |
|
Committed revision 59222 (2.5.2).
Committed revision 59223 (2.6).
Thanks rhamporyncus and jorendorff!!
|
|
| Date |
User |
Action |
Args |
| 2007-11-29 18:26:19 | gvanrossum | set | status: open -> closed resolution: fixed messages:
+ msg57945 keywords:
+ patch |
| 2007-11-29 18:24:01 | christian.heimes | set | messages:
+ msg57944 |
| 2007-11-29 18:22:41 | gvanrossum | set | files:
- fix1517.diff |
| 2007-11-29 18:22:36 | gvanrossum | set | files:
+ fix1517.diff messages:
+ msg57943 |
| 2007-11-29 18:19:33 | gvanrossum | set | files:
+ fix1517.diff nosy:
+ gvanrossum messages:
+ msg57942 versions:
+ Python 2.6, Python 2.5, Python 2.4, Python 2.3, Python 2.2.3, Python 2.2.2, Python 2.2.1, Python 2.2 |
| 2007-11-29 10:49:44 | christian.heimes | set | nosy:
+ christian.heimes messages:
+ msg57928 |
| 2007-11-29 01:41:10 | Rhamphoryncus | create | |
|