From b19360ad9062a63ec62e96b9eaf0c1765b994953 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 21 Jul 2026 09:34:48 +0200 Subject: [PATCH 1/6] 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. --- .../appsec/sca/ScaMethodCallbackInjector.java | 60 +++++++++ .../sca/ScaReachabilityTransformer.java | 36 +++++- .../sca/ScaReachabilityMethodLevelTest.java | 115 ++++++++++++++++++ .../sca/ScaReachabilityRetransformTest.java | 94 ++++++++++++++ 4 files changed, 303 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaMethodCallbackInjector.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaMethodCallbackInjector.java index 1432e9f27bf..f7e9859e8e8 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaMethodCallbackInjector.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaMethodCallbackInjector.java @@ -6,6 +6,7 @@ import net.bytebuddy.jar.asm.ClassReader; import net.bytebuddy.jar.asm.ClassVisitor; import net.bytebuddy.jar.asm.ClassWriter; +import net.bytebuddy.jar.asm.Handle; import net.bytebuddy.jar.asm.Label; import net.bytebuddy.jar.asm.MethodVisitor; import net.bytebuddy.jar.asm.Opcodes; @@ -100,6 +101,65 @@ public void visitFieldInsn(int opcode, String owner, String name, String descrip super.visitFieldInsn(opcode, owner, name, descriptor); } + @Override + public void visitTypeInsn(int opcode, String type) { + ensureInjected(); + super.visitTypeInsn(opcode, type); + } + + @Override + public void visitIntInsn(int opcode, int operand) { + ensureInjected(); + super.visitIntInsn(opcode, operand); + } + + @Override + public void visitLdcInsn(Object value) { + ensureInjected(); + super.visitLdcInsn(value); + } + + @Override + public void visitJumpInsn(int opcode, Label label) { + ensureInjected(); + super.visitJumpInsn(opcode, label); + } + + @Override + public void visitIincInsn(int varIndex, int increment) { + ensureInjected(); + super.visitIincInsn(varIndex, increment); + } + + @Override + public void visitTableSwitchInsn(int min, int max, Label dflt, Label... labels) { + ensureInjected(); + super.visitTableSwitchInsn(min, max, dflt, labels); + } + + @Override + public void visitLookupSwitchInsn(Label dflt, int[] keys, Label[] labels) { + ensureInjected(); + super.visitLookupSwitchInsn(dflt, keys, labels); + } + + @Override + public void visitMultiANewArrayInsn(String descriptor, int numDimensions) { + ensureInjected(); + super.visitMultiANewArrayInsn(descriptor, numDimensions); + } + + @Override + public void visitInvokeDynamicInsn( + String name, + String descriptor, + Handle bootstrapMethodHandle, + Object... bootstrapMethodArguments) { + ensureInjected(); + super.visitInvokeDynamicInsn( + name, descriptor, bootstrapMethodHandle, bootstrapMethodArguments); + } + private void ensureInjected() { if (!injected) { injected = true; diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java index 1e21cecad46..951bbc1a143 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java @@ -92,6 +92,20 @@ public final class ScaReachabilityTransformer implements ClassFileTransformer { /** Class names (internal format) queued for deferred retransformation by name lookup. */ @VisibleForTesting final Set pendingRetransformNames = ConcurrentHashMap.newKeySet(); + /** + * {@code retransformClasses()} is atomic for the whole array: if any single class in the batch + * fails to verify/redefine, the entire batch throws and would otherwise be re-queued forever (see + * {@link #performPendingRetransforms()}), permanently retaining every {@code Class} in that + * batch and leaking Metaspace. This tracks per-class failure counts so a class that keeps failing + * is dropped after {@link #MAX_RETRANSFORM_ATTEMPTS} instead of blocking the batch and being + * retried indefinitely. Entries are removed on success or once the limit is reached, so this map + * only grows with currently-failing classes. + */ + @VisibleForTesting + final Map, Integer> retransformFailureCount = new ConcurrentHashMap<>(); + + private static final int MAX_RETRANSFORM_ATTEMPTS = 5; + public ScaReachabilityTransformer(ScaCveDatabase database, Instrumentation instrumentation) { this.database = database; this.instrumentation = instrumentation; @@ -358,10 +372,28 @@ public void performPendingRetransforms() { log.debug( "SCA Reachability: retransformed {} class(es) for method-level detection", toRetransform.size()); + for (Class c : toRetransform) { + retransformFailureCount.remove(c); + } } catch (Throwable t) { log.debug("SCA Reachability: retransformClasses failed", t); - // Re-queue on failure so the next heartbeat can retry - pendingRetransform.addAll(toRetransform); + // retransformClasses() is atomic for the whole array, so a single unrelated failure (this + // transformer's own bytecode, another registered transformer, or a transient JVM issue) + // fails every class in the batch. Re-queue each class individually, up to a bounded number + // of attempts, so a class that keeps failing is eventually dropped instead of retaining its + // Class (and its ClassLoader) forever. + for (Class c : toRetransform) { + int attempts = retransformFailureCount.merge(c, 1, Integer::sum); + if (attempts < MAX_RETRANSFORM_ATTEMPTS) { + pendingRetransform.add(c); + } else { + retransformFailureCount.remove(c); + log.debug( + "SCA Reachability: giving up retransforming {} after {} failed attempts", + c.getName(), + attempts); + } + } } } diff --git a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityMethodLevelTest.java b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityMethodLevelTest.java index 5083745e48f..78a2fa98631 100644 --- a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityMethodLevelTest.java +++ b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityMethodLevelTest.java @@ -20,6 +20,12 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Supplier; +import net.bytebuddy.jar.asm.ClassReader; +import net.bytebuddy.jar.asm.ClassVisitor; +import net.bytebuddy.jar.asm.Handle; +import net.bytebuddy.jar.asm.MethodVisitor; +import net.bytebuddy.utility.OpenedClassReader; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -269,6 +275,115 @@ void inject_sameMethodNameInDifferentClassesProduceIndependentHits() throws Exce assertEquals("GHSA-shared", hits.get(0).vulnId()); } + /** + * Fixture whose methods start with bytecode opcodes that {@code MethodEntryInjector} used to + * leave unhandled (NEW, INVOKEDYNAMIC, LDC): the injected callback landed after that first + * instruction instead of at true method entry. + */ + public static class UnhandledFirstOpcodeMethods { + public static Object newFirst() { + return new Object(); + } + + public static Supplier lambdaFirst() { + return () -> "hi"; + } + + public static String ldcFirst() { + return "constant".trim(); + } + } + + @Test + void inject_withoutLineNumbersInjectsBeforeNewInstruction() throws Exception { + assertCallbackIsFirstInstruction("newFirst"); + } + + @Test + void inject_withoutLineNumbersInjectsBeforeInvokeDynamicInstruction() throws Exception { + assertCallbackIsFirstInstruction("lambdaFirst"); + } + + @Test + void inject_withoutLineNumbersInjectsBeforeLdcInstruction() throws Exception { + assertCallbackIsFirstInstruction("ldcFirst"); + } + + /** + * Regression test for the delayed-injection bug: verifies (via ASM inspection of the injected + * bytecode, not just observed behavior) that the very first instruction visited in {@code + * methodName} is the callback's own {@code LDC} of the injected {@code vulnId} — i.e. the + * callback runs strictly before the method's original first instruction. Before the fix, {@code + * MethodEntryInjector} did not override {@code visitTypeInsn}/{@code + * visitInvokeDynamicInsn}/{@code visitLdcInsn}, so for methods starting with {@code NEW}, {@code + * INVOKEDYNAMIC} or {@code LDC} the original instruction was visited first and the callback was + * spliced in right after it instead of before it. + */ + private void assertCallbackIsFirstInstruction(String methodName) throws Exception { + byte[] original = bytecodeWithoutDebugInfo(UnhandledFirstOpcodeMethods.class); + Map> callbacks = new HashMap<>(); + callbacks.put( + methodName, + Collections.singletonList( + spec( + "GHSA-first-instruction", + "com.example:lib", + "1.0.0", + UnhandledFirstOpcodeMethods.class.getName(), + methodName))); + byte[] modified = ScaMethodCallbackInjector.inject(original, callbacks); + + assertEquals( + "GHSA-first-instruction", + firstVisitedInstructionOf(modified, methodName), + "the injected callback's first LDC (vulnId) must be the method's first visited instruction"); + } + + /** + * Reads {@code methodName} from {@code bytecode} and returns whatever value/marker the very first + * opcode-visiting callback receives (the LDC constant for {@code visitLdcInsn}, or the opcode + * name for other instruction kinds). + */ + private static Object firstVisitedInstructionOf(byte[] bytecode, String methodName) { + Object[] first = {null}; + new ClassReader(bytecode) + .accept( + new ClassVisitor(OpenedClassReader.ASM_API) { + @Override + public MethodVisitor visitMethod( + int access, String name, String descriptor, String signature, String[] exc) { + if (!name.equals(methodName)) { + return null; + } + return new MethodVisitor(OpenedClassReader.ASM_API) { + @Override + public void visitLdcInsn(Object value) { + if (first[0] == null) { + first[0] = value; + } + } + + @Override + public void visitTypeInsn(int opcode, String type) { + if (first[0] == null) { + first[0] = "TYPE_INSN:" + opcode; + } + } + + @Override + public void visitInvokeDynamicInsn(String n, String d, Handle h, Object... args) { + if (first[0] == null) { + first[0] = "INVOKEDYNAMIC"; + } + } + }; + } + }, + 0); + assertNotNull(first[0], "no matching instruction found in " + methodName); + return first[0]; + } + // --------------------------------------------------------------------------- // transform(): two-phase design — first load enqueues, retransform injects // --------------------------------------------------------------------------- diff --git a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java index aa37b1fcb1f..b1ca66d7f05 100644 --- a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java +++ b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java @@ -31,6 +31,11 @@ public static class Target { public void method() {} } + /** Second dummy class, distinct from {@link Target}, used to simulate a mixed batch. */ + public static class Other { + public void method() {} + } + @Test void performPendingRetransforms_retransformsAllMatchingInstances() throws Exception { // Returning the same Class twice in getAllLoadedClasses() simulates two classloader @@ -78,6 +83,95 @@ void performPendingRetransforms_requeuesOnRetransformFailure() throws Exception "both classes must be re-queued in pendingRetransform for the next heartbeat retry"); } + @Test + void performPendingRetransforms_givesUpAfterMaxFailedAttempts() throws Exception { + // retransformClasses() is atomic for the whole array: an unrelated, permanently-failing class + // (this test simulates it with a RuntimeException on every attempt) must not be re-queued + // forever, or its Class (and its ClassLoader) is retained indefinitely — the Metaspace leak + // from APPSEC-69201. After MAX_RETRANSFORM_ATTEMPTS failures the class must be dropped instead + // of re-queued. + Instrumentation instr = mock(Instrumentation.class); + when(instr.isModifiableClass(Target.class)).thenReturn(true); + doThrow(new RuntimeException("retransform failed")).when(instr).retransformClasses(any()); + + ScaCveDatabase db = ScaCveDatabase.parse(new StringReader("{\"version\":1,\"entries\":[]}")); + ScaReachabilityTransformer t = new ScaReachabilityTransformer(db, instr); + t.pendingRetransform.add(Target.class); + + // Each failed attempt re-queues the class for the next heartbeat; after the limit it must be + // dropped instead. Run one extra iteration beyond the limit to confirm it stays dropped. + for (int attempt = 1; attempt <= 6; attempt++) { + t.performPendingRetransforms(); + } + + assertTrue( + t.pendingRetransform.isEmpty(), + "class must be dropped (not re-queued) once the retry limit is exceeded"); + assertTrue( + t.retransformFailureCount.isEmpty(), + "failure count must be cleared once the class is dropped, so the map does not grow" + + " unbounded"); + } + + @Test + void performPendingRetransforms_clearsFailureCountOnEventualSuccess() throws Exception { + Instrumentation instr = mock(Instrumentation.class); + when(instr.isModifiableClass(Target.class)).thenReturn(true); + doThrow(new RuntimeException("retransform failed")) + .doNothing() + .when(instr) + .retransformClasses(any()); + + ScaCveDatabase db = ScaCveDatabase.parse(new StringReader("{\"version\":1,\"entries\":[]}")); + ScaReachabilityTransformer t = new ScaReachabilityTransformer(db, instr); + + t.pendingRetransform.add(Target.class); + t.performPendingRetransforms(); // fails once, re-queued + assertEquals(1, t.pendingRetransform.size()); + + t.performPendingRetransforms(); // succeeds this time + + assertTrue(t.pendingRetransform.isEmpty(), "class must not be re-queued after success"); + assertTrue( + t.retransformFailureCount.isEmpty(), + "failure count must be cleared once retransformClasses succeeds"); + } + + @Test + void performPendingRetransforms_dropsBothClassesWhenBatchedWithAPermanentlyFailingOne() + throws Exception { + // A batch containing one permanently-failing class and one otherwise-healthy class fails + // atomically on every attempt while both are queued together, so both accumulate the same + // failure count and are dropped in the same call once MAX_RETRANSFORM_ATTEMPTS is reached — + // the healthy class is never isolated from its unrelated batch-mate and never retransforms. + // This documents a real limitation of per-class attempt counting: it bounds retention (no + // Metaspace leak) but does not guarantee an unrelated healthy class in the same batch is ever + // successfully retransformed. + Class poison = Target.class; + Class healthy = Other.class; + + Instrumentation instr = mock(Instrumentation.class); + when(instr.isModifiableClass(poison)).thenReturn(true); + when(instr.isModifiableClass(healthy)).thenReturn(true); + doThrow(new RuntimeException("retransform failed")).when(instr).retransformClasses(any()); + + ScaCveDatabase db = ScaCveDatabase.parse(new StringReader("{\"version\":1,\"entries\":[]}")); + ScaReachabilityTransformer t = new ScaReachabilityTransformer(db, instr); + t.pendingRetransform.add(poison); + t.pendingRetransform.add(healthy); + + for (int attempt = 1; attempt <= 6; attempt++) { + t.performPendingRetransforms(); + } + + assertTrue( + t.pendingRetransform.isEmpty(), + "both classes must be dropped once the shared batch exceeds the retry limit"); + assertTrue( + t.retransformFailureCount.isEmpty(), + "failure counts for both classes must be cleared once dropped"); + } + @Test void performPendingRetransforms_skipsNonModifiableClasses() throws Exception { // Non-modifiable classes (e.g. JDK classes, primitive wrappers) must be silently discarded From 1b6d2ecb082d05b6794d03f65dcf1895c10f9aa8 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 21 Jul 2026 10:34:53 +0200 Subject: [PATCH 2/6] 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. --- .../sca/ScaReachabilityTransformer.java | 159 +++++++++++------- .../sca/ScaReachabilityRetransformTest.java | 56 +++--- .../ScaReachabilityTransformerJava9Test.java | 15 +- 3 files changed, 146 insertions(+), 84 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java index 951bbc1a143..82c81d975ac 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java @@ -77,29 +77,34 @@ public final class ScaReachabilityTransformer implements ClassFileTransformer { final ConcurrentHashMap classpathArtifactCache = new ConcurrentHashMap<>(); /** - * Classes whose bytecode needs (re)transformation for method-level symbol injection: + * Batches of classes whose bytecode needs (re)transformation for method-level symbol injection: * *
    - *
  • Classes already loaded at startup before this transformer was registered. - *
  • Classes where JAR version resolution returned no results at load time and needs a retry. + *
  • Classes already loaded at startup before this transformer was registered (queued as + * singleton batches). + *
  • Batches that failed {@code retransformClasses()} and were bisected into halves (see + * {@link #performPendingRetransforms()}). *
* - * Drained and processed by {@link #performPendingRetransforms()} on each telemetry heartbeat. + *

Each element is retransformed via its own {@code retransformClasses()} call, independently + * of the other batches, so an unrelated failure in one batch never affects another. Drained and + * processed by {@link #performPendingRetransforms()} on each telemetry heartbeat. */ @VisibleForTesting - final ConcurrentLinkedQueue> pendingRetransform = new ConcurrentLinkedQueue<>(); + final ConcurrentLinkedQueue>> pendingRetransform = new ConcurrentLinkedQueue<>(); /** Class names (internal format) queued for deferred retransformation by name lookup. */ @VisibleForTesting final Set pendingRetransformNames = ConcurrentHashMap.newKeySet(); /** - * {@code retransformClasses()} is atomic for the whole array: if any single class in the batch - * fails to verify/redefine, the entire batch throws and would otherwise be re-queued forever (see - * {@link #performPendingRetransforms()}), permanently retaining every {@code Class} in that - * batch and leaking Metaspace. This tracks per-class failure counts so a class that keeps failing - * is dropped after {@link #MAX_RETRANSFORM_ATTEMPTS} instead of blocking the batch and being - * retried indefinitely. Entries are removed on success or once the limit is reached, so this map - * only grows with currently-failing classes. + * {@code retransformClasses()} is atomic for a given batch: if any single class in it fails to + * verify/redefine, the whole batch throws. For a multi-class batch, {@link + * #performPendingRetransforms()} reacts by bisecting it into two halves, deferred to the next + * heartbeat, to isolate the failing class(es) from any healthy batch-mates. Once a batch has been + * narrowed down to a single class, further failures of that same class are tracked here so it is + * dropped after {@link #MAX_RETRANSFORM_ATTEMPTS} instead of being retried forever and leaking + * its {@code Class} (and {@code ClassLoader}) in Metaspace. Entries are removed on success or + * once the limit is reached, so this map only grows with currently-failing singleton classes. */ @VisibleForTesting final Map, Integer> retransformFailureCount = new ConcurrentHashMap<>(); @@ -277,8 +282,9 @@ public void checkAlreadyLoadedClasses() { } // All symbols are method-level: always schedule retransformation so the bytecode // callback can be injected. We can't modify bytecode during the startup scan; deferred - // to performPendingRetransforms(). - pendingRetransform.add(clazz); + // to performPendingRetransforms(). Queued as its own singleton batch so a failure never + // couples it to an unrelated class. + pendingRetransform.add(Collections.singletonList(clazz)); } } @@ -300,21 +306,30 @@ public void performPendingRetransforms() { if (instrumentation == null) { return; // no-op when instrumentation is unavailable (e.g. in unit tests) } - // Drain the direct Class queue (from checkAlreadyLoadedClasses) - List> toRetransform = new ArrayList<>(); - Class clazz; - while ((clazz = pendingRetransform.poll()) != null) { - if (instrumentation.isModifiableClass(clazz)) { - toRetransform.add(clazz); + // Drain the batch queue (from checkAlreadyLoadedClasses and prior bisections), dropping any + // classes that are no longer modifiable from within each batch while preserving the rest of + // that batch's grouping. + List>> batches = new ArrayList<>(); + List> batch; + while ((batch = pendingRetransform.poll()) != null) { + List> modifiable = new ArrayList<>(batch.size()); + for (Class c : batch) { + if (instrumentation.isModifiableClass(c)) { + modifiable.add(c); + } + } + if (!modifiable.isEmpty()) { + batches.add(modifiable); } } - // Resolve any classes queued by name (from processClass timing failures). - // Use contains+removeAll instead of remove inside the loop: the same class may be loaded - // by multiple classloaders (e.g. Spring Boot LaunchedURLClassLoader creates more than one - // instance), and we must retransform ALL of them, not just the first one found. + // Resolve any classes queued by name (from processClass timing failures) into their own + // batch. Use contains+removeAll instead of remove inside the loop: the same class may be + // loaded by multiple classloaders (e.g. Spring Boot LaunchedURLClassLoader creates more than + // one instance), and we must retransform ALL of them, not just the first one found. if (!pendingRetransformNames.isEmpty()) { Set matched = new HashSet<>(); + List> resolved = new ArrayList<>(); for (Class loaded : instrumentation.getAllLoadedClasses()) { if (loaded == null) { continue; @@ -324,14 +339,17 @@ public void performPendingRetransforms() { // Always add to matched to drain the pending set; only retransform if modifiable. matched.add(name); if (instrumentation.isModifiableClass(loaded)) { - toRetransform.add(loaded); + resolved.add(loaded); } } } pendingRetransformNames.removeAll(matched); + if (!resolved.isEmpty()) { + batches.add(resolved); + } } - if (toRetransform.isEmpty()) { + if (batches.isEmpty()) { return; } @@ -343,49 +361,66 @@ public void performPendingRetransforms() { // spring-boot-starter-web) whose pom.properties is not in the class's own JAR. // Without pre-warming classpathArtifactCache, this scans all java.class.path JARs via // resolveDependencies() under JVM locks, reintroducing the snakeyaml deadlock. - for (Class c : toRetransform) { - ProtectionDomain pd = c.getProtectionDomain(); - if (pd == null) { - continue; - } - CodeSource cs = pd.getCodeSource(); - if (cs == null) { - continue; - } - URL loc = cs.getLocation(); - if (loc == null) { - continue; - } - resolveDependencies(loc); - String internalName = c.getName().replace('.', '/'); - List dbEntries = database.entriesForClass(internalName); - if (dbEntries != null) { - List classJarDeps = resolveDependenciesFromCache(loc); - for (ScaEntry entry : dbEntries) { - resolveVersionForArtifact(entry.artifact(), classJarDeps); + for (List> b : batches) { + for (Class c : b) { + ProtectionDomain pd = c.getProtectionDomain(); + if (pd == null) { + continue; + } + CodeSource cs = pd.getCodeSource(); + if (cs == null) { + continue; + } + URL loc = cs.getLocation(); + if (loc == null) { + continue; + } + resolveDependencies(loc); + String internalName = c.getName().replace('.', '/'); + List dbEntries = database.entriesForClass(internalName); + if (dbEntries != null) { + List classJarDeps = resolveDependenciesFromCache(loc); + for (ScaEntry entry : dbEntries) { + resolveVersionForArtifact(entry.artifact(), classJarDeps); + } } } } + // Each batch is retransformed independently: a failure in one batch never affects another, + // and a failing multi-class batch is bisected rather than dropped as a whole. + for (List> b : batches) { + retransformBatch(b); + } + } + + /** + * Attempts {@code retransformClasses()} for a single batch. On failure, a multi-class batch is + * bisected into two halves that are re-queued as independent batches for the next heartbeat — + * never retried within this same call — so that a healthy class is progressively isolated from an + * unrelated, permanently-failing batch-mate instead of being dropped alongside it. Once a batch + * is down to a single class, further failures of that class are bounded by {@link + * #retransformFailureCount} and {@link #MAX_RETRANSFORM_ATTEMPTS} to guarantee it is eventually + * dropped instead of leaking its {@code Class} forever. + */ + private void retransformBatch(List> batch) { try { - instrumentation.retransformClasses(toRetransform.toArray(new Class[0])); + instrumentation.retransformClasses(batch.toArray(new Class[0])); log.debug( - "SCA Reachability: retransformed {} class(es) for method-level detection", - toRetransform.size()); - for (Class c : toRetransform) { + "SCA Reachability: retransformed {} class(es) for method-level detection", batch.size()); + for (Class c : batch) { retransformFailureCount.remove(c); } } catch (Throwable t) { - log.debug("SCA Reachability: retransformClasses failed", t); - // retransformClasses() is atomic for the whole array, so a single unrelated failure (this - // transformer's own bytecode, another registered transformer, or a transient JVM issue) - // fails every class in the batch. Re-queue each class individually, up to a bounded number - // of attempts, so a class that keeps failing is eventually dropped instead of retaining its - // Class (and its ClassLoader) forever. - for (Class c : toRetransform) { + log.debug( + "SCA Reachability: retransformClasses failed for a batch of {} class(es)", + batch.size(), + t); + if (batch.size() == 1) { + Class c = batch.get(0); int attempts = retransformFailureCount.merge(c, 1, Integer::sum); if (attempts < MAX_RETRANSFORM_ATTEMPTS) { - pendingRetransform.add(c); + pendingRetransform.add(batch); } else { retransformFailureCount.remove(c); log.debug( @@ -393,6 +428,14 @@ public void performPendingRetransforms() { c.getName(), attempts); } + } else { + int mid = batch.size() / 2; + pendingRetransform.add(new ArrayList<>(batch.subList(0, mid))); + pendingRetransform.add(new ArrayList<>(batch.subList(mid, batch.size()))); + log.debug( + "SCA Reachability: bisecting failing batch of {} class(es) into two halves for the" + + " next heartbeat", + batch.size()); } } } diff --git a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java index b1ca66d7f05..40692eb8bfb 100644 --- a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java +++ b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java @@ -3,6 +3,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -10,6 +11,9 @@ import java.io.StringReader; import java.lang.instrument.Instrumentation; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import org.junit.jupiter.api.Test; /** @@ -80,7 +84,8 @@ void performPendingRetransforms_requeuesOnRetransformFailure() throws Exception assertEquals( 2, t.pendingRetransform.size(), - "both classes must be re-queued in pendingRetransform for the next heartbeat retry"); + "the failing batch of 2 must be bisected into 2 singleton batches for the next" + + " heartbeat retry"); } @Test @@ -96,7 +101,7 @@ void performPendingRetransforms_givesUpAfterMaxFailedAttempts() throws Exception ScaCveDatabase db = ScaCveDatabase.parse(new StringReader("{\"version\":1,\"entries\":[]}")); ScaReachabilityTransformer t = new ScaReachabilityTransformer(db, instr); - t.pendingRetransform.add(Target.class); + t.pendingRetransform.add(Collections.singletonList(Target.class)); // Each failed attempt re-queues the class for the next heartbeat; after the limit it must be // dropped instead. Run one extra iteration beyond the limit to confirm it stays dropped. @@ -125,7 +130,7 @@ void performPendingRetransforms_clearsFailureCountOnEventualSuccess() throws Exc ScaCveDatabase db = ScaCveDatabase.parse(new StringReader("{\"version\":1,\"entries\":[]}")); ScaReachabilityTransformer t = new ScaReachabilityTransformer(db, instr); - t.pendingRetransform.add(Target.class); + t.pendingRetransform.add(Collections.singletonList(Target.class)); t.performPendingRetransforms(); // fails once, re-queued assertEquals(1, t.pendingRetransform.size()); @@ -138,38 +143,47 @@ void performPendingRetransforms_clearsFailureCountOnEventualSuccess() throws Exc } @Test - void performPendingRetransforms_dropsBothClassesWhenBatchedWithAPermanentlyFailingOne() + void performPendingRetransforms_isolatesHealthyClassFromPermanentlyFailingBatchMateViaBisection() throws Exception { - // A batch containing one permanently-failing class and one otherwise-healthy class fails - // atomically on every attempt while both are queued together, so both accumulate the same - // failure count and are dropped in the same call once MAX_RETRANSFORM_ATTEMPTS is reached — - // the healthy class is never isolated from its unrelated batch-mate and never retransforms. - // This documents a real limitation of per-class attempt counting: it bounds retention (no - // Metaspace leak) but does not guarantee an unrelated healthy class in the same batch is ever - // successfully retransformed. + // A batch containing one permanently-failing class and one otherwise-healthy class fails once + // as a pair. Instead of tying both classes' failure counts together, the batch is immediately + // bisected into two singleton batches, deferred to the next heartbeat. On the next heartbeat + // the healthy class retransforms successfully on its own, isolated from its unrelated, + // permanently-failing batch-mate. Class poison = Target.class; Class healthy = Other.class; Instrumentation instr = mock(Instrumentation.class); when(instr.isModifiableClass(poison)).thenReturn(true); when(instr.isModifiableClass(healthy)).thenReturn(true); - doThrow(new RuntimeException("retransform failed")).when(instr).retransformClasses(any()); + doAnswer( + invocation -> { + // Mockito flattens the varargs invocation, so getArguments() yields the individual + // Class elements rather than the backing array. + for (Object arg : invocation.getArguments()) { + if (arg == poison) { + throw new RuntimeException("retransform failed"); + } + } + return null; + }) + .when(instr) + .retransformClasses(any()); ScaCveDatabase db = ScaCveDatabase.parse(new StringReader("{\"version\":1,\"entries\":[]}")); ScaReachabilityTransformer t = new ScaReachabilityTransformer(db, instr); - t.pendingRetransform.add(poison); - t.pendingRetransform.add(healthy); + t.pendingRetransform.add(new ArrayList<>(Arrays.asList(poison, healthy))); - for (int attempt = 1; attempt <= 6; attempt++) { - t.performPendingRetransforms(); - } + t.performPendingRetransforms(); // batch of 2 fails, bisects into [poison] and [healthy] + t.performPendingRetransforms(); // [healthy] succeeds alone; [poison] keeps failing alone + verify(instr).retransformClasses(healthy); assertTrue( - t.pendingRetransform.isEmpty(), - "both classes must be dropped once the shared batch exceeds the retry limit"); + t.pendingRetransform.stream().noneMatch(batch -> batch.contains(healthy)), + "healthy class must be successfully retransformed and not remain queued"); assertTrue( - t.retransformFailureCount.isEmpty(), - "failure counts for both classes must be cleared once dropped"); + t.retransformFailureCount.containsKey(poison), + "poison class must still be tracked as a failing singleton batch"); } @Test diff --git a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java index 48bfa48b1a9..785eea16d5a 100644 --- a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java +++ b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java @@ -177,7 +177,8 @@ void performPendingRetransforms_prewarms_jarCache_before_retransformClasses() th ScaCveDatabase db = ScaCveDatabase.parse(new StringReader(JACKSON_JSON)); ScaReachabilityTransformer transformer = new ScaReachabilityTransformer(db, mockInstr); - transformer.pendingRetransform.add(com.fasterxml.jackson.databind.ObjectMapper.class); + transformer.pendingRetransform.add( + singletonList(com.fasterxml.jackson.databind.ObjectMapper.class)); transformer.performPendingRetransforms(); assertFalse( @@ -223,7 +224,8 @@ void performPendingRetransforms_prewarms_classpathArtifactCache_for_aggregator_a ScaCveDatabase db = ScaCveDatabase.parse(new StringReader(crossJarJson)); ScaReachabilityTransformer transformer = new ScaReachabilityTransformer(db, mockInstr); - transformer.pendingRetransform.add(com.fasterxml.jackson.databind.ObjectMapper.class); + transformer.pendingRetransform.add( + singletonList(com.fasterxml.jackson.databind.ObjectMapper.class)); transformer.performPendingRetransforms(); // jackson-core is on the test classpath (transitive dependency of jackson-databind). @@ -307,7 +309,8 @@ void checkAlreadyLoadedClassesSchedulesOnlyMatchingNonBootstrapClasses() throws assertEquals(1, transformer.pendingRetransform.size()); assertSame( - ScaReachabilityMethodLevelTest.TargetClass.class, transformer.pendingRetransform.peek()); + ScaReachabilityMethodLevelTest.TargetClass.class, + transformer.pendingRetransform.peek().get(0)); } @Test @@ -315,12 +318,14 @@ void performPendingRetransforms_noopsWithoutInstrumentation() throws Exception { ScaReachabilityTransformer transformer = new ScaReachabilityTransformer( ScaCveDatabase.parse(new StringReader("{\"version\":1,\"entries\":[]}")), null); - transformer.pendingRetransform.add(ScaReachabilityMethodLevelTest.TargetClass.class); + transformer.pendingRetransform.add( + singletonList(ScaReachabilityMethodLevelTest.TargetClass.class)); transformer.performPendingRetransforms(); assertSame( - ScaReachabilityMethodLevelTest.TargetClass.class, transformer.pendingRetransform.peek()); + ScaReachabilityMethodLevelTest.TargetClass.class, + transformer.pendingRetransform.peek().get(0)); } @Test From 46d93b7f9daf7a4b87948885d0563e2d77972913 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 21 Jul 2026 11:53:58 +0200 Subject: [PATCH 3/6] 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. --- .../sca/ScaReachabilityTransformer.java | 40 +++----------- .../sca/ScaReachabilityRetransformTest.java | 52 ++++++------------- 2 files changed, 24 insertions(+), 68 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java index 82c81d975ac..67ce2df5a8b 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java @@ -96,21 +96,6 @@ public final class ScaReachabilityTransformer implements ClassFileTransformer { /** Class names (internal format) queued for deferred retransformation by name lookup. */ @VisibleForTesting final Set pendingRetransformNames = ConcurrentHashMap.newKeySet(); - /** - * {@code retransformClasses()} is atomic for a given batch: if any single class in it fails to - * verify/redefine, the whole batch throws. For a multi-class batch, {@link - * #performPendingRetransforms()} reacts by bisecting it into two halves, deferred to the next - * heartbeat, to isolate the failing class(es) from any healthy batch-mates. Once a batch has been - * narrowed down to a single class, further failures of that same class are tracked here so it is - * dropped after {@link #MAX_RETRANSFORM_ATTEMPTS} instead of being retried forever and leaking - * its {@code Class} (and {@code ClassLoader}) in Metaspace. Entries are removed on success or - * once the limit is reached, so this map only grows with currently-failing singleton classes. - */ - @VisibleForTesting - final Map, Integer> retransformFailureCount = new ConcurrentHashMap<>(); - - private static final int MAX_RETRANSFORM_ATTEMPTS = 5; - public ScaReachabilityTransformer(ScaCveDatabase database, Instrumentation instrumentation) { this.database = database; this.instrumentation = instrumentation; @@ -399,35 +384,26 @@ public void performPendingRetransforms() { * bisected into two halves that are re-queued as independent batches for the next heartbeat — * never retried within this same call — so that a healthy class is progressively isolated from an * unrelated, permanently-failing batch-mate instead of being dropped alongside it. Once a batch - * is down to a single class, further failures of that class are bounded by {@link - * #retransformFailureCount} and {@link #MAX_RETRANSFORM_ATTEMPTS} to guarantee it is eventually - * dropped instead of leaking its {@code Class} forever. + * is down to a single class, a further failure of that class means it has already been proven to + * be the actual cause (or, for a class that started as a singleton, that its very first attempt + * failed): it is dropped immediately rather than retried, so it never leaks its {@code Class} + * (and {@code ClassLoader}) in Metaspace. See APPSEC-69201 for the trade-off this implies. */ private void retransformBatch(List> batch) { try { instrumentation.retransformClasses(batch.toArray(new Class[0])); log.debug( "SCA Reachability: retransformed {} class(es) for method-level detection", batch.size()); - for (Class c : batch) { - retransformFailureCount.remove(c); - } } catch (Throwable t) { log.debug( "SCA Reachability: retransformClasses failed for a batch of {} class(es)", batch.size(), t); if (batch.size() == 1) { - Class c = batch.get(0); - int attempts = retransformFailureCount.merge(c, 1, Integer::sum); - if (attempts < MAX_RETRANSFORM_ATTEMPTS) { - pendingRetransform.add(batch); - } else { - retransformFailureCount.remove(c); - log.debug( - "SCA Reachability: giving up retransforming {} after {} failed attempts", - c.getName(), - attempts); - } + log.debug( + "SCA Reachability: giving up retransforming {} after a failed attempt as a singleton" + + " batch", + batch.get(0).getName()); } else { int mid = batch.size() / 2; pendingRetransform.add(new ArrayList<>(batch.subList(0, mid))); diff --git a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java index 40692eb8bfb..6948bf509ff 100644 --- a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java +++ b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java @@ -89,12 +89,12 @@ void performPendingRetransforms_requeuesOnRetransformFailure() throws Exception } @Test - void performPendingRetransforms_givesUpAfterMaxFailedAttempts() throws Exception { + void performPendingRetransforms_dropsSingletonBatchImmediatelyOnFailure() throws Exception { // retransformClasses() is atomic for the whole array: an unrelated, permanently-failing class // (this test simulates it with a RuntimeException on every attempt) must not be re-queued // forever, or its Class (and its ClassLoader) is retained indefinitely — the Metaspace leak - // from APPSEC-69201. After MAX_RETRANSFORM_ATTEMPTS failures the class must be dropped instead - // of re-queued. + // from APPSEC-69201. Once a batch is down to a single class, a failure is treated as proof the + // class itself is the cause and it is dropped on the spot, with no retry budget. Instrumentation instr = mock(Instrumentation.class); when(instr.isModifiableClass(Target.class)).thenReturn(true); doThrow(new RuntimeException("retransform failed")).when(instr).retransformClasses(any()); @@ -103,53 +103,35 @@ void performPendingRetransforms_givesUpAfterMaxFailedAttempts() throws Exception ScaReachabilityTransformer t = new ScaReachabilityTransformer(db, instr); t.pendingRetransform.add(Collections.singletonList(Target.class)); - // Each failed attempt re-queues the class for the next heartbeat; after the limit it must be - // dropped instead. Run one extra iteration beyond the limit to confirm it stays dropped. - for (int attempt = 1; attempt <= 6; attempt++) { - t.performPendingRetransforms(); - } + t.performPendingRetransforms(); assertTrue( - t.pendingRetransform.isEmpty(), - "class must be dropped (not re-queued) once the retry limit is exceeded"); - assertTrue( - t.retransformFailureCount.isEmpty(), - "failure count must be cleared once the class is dropped, so the map does not grow" - + " unbounded"); + t.pendingRetransform.isEmpty(), "class must be dropped (not re-queued) after one failure"); } @Test - void performPendingRetransforms_clearsFailureCountOnEventualSuccess() throws Exception { + void performPendingRetransforms_doesNotRequeueSingletonBatchOnSuccess() throws Exception { Instrumentation instr = mock(Instrumentation.class); when(instr.isModifiableClass(Target.class)).thenReturn(true); - doThrow(new RuntimeException("retransform failed")) - .doNothing() - .when(instr) - .retransformClasses(any()); ScaCveDatabase db = ScaCveDatabase.parse(new StringReader("{\"version\":1,\"entries\":[]}")); ScaReachabilityTransformer t = new ScaReachabilityTransformer(db, instr); t.pendingRetransform.add(Collections.singletonList(Target.class)); - t.performPendingRetransforms(); // fails once, re-queued - assertEquals(1, t.pendingRetransform.size()); - - t.performPendingRetransforms(); // succeeds this time + t.performPendingRetransforms(); + verify(instr).retransformClasses(Target.class); assertTrue(t.pendingRetransform.isEmpty(), "class must not be re-queued after success"); - assertTrue( - t.retransformFailureCount.isEmpty(), - "failure count must be cleared once retransformClasses succeeds"); } @Test void performPendingRetransforms_isolatesHealthyClassFromPermanentlyFailingBatchMateViaBisection() throws Exception { // A batch containing one permanently-failing class and one otherwise-healthy class fails once - // as a pair. Instead of tying both classes' failure counts together, the batch is immediately - // bisected into two singleton batches, deferred to the next heartbeat. On the next heartbeat - // the healthy class retransforms successfully on its own, isolated from its unrelated, - // permanently-failing batch-mate. + // as a pair. Rather than tying both classes together, the batch is immediately bisected into + // two singleton batches, deferred to the next heartbeat. On the next heartbeat the healthy + // class retransforms successfully on its own, isolated from its unrelated, permanently-failing + // batch-mate — which is dropped on the spot since it fails again alone. Class poison = Target.class; Class healthy = Other.class; @@ -175,15 +157,13 @@ void performPendingRetransforms_isolatesHealthyClassFromPermanentlyFailingBatchM t.pendingRetransform.add(new ArrayList<>(Arrays.asList(poison, healthy))); t.performPendingRetransforms(); // batch of 2 fails, bisects into [poison] and [healthy] - t.performPendingRetransforms(); // [healthy] succeeds alone; [poison] keeps failing alone + t.performPendingRetransforms(); // [healthy] succeeds alone; [poison] fails alone and is dropped verify(instr).retransformClasses(healthy); assertTrue( - t.pendingRetransform.stream().noneMatch(batch -> batch.contains(healthy)), - "healthy class must be successfully retransformed and not remain queued"); - assertTrue( - t.retransformFailureCount.containsKey(poison), - "poison class must still be tracked as a failing singleton batch"); + t.pendingRetransform.isEmpty(), + "healthy class must be successfully retransformed and the poison class dropped, leaving" + + " nothing queued"); } @Test From 1f1b4a3a0aec075318b36c8be6edf562f04489bc Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 21 Jul 2026 12:17:13 +0200 Subject: [PATCH 4/6] 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. --- .../sca/ScaReachabilityTransformer.java | 18 +++++--- .../ScaReachabilityTransformerJava9Test.java | 45 +++++++++++++++++++ 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java index 67ce2df5a8b..0080cb1774e 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java @@ -80,8 +80,8 @@ public final class ScaReachabilityTransformer implements ClassFileTransformer { * Batches of classes whose bytecode needs (re)transformation for method-level symbol injection: * *

    - *
  • Classes already loaded at startup before this transformer was registered (queued as - * singleton batches). + *
  • Classes already loaded at startup before this transformer was registered (queued as a + * single shared batch by {@link #checkAlreadyLoadedClasses()}). *
  • Batches that failed {@code retransformClasses()} and were bisected into halves (see * {@link #performPendingRetransforms()}). *
@@ -246,6 +246,7 @@ private byte[] processClass( * would produce false positives if used as reachability proxies. See APPSEC-62260. */ public void checkAlreadyLoadedClasses() { + List> toRetransform = new ArrayList<>(); for (Class clazz : instrumentation.getAllLoadedClasses()) { if (clazz == null) { continue; @@ -267,9 +268,16 @@ public void checkAlreadyLoadedClasses() { } // All symbols are method-level: always schedule retransformation so the bytecode // callback can be injected. We can't modify bytecode during the startup scan; deferred - // to performPendingRetransforms(). Queued as its own singleton batch so a failure never - // couples it to an unrelated class. - pendingRetransform.add(Collections.singletonList(clazz)); + // to performPendingRetransforms(). + toRetransform.add(clazz); + } + if (!toRetransform.isEmpty()) { + // Queued as a single shared batch: the common case is that all of these classes retransform + // successfully together, so this keeps the startup scan to one retransformClasses() call + // instead of one per already-loaded class. If the batch does fail, + // performPendingRetransforms() + // bisects it on the next heartbeat like any other batch. + pendingRetransform.add(toRetransform); } } diff --git a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java index 785eea16d5a..e47e19568be 100644 --- a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java +++ b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java @@ -51,6 +51,11 @@ */ class ScaReachabilityTransformerJava9Test { + /** Second dummy vulnerable class, distinct from {@code TargetClass}, used to test batching. */ + public static class SecondTargetClass { + public void method() {} + } + private static final String JACKSON_JSON = "{\"version\":1,\"entries\":[{" + "\"vuln_id\":\"GHSA-test-jackson\"," @@ -313,6 +318,46 @@ void checkAlreadyLoadedClassesSchedulesOnlyMatchingNonBootstrapClasses() throws transformer.pendingRetransform.peek().get(0)); } + @Test + void checkAlreadyLoadedClasses_queuesAllMatchingClassesAsOneSharedBatch() throws Exception { + // Startup can find many already-loaded vulnerable classes at once. Queuing each as its own + // singleton batch would turn the common (all-succeed) case into one retransformClasses() call + // per class instead of one call for all of them; bisection only needs to kick in if this shared + // batch actually fails on a later heartbeat. + String targetName = + ScaReachabilityMethodLevelTest.TargetClass.class.getName().replace('.', '/'); + String secondName = SecondTargetClass.class.getName().replace('.', '/'); + String json = + "{\"version\":1,\"entries\":[" + + "{\"vuln_id\":\"GHSA-target\",\"artifact\":\"com.example:lib\"," + + "\"version_ranges\":[\"< 999.0.0\"]," + + "\"symbols\":[{\"class\":\"" + + targetName + + "\",\"method\":\"vulnerableMethod\"}]}," + + "{\"vuln_id\":\"GHSA-second\",\"artifact\":\"com.example:lib2\"," + + "\"version_ranges\":[\"< 999.0.0\"]," + + "\"symbols\":[{\"class\":\"" + + secondName + + "\",\"method\":\"method\"}]}" + + "]}"; + + Instrumentation mockInstr = mock(Instrumentation.class); + when(mockInstr.getAllLoadedClasses()) + .thenReturn( + new Class[] { + ScaReachabilityMethodLevelTest.TargetClass.class, SecondTargetClass.class, + }); + ScaReachabilityTransformer transformer = + new ScaReachabilityTransformer(ScaCveDatabase.parse(new StringReader(json)), mockInstr); + + transformer.checkAlreadyLoadedClasses(); + + assertEquals(1, transformer.pendingRetransform.size(), "both classes must share one batch"); + assertEquals( + Arrays.asList(ScaReachabilityMethodLevelTest.TargetClass.class, SecondTargetClass.class), + transformer.pendingRetransform.peek()); + } + @Test void performPendingRetransforms_noopsWithoutInstrumentation() throws Exception { ScaReachabilityTransformer transformer = From aa55d83ba312b0680997ce435e8a9fbf5b75bde7 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 21 Jul 2026 12:42:22 +0200 Subject: [PATCH 5/6] 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. --- .../sca/ScaReachabilityMethodLevelTest.java | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityMethodLevelTest.java b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityMethodLevelTest.java index 78a2fa98631..a1815dc22e6 100644 --- a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityMethodLevelTest.java +++ b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityMethodLevelTest.java @@ -384,6 +384,98 @@ public void visitInvokeDynamicInsn(String n, String d, Handle h, Object... args) return first[0]; } + /** + * Fixture exercising the instruction kinds {@link ScaMethodCallbackInjector.MethodEntryInjector} + * must forward to the delegate unchanged: BIPUSH ({@code visitIntInsn}), a conditional jump + * ({@code visitJumpInsn}), a loop increment ({@code visitIincInsn}), a dense switch ({@code + * visitTableSwitchInsn}), a sparse switch ({@code visitLookupSwitchInsn}) and a 2D array + * allocation ({@code visitMultiANewArrayInsn}). + */ + public static class OpcodeCoverageMethods { + public static int intInsn() { + return 100; + } + + public static int jumpInsn(boolean flag) { + if (flag) { + return 1; + } + return 2; + } + + public static int iincInsn() { + int i = 0; + i++; + return i; + } + + public static int tableSwitchInsn(int x) { + switch (x) { + case 0: + return 1; + case 1: + return 2; + case 2: + return 3; + default: + return -1; + } + } + + public static int lookupSwitchInsn(int x) { + switch (x) { + case 0: + return 1; + case 100: + return 2; + case 10000: + return 3; + default: + return -1; + } + } + + public static int[][] multiANewArrayInsn() { + return new int[2][3]; + } + } + + @Test + void inject_preservesBehaviorForAllInstructionKinds() throws Exception { + // Coverage for MethodEntryInjector's visitIntInsn/visitJumpInsn/visitIincInsn/ + // visitTableSwitchInsn/visitLookupSwitchInsn/visitMultiANewArrayInsn overrides: each must + // forward to the delegate unchanged so the original method body still executes correctly + // once the entry callback is spliced in. + byte[] original = bytecodeOf(OpcodeCoverageMethods.class); + Map> callbacks = new HashMap<>(); + for (String methodName : + new String[] { + "intInsn", + "jumpInsn", + "iincInsn", + "tableSwitchInsn", + "lookupSwitchInsn", + "multiANewArrayInsn" + }) { + callbacks.put( + methodName, + Collections.singletonList( + spec("GHSA-" + methodName, "com.example:lib", "1.0.0", "T", methodName))); + } + + Class cls = loadModified(ScaMethodCallbackInjector.inject(original, callbacks)); + + assertEquals(100, cls.getMethod("intInsn").invoke(null)); + assertEquals(1, cls.getMethod("jumpInsn", boolean.class).invoke(null, true)); + assertEquals(2, cls.getMethod("jumpInsn", boolean.class).invoke(null, false)); + assertEquals(1, cls.getMethod("iincInsn").invoke(null)); + assertEquals(2, cls.getMethod("tableSwitchInsn", int.class).invoke(null, 1)); + assertEquals(2, cls.getMethod("lookupSwitchInsn", int.class).invoke(null, 100)); + assertEquals(2, ((int[][]) cls.getMethod("multiANewArrayInsn").invoke(null)).length); + + assertEquals(6, drainHits().size(), "each instrumented method fires its own callback"); + } + // --------------------------------------------------------------------------- // transform(): two-phase design — first load enqueues, retransform injects // --------------------------------------------------------------------------- From adf315f8009b74c9354877a0f3d804ace7be7f5a Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Wed, 22 Jul 2026 12:30:15 +0200 Subject: [PATCH 6/6] 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. --- .../appsec/sca/ScaReachabilityTransformer.java | 11 +++-------- .../appsec/sca/ScaReachabilityRetransformTest.java | 4 ++-- .../sca/ScaReachabilityTransformerJava9Test.java | 5 +++-- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java index 0080cb1774e..2a28aff95c4 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java @@ -305,14 +305,9 @@ public void performPendingRetransforms() { List>> batches = new ArrayList<>(); List> batch; while ((batch = pendingRetransform.poll()) != null) { - List> modifiable = new ArrayList<>(batch.size()); - for (Class c : batch) { - if (instrumentation.isModifiableClass(c)) { - modifiable.add(c); - } - } - if (!modifiable.isEmpty()) { - batches.add(modifiable); + batch.removeIf(c -> !instrumentation.isModifiableClass(c)); + if (!batch.isEmpty()) { + batches.add(batch); } } diff --git a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java index 6948bf509ff..48d20b2762d 100644 --- a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java +++ b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityRetransformTest.java @@ -101,7 +101,7 @@ void performPendingRetransforms_dropsSingletonBatchImmediatelyOnFailure() throws ScaCveDatabase db = ScaCveDatabase.parse(new StringReader("{\"version\":1,\"entries\":[]}")); ScaReachabilityTransformer t = new ScaReachabilityTransformer(db, instr); - t.pendingRetransform.add(Collections.singletonList(Target.class)); + t.pendingRetransform.add(new ArrayList<>(Collections.singletonList(Target.class))); t.performPendingRetransforms(); @@ -117,7 +117,7 @@ void performPendingRetransforms_doesNotRequeueSingletonBatchOnSuccess() throws E ScaCveDatabase db = ScaCveDatabase.parse(new StringReader("{\"version\":1,\"entries\":[]}")); ScaReachabilityTransformer t = new ScaReachabilityTransformer(db, instr); - t.pendingRetransform.add(Collections.singletonList(Target.class)); + t.pendingRetransform.add(new ArrayList<>(Collections.singletonList(Target.class))); t.performPendingRetransforms(); verify(instr).retransformClasses(Target.class); diff --git a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java index e47e19568be..573424432ad 100644 --- a/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java +++ b/dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java @@ -18,6 +18,7 @@ import java.io.StringReader; import java.lang.instrument.Instrumentation; import java.net.URLClassLoader; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; import org.junit.jupiter.api.AfterEach; @@ -183,7 +184,7 @@ void performPendingRetransforms_prewarms_jarCache_before_retransformClasses() th ScaReachabilityTransformer transformer = new ScaReachabilityTransformer(db, mockInstr); transformer.pendingRetransform.add( - singletonList(com.fasterxml.jackson.databind.ObjectMapper.class)); + new ArrayList<>(singletonList(com.fasterxml.jackson.databind.ObjectMapper.class))); transformer.performPendingRetransforms(); assertFalse( @@ -230,7 +231,7 @@ void performPendingRetransforms_prewarms_classpathArtifactCache_for_aggregator_a ScaReachabilityTransformer transformer = new ScaReachabilityTransformer(db, mockInstr); transformer.pendingRetransform.add( - singletonList(com.fasterxml.jackson.databind.ObjectMapper.class)); + new ArrayList<>(singletonList(com.fasterxml.jackson.databind.ObjectMapper.class))); transformer.performPendingRetransforms(); // jackson-core is on the test classpath (transitive dependency of jackson-databind).