Compare hex checksums case-insensitively in Download._check_hash - #1048
Open
karthikchundi-commits wants to merge 1 commit into
Open
karthikchundi-commits wants to merge 1 commit into
karthikchundi-commits wants to merge 1 commit into
Conversation
hashlib's hexdigest() is always lowercase, but the sha256/md5 field in a Croissant JSON-LD isn't specified as case-locked, and hex-encoded checksums are conventionally treated as case-insensitive by every common checksum tool. A dataset published with an uppercase hex checksum currently fails a correct, fully-intact download with a spurious "Hash of downloaded file is not identical" error. Lowercase only the hex comparison, not the base64 fallback below it - base64 is genuinely case-sensitive (upper/lowercase letters encode different values), so normalizing case there would risk silently accepting a corrupted file whose base64 hash happens to differ from the expected one only in letter case. Added a regression test using the well-known SHA-256 of empty content, published as the existing lowercase test data uppercased. Assisted-by: AI Signed-off-by: Karth <karthik.chundi@gmail.com>
|
MLCommons CLA bot: |
Author
|
recheck |
This branch has not been deployed
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
1 participant
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.
What
Download._check_hashcompares a downloaded file's SHA-256/MD5 hex digest against thesha256/md5value in the Croissant JSON-LD using a plain==.hashlib'shexdigest()is always lowercase, but hex-encoded checksums are conventionally treated as case-insensitive by essentially every checksum tool (many emit uppercase, e.g. Windows'Get-FileHash). A dataset published with an uppercase hex checksum currently makes a correct, fully-intact download fail with a spurious"Hash of downloaded file ... is not identical with the reference"error.Fix
Lowercase only the hex comparison. The base64 fallback comparison right below it is intentionally left untouched - base64 is genuinely case-sensitive (upper/lowercase letters encode different values), so normalizing case there would risk silently accepting a corrupted file whose base64 hash happens to differ from the expected one only in letter case.
Testing
Added
test_uppercase_hex_hash_does_match, using the well-known SHA-256 of empty content (the same value the existingtest_sha256_hashes_do_matchalready uses in lowercase), uppercased, across all threeCroissantVersions.Ran the full
mlcroissant/_src/operation_graph/test suite locally: 95 passed, 1 skipped, 3 pre-existing failures - all Windows path-separator (\vs/) comparison issues in unrelated functions (test_get_download_filepath,test_get_fullpaths, onetest_apply_transforms_fnregex case), confirmed unrelated to this change by inspection; none touch_check_hashor the hash-comparison code path.