Skip to content

HDDS-16298. ECBlockChecksumComputer.computeMd5Crc calls MessageDigest.digest() twice and returns empty MD5 - #11125

Open
shuan1026 wants to merge 2 commits into
apache:masterfrom
shuan1026:HDDS-16298
Open

HDDS-16298. ECBlockChecksumComputer.computeMd5Crc calls MessageDigest.digest() twice and returns empty MD5#11125
shuan1026 wants to merge 2 commits into
apache:masterfrom
shuan1026:HDDS-16298

Conversation

@shuan1026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

computeMd5Crc called MessageDigest.digest() twice. The second call hashes empty input after reset, so EC MD5MD5CRC file checksums were content-independent. Use the first digest for setOutBytes.

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

@jojochuang jojochuang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 jojochuang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@jojochuang

Copy link
Copy Markdown
Contributor

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: TestECBlockChecksumComputer.java

Mirrors TestReplicatedBlockChecksumComputer for EC:

  • testComputeMd5Crc — builds stripe checksum with parity padding stripped, asserts output equals MD5Hash.digest(dataChecksumBytes) (would fail on the pre-fix double-digest() bug).
  • testComputeCompositeCrc — asserts composite CRC bytes against an independently computed CrcComposer result over parity-stripped stripe data.

Verified locally:

mvn -pl :ozone-client test -Dtest=TestECBlockChecksumComputer -DskipShade -DskipRecon -DskipDocs

Both tests pass with your one-line fix applied.

@shuan1026

Copy link
Copy Markdown
Contributor Author

Thanks @jojochuang for the review! Test coverage has been added.

@jojochuang jojochuang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 pending CI

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