Skip to content

gh-132461: Fix crash in OrderedDict.setdefault when key has unstable hash #132462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+23 −2
Open

gh-132461: Fix crash in OrderedDict.setdefault when key has unstable hash #132462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08759ed
Fix crash in OrderedDict.setdefault when key has unstable hash
dura0ok Apr 13, 2025
0a6eda3
test_ordered_dict: Add test for unstable hash behavior
dura0ok Apr 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion Lib/test/test_ordered_dict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import abc
import builtins
import contextlib
import copy
import gc
import operator
import pickle
import re
from random import randrange, shuffle
from random import randrange, shuffle, randint
import struct
import sys
import unittest
Expand Down Expand Up @@ -729,6 +730,22 @@ def test_merge_operator(self):
with self.assertRaises(ValueError):
a |= "BAD"

def test_unstable_hash_should_not_abort_issue132461(self):
# OrderedDict should raise, not abort, when used with unstable-hash keys
large_num = 2**64

class WeirdBase(abc.ABCMeta):
def __hash__(self):
return randint(0, large_num)

class weird_bytes(bytes, metaclass=WeirdBase):
pass

obj = self.OrderedDict()
with self.assertRaises(Exception):
for x in range(100):
obj.setdefault(weird_bytes, None)
Comment on lines +735 to +747
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
large_num = 2**64
class WeirdBase(abc.ABCMeta):
def __hash__(self):
return randint(0, large_num)
class weird_bytes(bytes, metaclass=WeirdBase):
pass
obj = self.OrderedDict()
with self.assertRaises(Exception):
for x in range(100):
obj.setdefault(weird_bytes, None)
class A:
c = 0
def __hash__(self):
self.c += 1
return self.c
a = A()
obj = self.OrderedDict()
msg = "OrderedDict key appears to change its hash value over time"
with self.assertRaisesRegex(RuntimeError, msg):
for _ in range(100):
obj.setdefault(a, None)

We can also remove abc dependency and randint import.

Copy link
Contributor Author

@dura0ok dura0ok Apr 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

part of test

        with self.assertRaises(KeyError):
            for x in range(100):
                obj.setdefault(weird_bytes, None)

error

ERROR: test_unstable_hash_should_not_abort_issue132461 (test.test_ordered_dict.CPythonOrderedDictTests.test_unstable_hash_should_not_abort_issue132461)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/learning/cpython/Lib/test/test_ordered_dict.py", line 747, in test_unstable_hash_should_not_abort_issue132461
    obj.setdefault(weird_bytes, None)
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
RuntimeError: OrderedDict key appears to change its hash value over time

fixed part of test

        with self.assertRaises(RuntimeError):
            for x in range(100):
                obj.setdefault(weird_bytes, None)

ERROR: test_unstable_hash_should_not_abort_issue132461 (test.test_ordered_dict.CPythonOrderedDictSubclassTests.test_unstable_hash_should_not_abort_issue132461)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/learning/cpython/Lib/test/test_ordered_dict.py", line 747, in test_unstable_hash_should_not_abort_issue132461
    obj.setdefault(weird_bytes, None)
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
KeyError: <class 'test.test_ordered_dict.OrderedDictTests.test_unstable_hash_should_not_abort_issue132461.<locals>.weird_bytes'>

======================================================================
ERROR: test_unstable_hash_should_not_abort_issue132461 (test.test_ordered_dict.PurePythonOrderedDictSubclassTests.test_unstable_hash_should_not_abort_issue132461)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/learning/cpython/Lib/test/test_ordered_dict.py", line 747, in test_unstable_hash_should_not_abort_issue132461
    obj.setdefault(weird_bytes, None)
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
  File "/learning/cpython/Lib/collections/__init__.py", line 274, in setdefault
    return self[key]
           ~~~~^^^^^
KeyError: <class 'test.test_ordered_dict.OrderedDictTests.test_unstable_hash_should_not_abort_issue132461.<locals>.weird_bytes'>

======================================================================
ERROR: test_unstable_hash_should_not_abort_issue132461 (test.test_ordered_dict.PurePythonOrderedDictTests.test_unstable_hash_should_not_abort_issue132461)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/learning/cpython/Lib/test/test_ordered_dict.py", line 747, in test_unstable_hash_should_not_abort_issue132461
    obj.setdefault(weird_bytes, None)
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
  File "/learning/cpython/Lib/collections/__init__.py", line 274, in setdefault
    return self[key]
           ~~~~^^^^^
KeyError: <class 'test.test_ordered_dict.OrderedDictTests.test_unstable_hash_should_not_abort_issue132461.<locals>.weird_bytes'>

Very strange

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, we should only catch RuntimeError and not arbitrary exceptions. The assertion failing may be an indication that something else is wrong.


@support.cpython_only
def test_ordered_dict_items_result_gc(self):
# bpo-42536: OrderedDict.items's tuple-reuse speed trick breaks the GC's
Expand Down
6 changes: 5 additions & 1 deletion Objects/odictobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,7 +1033,11 @@ OrderedDict_setdefault_impl(PyODictObject *self, PyObject *key,
if (result == NULL) {
if (PyErr_Occurred())
return NULL;
assert(_odict_find_node(self, key) == NULL);
if (_odict_find_node(self, key) != NULL) {
PyErr_SetString(PyExc_RuntimeError,
"OrderedDict key appears to change its hash value over time");
return NULL;
}
if (PyODict_SetItem((PyObject *)self, key, default_value) >= 0) {
result = Py_NewRef(default_value);
}
Expand Down
Loading