fix(core): require extra_digest to be bytes-like in make_program_cache_key - #2555
Open
LeSingh1 wants to merge 1 commit into
Open
fix(core): require extra_digest to be bytes-like in make_program_cache_key#2555LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
…e_key
Every guard around extra_digest asks only whether it ``is not None``:
if extra_digest is None:
external = [n for n in _EXTERNAL_CONTENT_OPTIONS if _option_is_set(options, n)]
if external:
raise ValueError("... refuses to build a key ... without an extra_digest ...")
and the value is consumed with a bare coercion:
if extra_digest is not None:
_update("extra_digest", bytes(extra_digest))
So a non-bytes value satisfies the requirement to supply a digest without
supplying one. ``extra_digest=0`` -- a plausible "no digest yet" sentinel, or
the result of an int-returning hash helper -- is the worst shape, because
``bytes(0)`` is empty:
make_program_cache_key(..., options=ProgramOptions(include_path="/my/headers"),
extra_digest=0) # accepted
key(extra_digest=0) == key(extra_digest=b"") # True
The caller gets a persistent cache key that is completely blind to the header
contents it was asked to fingerprint -- exactly the failure the ValueError
exists to prevent. Every edit under /my/headers is then served a stale cubin.
Related, from the same missing type check:
key(extra_digest=5) == key(extra_digest=b"\x00" * 5) # True -- collision
extra_digest=-1 -> ValueError: negative count
extra_digest="abc" -> TypeError: string argument without an encoding
extra_digest=1.5 -> TypeError: cannot convert 'float' object to bytes
all surfacing from inside the hasher rather than from the argument check.
Check the type up front, before any guard consults it. bytes, bytearray,
memoryview, and None are accepted, as documented; anything else raises
TypeError, matching how this module already reports a bad ``code`` or
``name_expressions`` element type.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
make_program_cache_keyrequires anextra_digestwhenever the options pull in file content the key cannot otherwise observe (include_path,pre_include,pch,use_pch,pch_dir;use_libdevice=Trueon NVVM; an NVRTCoptions.namewith a directory component). Every one of those guards asks only whether it is not None:and the value is consumed with a bare coercion:
So a non-bytes value satisfies the requirement to supply a digest without supplying one.
extra_digest=0is the worst shape, becausebytes(0)isb"":The caller ends up with a persistent cache key that is completely blind to the header contents it was asked to fingerprint — precisely the failure the
ValueErrorexists to prevent. Every edit under/my/headersis then served a stale cubin.0is a plausible "no digest yet" sentinel, and an int-returning hash helper (hash(...),zlib.crc32(...)) lands here just as easily.Verified against
main, with the baselineinclude_pathcase for contrast:maininclude_path=...,extra_digest=NoneValueError: ... without an extra_digest(correct)include_path=...,extra_digest=0key(extra_digest=0)vskey(extra_digest=b"")key(extra_digest=5)vskey(extra_digest=b"\x00"*5)extra_digest=-1ValueError: negative countextra_digest="abc"TypeError: string argument without an encodingextra_digest=1.5TypeError: cannot convert 'float' object to bytesThe last three surface from inside the hasher rather than from an argument check.
Fix
Check the type up front, before any guard consults it.
bytes,bytearray,memoryview, andNoneare accepted — the documented contract — and anything else raisesTypeError, matching how this module already reports a badcodeorname_expressionselement type. The docstring's parameter description andRaisessection are updated.No behavior change for any bytes-like or
Nonedigest.Tests
Added to
cuda_core/tests/test_program_cache.py, beside the existingtest_make_program_cache_key_rejects:test_make_program_cache_key_rejects_non_bytes_extra_digestover0,5,-1,True,"abcd",1.5,["abcd"];test_make_program_cache_key_accepts_bytes_like_extra_digest—bytes/bytearray/memoryvieware accepted and hash identically;test_non_bytes_extra_digest_cannot_bypass_the_external_content_guard— pins the whole story:Nonestill raises theValueError,0now raisesTypeError, and a real digest still gets through.Verification I could and could not do
_keys.pyneedsProgramOptions,cuda_utils, andcuda.core.typing, so I loaded it by path with stubs for those three (the version probes are already exception-tolerant via_hash_probe_failure, so a stub that raises is fine) and ran every case above against bothupstream/mainand the fix. The table above is from theupstream/mainrun; with the fix all seven rejected values raiseTypeError, all three bytes-like spellings are accepted and hash identically,Noneis unchanged, and theinclude_pathguard is no longer bypassable.ruff checkcompared against anupstream/mainbaseline of the same file: no new findings (the file has 4 pre-existingUP038reports under my local ruff 0.12.11; the repo pins v0.15.9 where that rule no longer exists — I used theX | Yform so the count stays at 4 either way).ruff format --checkandpython -m py_compileclean.pytest cuda_core/tests/test_program_cache.pyitself — it importscuda.core, which is not importable here (no CUDA driver, no built extension modules). The stub run exercises the same code path the new tests do. Please treat CI as the first real run.Related
Independent of #2553 (constructor validation in
_in_memory.py/_file_stream.py) — different file, no overlap.