HDDS-16298. ECBlockChecksumComputer.computeMd5Crc calls MessageDigest.digest() twice and returns empty MD5 - #11125
HDDS-16298. ECBlockChecksumComputer.computeMd5Crc calls MessageDigest.digest() twice and returns empty MD5#11125shuan1026 wants to merge 2 commits into
Conversation
….digest() twice and returns empty MD5
jojochuang
left a comment
There was a problem hiding this comment.
This is subtle. The bug: the digest is reset after the first call is made. Obvious there's a test gap here otherwise we'd have found out. Can we add a unit test to ensure checksum value is expected? This is critical to the integrity of data so quite important.
jojochuang
left a comment
There was a problem hiding this comment.
Review
The fix is correct and minimal: MessageDigest.digest() finalizes and resets the digester, so the second call hashes empty input (MD5 of the empty string) and was what setOutBytes returned. The debug log on the line above already used the first fileMD5, which is a good hint this was a copy/paste slip from HDDS-10480 (#6342, Mar 2024).
Impact: MD5MD5CRC EC file checksums were content-independent — every key would get the same MD5 regardless of stripe checksum data. COMPOSITE_CRC path is unaffected (blockCrcComposer.digest() is called once).
Test gap: TestReplicatedBlockChecksumComputer.testComputeMd5Crc asserts the expected MD5 bytes for replicated blocks, but there is no EC counterpart. TestFileChecksumHelper EC/MD5MD5CRC cases only check checksum type and block count, not the digest value — so this regression survived since HDDS-10480.
Note: the PR description references TestECBlockChecksumComputer, but that class is not in this PR (only the one-line production fix). Please either add the test here or update the test plan.
Inline concern on computeMd5Crc:
This is subtle. The bug: the digest is reset after the first digest() call, and the second call returns the MD5 of empty input. There is an obvious test gap here — otherwise we would have caught this when HDDS-10480 landed.
Can we add a unit test (mirroring TestReplicatedBlockChecksumComputer.testComputeMd5Crc) that builds an ECBlockChecksumComputer with known stripe checksum bytes and asserts the output MD5 matches MD5Hash.digest(...) over the parity-stripped stripe data? This is critical to data integrity, so worth locking down in this PR rather than as follow-up.
|
Added unit-test coverage on a fork branch — please feel free to cherry-pick or copy into this PR. Branch: https://github.com/jojochuang/ozone/tree/HDDS-16298-add-tests New file: Mirrors
Verified locally: Both tests pass with your one-line fix applied. |
|
Thanks @jojochuang for the review! Test coverage has been added. |
What changes were proposed in this pull request?
computeMd5CrccalledMessageDigest.digest()twice. The second call hashes empty input after reset, so ECMD5MD5CRCfile checksums were content-independent. Use the first digest forsetOutBytes.This was introduced in HDDS-10480 when computeMd5Crc was rewritten to use MessageDigest.update(). The new code calls digester.digest() twice; the second call (after reset) is passed to setOutBytes.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16298
How was this patch tested?
mvn -pl :ozone-client test -Dtest=TestECBlockChecksumComputer