fix: add path and length delimiter to Sha directory hashing to prevent collisions#5262
fix: add path and length delimiter to Sha directory hashing to prevent collisions#5262circuito-suman wants to merge 3 commits into
Conversation
…t collisions Without a per-file delimiter, two directory trees whose file contents concatenate to the same byte sequence produce identical SHA-256 hashes, breaking the contract the class advertises. For example: dir A: file1="ab", file2="c" → concat "abc" → same hash dir B: file1="a", file2="bc" → concat "abc" → same hash Fix: before streaming each file's content into the digest, feed: 1. the relative file path (UTF-8 bytes) — so renames change the hash 2. a NUL byte separator — unambiguous boundary between path and content 3. the 8-byte file size (big-endian long) — so boundary shifts change the hash Adds ShaTest covering the collision case, file-rename case, and determinism, as required by the acceptance criteria in objectionary#5254. Fixes objectionary#5254
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
✅ Performance gain: |
|
|
@circuito-suman It is not a good idea to name Git branches the way you named this one: " |
|
Got it, will use the ticket number as branch name next time. Thanks for the feedback. |
gemshrine
left a comment
gemshrine
left a comment
There was a problem hiding this comment.
Three concerns — one that explains the mvn failure on all three OS jobs, one cross-platform hash instability in Sha itself, and one about the caller Cache.dirSha still carrying the same class of collision this PR aims to close. Details in the inline comments.
| MessageDigest.getInstance("SHA-256").digest(msg.getBytes(encoding)) | ||
| ) | ||
| ) | ||
| Matchers.equalTo(CacheTest.hash(temp, source)) |
There was a problem hiding this comment.
CacheTest.hash(temp, source) relativizes against temp, so it hashes "message.txt" + NUL + 8B(size) + content. But Cache.sha(source) delegates to new Sha(source).toString(), and Sha.hash walks a single file with path = source — meaning path.relativize(file) is the empty Path, and production hashes "" + NUL + 8B(size) + content. The assertion can never match, which explains why mvn fails on all three OS jobs.
Same mismatch in generatesCorrectHashForLargeFile (line 176), generatesCorrectHashForTinyFile (line 197), and the folder test at lines 223–224. Passing source (the file itself) as the relativization root — hash(source, source) — would give the empty rel prefix and match production.
| // 1. Hash the relative path so renames change the digest | ||
| final Path rel = path.relativize(file); | ||
| digest.update( | ||
| rel.toString().getBytes(StandardCharsets.UTF_8) |
There was a problem hiding this comment.
rel.toString() uses the OS-native separator (\ on Windows, / on Unix), so the same directory tree produces different digests on different OS. The sort key on line 60 (Comparator.comparing(Path::toString)) inherits the same OS dependency; for path shapes like a1 (0x31) vs a/b (0x2F on Unix, \ = 0x5C on Windows), the two entries land on different sides of each other and change the concatenation order.
Since Cache writes the hash to a .sha256 file that a subsequent build (potentially on a different host) reads, this reintroduces a portability class of hash instability. Normalizing to / closes both cases:
rel.toString().replace(java.io.File.separatorChar, '/').getBytes(StandardCharsets.UTF_8)And the same normalization on the sort key.
| // 2. NUL byte as unambiguous separator between path and content | ||
| digest.update((byte) 0); | ||
| // 3. Hash the file size so boundary shifts change the digest | ||
| final long size = Files.size(file); |
There was a problem hiding this comment.
Hashing path + NUL + size + content closes the collision when someone calls new Sha(dir).toString() directly. But the maven-plugin's actual directory-hashing caller — Cache.dirSha at Cache.java:142 — hashes each regular file with new Sha(p).toString() and then base64-joins the results. Because Sha on a single file uses path = file, rel is empty, and none of the new fields (path prefix, size) reach the outer aggregation. Two dirs [foo/x="a"] and [bar/x="a"] still produce identical Cache dir-hashes (same for flat [x="a"] vs nested [sub/x="a"]).
If Fixes #5254 is meant to close the class of bug for Cache, the fix needs to reach Cache.dirSha — either by having it call Sha on the directory as a whole (which needs the cross-platform normalization from the other comment) or by hashing the per-file rel path inside dirSha too.
3 participants
Without a per-file delimiter, two directory trees whose file contents concatenate to the same byte sequence produce identical SHA-256 hashes, breaking the contract the class advertises. For example:
dir A: file1="ab", file2="c" → concat "abc" → same hash
dir B: file1="a", file2="bc" → concat "abc" → same hash
Fix: before streaming each file's content into the digest, feed:
Adds ShaTest covering the collision case, file-rename case, and determinism, as required by the acceptance criteria in #5254.
Fixes #5254