Repository navigation
Conversation
seperman
left a comment
There was a problem hiding this comment.
Static review: #639 Avoid numeric hash-cache collisions with user tuples
Summary: Since 9.0.0 (6d750d4), DeepHash stores numbers in its cache under the key (type(obj), obj). That key equals an ordinary user tuple such as (int, 1), and also (int, 1.0) because 1 == 1.0. Whichever of the two is hashed second gets the other's cached hash, so ignore_order=True hides the difference. The PR prefixes numeric keys with a private class marker, giving (_NumberHashKey, type, value), and the public accessors unwrap only keys that carry the marker. Reading the code, this fixes the collision. Hash values for ordinary inputs stay the same; only the internal key shape changes.
Overlap with other PRs
- This follows up on the cache-key design from 6d750d4 (#485, int vs. float).
- #580 (closed) took a different, diff-side approach to #485 and is unrelated to this collision. #607 (merged) only changes how
diff.pyhandles ordered iterables. - No other open PR touches
_make_hash_keyor_unwrap_hash_key. The PR merges cleanly with #641 and #642.
Security
deepdiff/serialization.py:94adds'deepdiff.deephash._NumberHashKey'toSAFE_TO_IMPORT. I consider this low risk:- The class (
deepdiff/deephash.py:103) is empty: no__reduce__,__setstate__,__getattr__or__del__. - Unpickling it gives either the class itself or an empty instance whose attributes BUILD can set. No code runs.
_RestrictedUnpickler.find_classchecks the exactmodule.namestring and resolves it with a plaingetattr, so this entry does not allow dotted-name traversal.
- The class (
- Why the entry is there: without it,
pickle_load(pickle_dump(dh.hashes))would stop working. The old keys only referencedbuiltins.int,builtins.floatand so on, which are already allowlisted. I found no deepdiff code that picklesDeepHash.hashesitself: Delta does not, and the multiprocessing workers use stdlib pickle and do not send hash dicts back. So the entry mainly keeps that round-trip working for users, and for the newrestricted_pickletest. The maintainer may still prefer to keep the allowlist minimal. - No other concerns. The tests use
pickle.loads(pickle.dumps(...))only on data they build themselves.
Correctness
-
deepdiff/deephash.py:630and:637change together, so cache writes in_hash,__contains__, and_getitem/get/get_keyall use the same key shape. -
_unwrap_hash_key(:388-391) now unwraps only marked keys. As a side effect,keys()anditems()no longer turn a genuine user tuple(int, 5)into5. -
The marker is a class rather than an
object()sentinel, so its identity survivesdeepcopyand pickle (classes are pickled by reference). This matters when a cache is reused throughhashes=. -
Compatibility: the shape of raw
.hasheskeys changes. Suppose a cache built by 9.0/9.1 is passed back throughhashes=:- Numbers miss the cache and are recomputed, which is correct.
- The old
(int, 1)entries are now found by a user tuple(int, 1), which brings the old collision back for that run.
The description says such dicts should be rebuilt; a CHANGELOG note would make that more visible.
-
Performance: each numeric key gets one more tuple element, and the unwrap check is cheaper than before. The difference is negligible.
Tests
- Reading the code, the new cases should fail on
dev:- In forward order, the tuple finds the number's cached
(int, 1)entry. - In reverse order, the tuple's own entry replaces the cache entry of the
1inside it, so the later lookup of1returns the tuple's hash. test_numeric_cache_does_not_contain_unhashed_tuplefails through__contains__.
- In forward order, the tuple finds the number's cached
- The
restricted_picklecase exercises the new allowlist entry. - Nit:
tests/test_hash.py:411callsnp.int64(1)inside theparametrizearguments, so it runs when tests are collected. Without numpy,npisNoneand collection would error instead of skipping. This is fine with the test extras installed.
Suggestions
- Add a CHANGELOG entry covering the fix and the change in raw
.hasheskey shape.
AI-assisted static review; the code was not executed.
|
Thanks for the review. Added an Unreleased CHANGELOG entry in 79a6d14 covering the collision fix, the raw Re-ran the full suite including slow tests on Python 3.12.14: 1,329 passed, 8 skipped; the CI-blocking Flake8 checks and Pyright on both affected production files also passed. Hosted CI for this commit is awaiting maintainer approval. |
Problem
The typed numeric cache key for
1is currently the ordinary tuple(int, 1). A user-provided tuple with that value therefore aliases the cached number:The reverse comparison is also missed. DeepHash lookups and public key iteration can conflate the same distinct objects.
Fix
The change is limited to
deephash.py, one safe-import entry for its inert marker inserialization.py, and regression tests. It does not change the serialized hash content of ordinary numeric inputs or numeric-equivalence behavior withignore_numeric_type_changes=True. Ordinary tuple keys now remain intact in public key iteration with either flag value.Validation
devand pass after the fix.pytest --cov=deepdiff --cov-report=term-missing --runslow --benchmark-disable: 1,329 passed, 8 skipped, 95% overall coverage (98% for deephash.py).git diff --check: passed. Full touched-file lint introduces no new diagnostics.Compatibility note
Numeric entries in raw
.hashesdictionaries use a new key shape. Such dictionaries retained from older versions should be rebuilt. Public object-based lookup remains unchanged.