Skip to content

Avoid numeric hash-cache collisions with user tuples - #639

Open
FanWu-ai wants to merge 2 commits into
qlustered:devfrom
FanWu-ai:fix/numeric-hash-cache-collision
Open

FanWu-ai wants to merge 2 commits into
qlustered:devfrom
FanWu-ai:fix/numeric-hash-cache-collision

Conversation

@FanWu-ai

@FanWu-ai FanWu-ai commented Oct 4, 2026

Copy link
Copy Markdown

Problem

The typed numeric cache key for 1 is currently the ordinary tuple (int, 1). A user-provided tuple with that value therefore aliases the cached number:

DeepDiff([1], [(int, 1)], ignore_order=True)
# Before: {}
# After: {'type_changes': {'root[0]': {
#     'old_type': int, 'new_type': tuple,
#     'old_value': 1, 'new_value': (int, 1)}}}

The reverse comparison is also missed. DeepHash lookups and public key iteration can conflate the same distinct objects.

Fix

  • Prefix numeric cache keys with a private class marker, keeping the numeric type/value distinction while separating ordinary user tuples.
  • Unwrap only marked keys in the public accessors.
  • Use a class marker so cache keys retain their identity through pickle and deepcopy.

The change is limited to deephash.py, one safe-import entry for its inert marker in serialization.py, and regression tests. It does not change the serialized hash content of ordinary numeric inputs or numeric-equivalence behavior with ignore_numeric_type_changes=True. Ordinary tuple keys now remain intact in public key iteration with either flag value.

Validation

  • All 20 new regression cases fail on unchanged dev and pass after the fix.
  • Covers int, float, complex, Decimal and NumPy numbers; both insertion/comparison directions; public accessors; shared cache reuse; standard and restricted pickle/deepcopy.
  • Linux CPython 3.12.14: pytest --cov=deepdiff --cov-report=term-missing --runslow --benchmark-disable: 1,329 passed, 8 skipped, 95% overall coverage (98% for deephash.py).
  • Pyright for both changed production files: no errors or warnings.
  • CI-blocking Flake8 rules and git diff --check: passed. Full touched-file lint introduces no new diagnostics.

Compatibility note

Numeric entries in raw .hashes dictionaries use a new key shape. Such dictionaries retained from older versions should be rebuilt. Public object-based lookup remains unchanged.

@seperman seperman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.py handles ordered iterables.
  • No other open PR touches _make_hash_key or _unwrap_hash_key. The PR merges cleanly with #641 and #642.

Security

  • deepdiff/serialization.py:94 adds 'deepdiff.deephash._NumberHashKey' to SAFE_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_class checks the exact module.name string and resolves it with a plain getattr, so this entry does not allow dotted-name traversal.
  • Why the entry is there: without it, pickle_load(pickle_dump(dh.hashes)) would stop working. The old keys only referenced builtins.int, builtins.float and so on, which are already allowlisted. I found no deepdiff code that pickles DeepHash.hashes itself: 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 new restricted_pickle test. 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:630 and :637 change together, so cache writes in _hash, __contains__, and _getitem/get/get_key all use the same key shape.

  • _unwrap_hash_key (:388-391) now unwraps only marked keys. As a side effect, keys() and items() no longer turn a genuine user tuple (int, 5) into 5.

  • The marker is a class rather than an object() sentinel, so its identity survives deepcopy and pickle (classes are pickled by reference). This matters when a cache is reused through hashes=.

  • Compatibility: the shape of raw .hashes keys changes. Suppose a cache built by 9.0/9.1 is passed back through hashes=:

    • 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 1 inside it, so the later lookup of 1 returns the tuple's hash.
    • test_numeric_cache_does_not_contain_unhashed_tuple fails through __contains__.
  • The restricted_pickle case exercises the new allowlist entry.
  • Nit: tests/test_hash.py:411 calls np.int64(1) inside the parametrize arguments, so it runs when tests are collected. Without numpy, np is None and 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 .hashes key shape.

AI-assisted static review; the code was not executed.

@FanWu-ai

Copy link
Copy Markdown
Author

Thanks for the review. Added an Unreleased CHANGELOG entry in 79a6d14 covering the collision fix, the raw .hashes key-shape change, and the need to rebuild caches retained from 9.0/9.1 before reusing them through hashes=.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants