Fix ByteBuf memory leak in blob deserialization error paths

[j-tyler]

Summary

When CRC validation fails during blob deserialization in MessageFormatRecord, the bytebuf allocated by Utils.readNettyByteBufFromCrcInputStream() was not being released, causing a memory leak.

The issue affected all three blob format versions (V1, V2, V3) in their respective deserializeBlobRecord() methods. When corrupt data triggered a MessageFormatException during CRC validation, the allocated bytebuf slice remained unreleased.

Changes:

• Extract CRC validation logic into validateCrcAndManageByteBuf() helper method
• Implement try-finally pattern to ensure ByteBuf.release() on validation failure
• Apply fix consistently across Blob_Format_V1, V2, and V3 deserializers
• Add comprehensive leak detection tests using custom ByteBuf wrappers

Testing Done

• MessageFormatCorruptDataLeakTest verifies ByteBuf cleanup on corrupt data
• Parameterized tests cover all blob format versions (V1, V2, V3)
• Control tests ensure no false positives on successful deserialization
• Uses SliceCapturingByteBuf wrapper via DelegateByteBuf to track slice creation

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 197 lines in your changes missing coverage. Please review.
✅ Project coverage is 31.36%. Comparing base (52ba813) to head (7be3272).
⚠️ Report is 331 commits behind head on master.

Files with missing lines	Patch %	Lines
...java/com/github/ambry/commons/DelegateByteBuf.java	0.00%	186 Missing ⚠️
...ithub/ambry/messageformat/MessageFormatRecord.java	0.00%	10 Missing ⚠️
...ls/src/main/java/com/github/ambry/utils/Utils.java	0.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (52ba813) and HEAD (7be3272). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (52ba813)	HEAD (7be3272)
	3	2

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #3177       +/-   ##
=============================================
- Coverage     64.24%   31.36%   -32.88%     
+ Complexity    10398     5809     -4589     
=============================================
  Files           840      931       +91     
  Lines         71755    79180     +7425     
  Branches       8611     9430      +819     
=============================================
- Hits          46099    24835    -21264     
- Misses        23004    52360    +29356     
+ Partials       2652     1985      -667     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justinlin-linkedin]

hmm, this one is tricky. The reason why we are not releasing this ByteBuf anywhere is because we didn't increase the refCounter when creating this ByteBuf. In readNettyByteBufFromCrcInputStream, we use slice method to create this ByteBuf, instead of retainedSlice.

If we call release on the failure here, then we would have to increase the refCounter by calling retainedSlice, but that would mean in the success path, we also have to call release one more time.

It does seems like a good idea to call retain/release here, since it shows the clear ownership of each ByteBuf. But in this case, we would have update other files on the success path to release this ByteBuf. What do you think?

[j-tyler]

Take a look at how this is handled in GetBlobOperation.handleBody() and subsequent calls. The chunkBuf within the BlobData is owned by the caller and it's their responsibility to release. If MessageFormatRecord.deserializeBlob() throws, GetBlobOperation.handleBody() never handles the bytebuf here or in the input stream.

In my read and testing of Utils.readNettyByteBufFromCrcInputStream() it always returns a Bytebuf passing the ownership (and the responsibility to release) to the caller. Thus if validateCrcAndManageByteBuf fails we must release here.

[j-tyler]

There's another path where we improve the memory management expectations in readNettyByteBufFromCrcInputStream and it's callers which is probably preferred but a slightly larger change. Let me pull this back to draft and spend some more time in testing that approach.

[j-tyler]

The readByteBufferFromCrcInputStream methods which return ByteBuffer (which are not part of this change) would require a significant code change as ByteBuffer doesn't have refCnt semantics. If we decide to do that work it should happen in a new PR. Made the minimal change for this fix.

[j-tyler]

The more I work on this the more I see work that needs to be done. Pulling this back to draft for now.