This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: lookdict should INCREF/DECREF startkey around PyObject_RichCompareBool
Type: Stage:
Components: Interpreter Core Versions: Python 2.4, Python 2.3, Python 2.2.3, Python 2.6, Python 2.2.2, Python 2.5, Python 2.2.1, Python 2.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Rhamphoryncus, christian.heimes, gvanrossum
Priority: normal Keywords: patch

Created on 2007-11-29 01:41 by Rhamphoryncus, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
dictbug.py Rhamphoryncus, 2007-11-29 01:41
fix1517.diff gvanrossum, 2007-11-29 18:22
Messages (6)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2007-11-29 18:26
Committed revision 59222 (2.5.2).
Committed revision 59223 (2.6).

Thanks rhamporyncus and jorendorff!!
History
Date User Action Args
2022-04-11 14:56:28adminsetgithub: 45858
2007-11-29 18:26:19gvanrossumsetstatus: open -> closed
resolution: fixed
messages: + msg57945
keywords: + patch
2007-11-29 18:24:01christian.heimessetmessages: + msg57944
2007-11-29 18:22:41gvanrossumsetfiles: - fix1517.diff
2007-11-29 18:22:36gvanrossumsetfiles: + fix1517.diff
messages: + msg57943
2007-11-29 18:19:33gvanrossumsetfiles: + 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:44christian.heimessetnosy: + christian.heimes
messages: + msg57928
2007-11-29 01:41:10Rhamphoryncuscreate