Skip to content

Conversation

@j-tyler
Copy link
Contributor

@j-tyler j-tyler commented Nov 11, 2025

Summary

DecryptJob.closeJob() does not release encryptedBlobContent when called.

  • DecryptJob constructor takes ownership of encryptedBlobContent ByteBuf
  • Normal execution: run() releases the buffer in its finally block
  • Abort scenario: closeJob() is called before run() executes
  • Problem: closeJob() only invoked callback, never released the owned buffer
  • Result: Every aborted decryption job leaked a ByteBuf

DecryptJob owns encryptedBlobContent after construction and must release it in all exit paths.

GCMCryptoService.decrypt() exception handler releass caller's buffer instead of service's buffer.

  • Service allocates new decryptedContent buffer
  • Decryption fails during doFinal()
  • Exception handler releases toDecrypt (input buffer owned by caller)
  • Exception handler never releases decryptedContent (output buffer owned by service)
  • Result: Double bug - leaked service's buffer AND corrupted caller's buffer

Input toDecrypt ownership is retained by the caller. Service must not touch it Output decryptedContent is owned by service until it returns. Service must release on error.

Fix 1: DecryptJob.closeJob() release owned buffer
Fix 2: GCMCryptoService.decrypt() released correct buffer

Testing Done

testDecryptJobCloseBeforeRunReleasesBuffer()

  • Calls closeJob() before run() executes (abort scenario)
  • Proves bug: Without fix, NettyByteBufLeakHelper detects encryptedBlobContent
  • Validates fix: With fix, no leak detected

testDecryptJobCloseIdempotent()

  • Calls closeJob() multiple times
  • Proves fix works: No IllegalReferenceCountException from double-release
  • Validates AtomicBoolean guard: Cleanup logic only executes once

testDecryptJobNormalExecutionNoLeak()

  • Baseline test for normal run() execution
  • Confirms happy path already correct

testDecryptWithCorruptedCiphertextOwnership()

  • Corrupts ciphertext to trigger exception during doFinal()
  • Proves bug: Without fix, input buffer refCnt=0 (incorrectly released)
  • Validates fix: With fix, input buffer refCnt=1 (untouched)

testDecryptWithWrongKeyOwnership()

  • Uses wrong key to trigger authentication failure
  • Tests different exception path with same ownership requirements
  • Validates fix: Input buffer untouched, no leak

testSuccessfulDecryptOwnership()

  • Baseline test for successful decryption
  • Confirms service doesn't release input in success path

DecryptJob.closeJob() does not release encryptedBlobContent when called.

- DecryptJob constructor takes ownership of encryptedBlobContent ByteBuf
- Normal execution: run() releases the buffer in its finally block
- Abort scenario: closeJob() is called before run() executes
- Problem: closeJob() only invoked callback, never released the owned buffer
- Result: Every aborted decryption job leaked a ByteBuf

DecryptJob owns encryptedBlobContent after construction and must release it
in all exit paths.

GCMCryptoService.decrypt() exception handler releass caller's buffer instead
of service's buffer.

- Service allocates new decryptedContent buffer
- Decryption fails during doFinal()
- Exception handler releases toDecrypt (input buffer owned by caller)
- Exception handler never releases decryptedContent (output buffer owned by service)
- Result: Double bug - leaked service's buffer AND corrupted caller's buffer

Input toDecrypt ownership is retained by the caller. Service must not touch it
Output decryptedContent is owned by service until it returns. Service must release on error

testDecryptJobCloseBeforeRunReleasesBuffer()
- Calls closeJob() before run() executes (abort scenario)
- Proves bug: Without fix, NettyByteBufLeakHelper detects encryptedBlobContent
- Validates fix: With fix, no leak detected

testDecryptJobCloseIdempotent()
- Calls closeJob() multiple times
- Proves fix works: No IllegalReferenceCountException from double-release
- Validates AtomicBoolean guard: Cleanup logic only executes once

testDecryptJobNormalExecutionNoLeak()
- Baseline test for normal run() execution
- Confirms happy path already correct

testDecryptWithCorruptedCiphertextOwnership()
- Corrupts ciphertext to trigger exception during doFinal()
- Proves bug: Without fix, input buffer refCnt=0 (incorrectly released)
- Validates fix: With fix, input buffer refCnt=1 (untouched)

testDecryptWithWrongKeyOwnership()
- Uses wrong key to trigger authentication failure
- Tests different exception path with same ownership requirements
- Validates fix: Input buffer untouched, no leak

testSuccessfulDecryptOwnership()
- Baseline test for successful decryption
- Confirms service doesn't release input in success path

Fix 1: DecryptJob.closeJob() release owned buffer
Fix 2: GCMCryptoService.decrypt() released correct buffer
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.89%. Comparing base (52ba813) to head (281d1e3).
⚠️ Report is 323 commits behind head on master.

Files with missing lines Patch % Lines
.../main/java/com/github/ambry/router/DecryptJob.java 87.50% 0 Missing and 1 partial ⚠️
...java/com/github/ambry/router/GCMCryptoService.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3170      +/-   ##
============================================
+ Coverage     64.24%   69.89%   +5.65%     
- Complexity    10398    12796    +2398     
============================================
  Files           840      930      +90     
  Lines         71755    78943    +7188     
  Branches       8611     9429     +818     
============================================
+ Hits          46099    55179    +9080     
+ Misses        23004    20844    -2160     
- Partials       2652     2920     +268     

☔ 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.

@SophieGuo410 SophieGuo410 merged commit 256b6c2 into linkedin:master Nov 17, 2025
5 checks passed
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.

4 participants