map: Always hash the object in mp_map_lookup. #17723
Conversation
jepler
force-pushed
the
ordereddict-slice-crash
branch
from
b953be5 to
f9e60f8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17723 +/- ##
=======================================
Coverage 98.44% 98.44%
=======================================
Files 171 171
Lines 22208 22208
=======================================
Hits 21863 21863
Misses 345 345 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Code size report: |
|
The test fails on some platforms because Python 3.12 made slice objects hashable. >>> d = collections.OrderedDict()
>>> d[:] = "oops"
>>> d
OrderedDict({slice(None, None, None): 'oops'}) |
jepler
force-pushed
the
ordereddict-slice-crash
branch
from
f9e60f8 to
e4e97f5
Compare
|
I pulled the new test into a separate file with a .exp included so that it can run regardless of Python version. |
Fuzzing produced a crash in OrderedDict which reduced to the following: ``` from collections import OrderedDict x = OrderedDict() x[: 'a'] = 1 x['b'] = 2 ``` This occurs because, as an optimization, the key is not hashed when searching an OrderedDict, so an unhashable key can be stored. Even worse, the slice in this case is from build_slice_stack_allocated so by the time the 2nd indexing operation is being performed, it's nonsense instead of a Python object. The new test is in its own file because Python 3.12 actually made slice objects hashable so a .exp file is required. Signed-off-by: Jeff Epler <jepler@gmail.com>
jepler
force-pushed
the
ordereddict-slice-crash
branch
from
e4e97f5 to
cf1322f
Compare
This change will most likely impact performance quite a lot. That's because all built-in dicts (method tables and module dicts) have So we should at least measure the impact of this change on performance. And then probably need to find another way to fix this issue. |
|
That makes sense. Maybe the alternative is to add a specific check for slice objects. It's the (new?) temporary nature of the slice object which turns it from a "well don't do that then" bug into a crashing bug. |
Summary
Fuzzing produced a crash in OrderedDict which reduced to the following:
This occurs because, as an optimization, the key is not hashed when searching an OrderedDict, so an unhashable key can be stored. Even worse, the slice in this case is from
build_slice_stack_allocatedso by the time the 2nd indexing operation is being performed, it's nonsense instead of a Python object.Testing
I added a test for this and ran the testsuite locally.
Trade-offs and Alternatives
This might slightly lower the performance of OrderedDict objects because before this change they avoided hashing the keys at all. However, I'm not aware of a way to check whether an object is hashable short of actually hashing it.