🍒 12013 - Fix Metaspace leak in SCA Reachability retransform and opcode injection#12075
Merged
Merged
Conversation
…on (#12013) Fix Metaspace leak in SCA Reachability (APPSEC-69201) - Bound retransformClasses() retries: track per-class failure counts and give up after MAX_RETRANSFORM_ATTEMPTS instead of re-queueing failed classes forever, which retained Class<?>/ClassLoader references indefinitely. - Cover all 13 ASM MethodVisitor visitXxxInsn callbacks in ScaMethodCallbackInjector.MethodEntryInjector so the entry-point injection triggers on the first bytecode instruction regardless of its opcode kind, instead of only on the 4 previously handled ones. Bisect failing retransform batches instead of coupling them by failure count Split pendingRetransform into independent per-batch queue entries and retransform each batch separately; on failure, bisect multi-class batches into two halves deferred to the next heartbeat instead of tying an unrelated healthy class to a permanently-failing one. Simplify singleton-batch retransform retry to drop-on-first-failure Replace the retransformFailureCount/MAX_RETRANSFORM_ATTEMPTS bounded retry with an immediate drop once a batch is narrowed down to a single class, per the performance-over-detection trade-off agreed with mcculls on PR #12013. Batch already-loaded classes as one shared retransform group checkAlreadyLoadedClasses() previously queued each already-loaded vulnerable class as its own singleton batch, turning the common all-succeed startup path into one retransformClasses() call per class instead of one call for all of them. Queue them as a single shared batch instead; bisection still applies on the next heartbeat if that batch fails. Found by Codex review on PR #12013. Add test coverage for remaining MethodEntryInjector opcode overrides jacocoTestCoverageVerification failed: visitIntInsn/visitJumpInsn/visitIincInsn/ visitTableSwitchInsn/visitLookupSwitchInsn/visitMultiANewArrayInsn were never exercised by any test, leaving instruction coverage at 0.7 against the 0.8 minimum for the appsec module. Remove non-modifiable classes from batch in place instead of copying to a second list Addresses mccull's review comment: mutate the polled batch directly via removeIf() instead of allocating a second ArrayList to hold the modifiable classes. Updates tests that seeded pendingRetransform with immutable singletonList() batches, which is no longer valid now that batches must support in-place removal. Merge branch 'master' into sca-bug Merge branch 'master' into sca-bug Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io> (cherry picked from commit 428bb15)
claponcet
added
type: bug fix
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The changed retransformation batching and ASM entry injection paths were reviewed against the PR diff and surrounding tests, with no clear diff-only behavioral regression identified. Focused Gradle execution was unavailable because the wrapper could not download offline and the installed build requires Java 25; no additional tests were recommended because the PR already adds coverage for the relevant opcode and failure-batching cases.
🤖 Datadog Autotest · Commit b06c87e · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
🎯 Code Coverage (details) 🔗 Commit SHA: b06c87e | Docs | Datadog PR Page | Give us feedback! |
Contributor
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport #12013 to release/v1.64.x