From 209973c00a334a7f0793ec2bc04902f25c6e631f Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Wed, 19 Aug 2026 13:41:27 -0400 Subject: [PATCH 1/3] Guard rethrowIfBlockingException call against unexpected failures BlockingExceptionHandler.rethrowIfBlockingException was invoked outside the try/catch that guards the rest of the instrumentation exception handler, so any unexpected Throwable from that call itself (e.g. a NoClassDefFoundError from a classloader that can't see the appsec module) would escape uncaught, replacing the original swallowed exception and crashing the instrumented method's caller instead of being logged. Move the call inside the try/catch and only rethrow when the caught exception is actually a BlockingException. Co-Authored-By: Claude Sonnet 5 --- .../tooling/bytebuddy/ExceptionHandlers.java | 33 ++++++++++++++++--- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java index 5de9c8630ca..57578d2e822 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java @@ -49,24 +49,37 @@ public Size apply(final MethodVisitor mv, final Implementation.Context context) // // Emits the following Java-equivalent code when exitOnFailure is false: // - // BlockingExceptionHandler.rethrowIfBlockingException(t); // try { + // BlockingExceptionHandler.rethrowIfBlockingException(t); // InstrumentationErrors.recordError(); // org.slf4j.LoggerFactory.getLogger((Class) ExceptionLogger.class) // .debug("Failed to handle exception in instrumentation for (" + adviceName + // ")", t); // } catch (Throwable t2) { + // if (t2 instanceof BlockingException) throw t2; // } // // and the same with .error(...) followed by System.exit(1) when exitOnFailure is true. + // + // rethrowIfBlockingException is inside the try/catch (rather than called bare, as + // before) so that an unexpected failure resolving/invoking it - e.g. a + // NoClassDefFoundError from a classloader that can't see the appsec module - is + // swallowed like any other instrumentation error instead of replacing the original + // exception and escaping into the instrumented method's caller. The catch handler + // re-throws only when the caught exception really is the BlockingException the call + // was meant to propagate. final Label logStart = new Label(); final Label logEnd = new Label(); final Label eatException = new Label(); + final Label notBlocking = new Label(); final Label handlerExit = new Label(); // Frames are only meaningful for class files in version 6 or later. final boolean frames = context.getClassFileVersion().isAtLeast(ClassFileVersion.JAVA_V6); + mv.visitTryCatchBlock(logStart, logEnd, eatException, "java/lang/Throwable"); + mv.visitLabel(logStart); + if (appSecEnabled) { // Need throwable on top for rethrowIfBlockingException. // stack: (top) adviceName, throwable -> top throwable @@ -81,8 +94,6 @@ public Size apply(final MethodVisitor mv, final Implementation.Context context) mv.visitInsn(Opcodes.SWAP); } - mv.visitTryCatchBlock(logStart, logEnd, eatException, "java/lang/Throwable"); - mv.visitLabel(logStart); // record instrumentation error if (detailedErrors) { // recordError(Throwable) needs throwable on top, then we restore. @@ -148,12 +159,24 @@ public Size apply(final MethodVisitor mv, final Implementation.Context context) mv.visitLabel(logEnd); mv.visitJumpInsn(Opcodes.GOTO, handlerExit); - // if the runtime can't reach our ExceptionHandler or logger, - // silently eat the exception + // If the runtime can't reach our ExceptionHandler or logger, or + // rethrowIfBlockingException itself failed unexpectedly, silently eat the exception - + // unless it's the BlockingException rethrowIfBlockingException was meant to propagate, + // in which case let it through. mv.visitLabel(eatException); if (frames) { mv.visitFrame(Opcodes.F_SAME1, 0, null, 1, new Object[] {"java/lang/Throwable"}); } + if (appSecEnabled) { + mv.visitInsn(Opcodes.DUP); + mv.visitTypeInsn(Opcodes.INSTANCEOF, "datadog/appsec/api/blocking/BlockingException"); + mv.visitJumpInsn(Opcodes.IFEQ, notBlocking); + mv.visitInsn(Opcodes.ATHROW); + mv.visitLabel(notBlocking); + if (frames) { + mv.visitFrame(Opcodes.F_SAME1, 0, null, 1, new Object[] {"java/lang/Throwable"}); + } + } mv.visitInsn(Opcodes.POP); // mv.visitMethodInsn(Opcodes.INVOKEVIRTUAL, "java/lang/Throwable", // "printStackTrace", "()V", false); From 53c967f223990f7fde68558e6a023e8b7de2445a Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 21 Aug 2026 10:24:25 -0400 Subject: [PATCH 2/3] Add failing regression test for BlockingException resolution in eatException handler Documents the gap flagged by codex on PR #12240: the eatException catch block's own `instanceof BlockingException` check resolves the class via the instrumented class's own classloader, so a classloader that can't see the appsec module gets a NoClassDefFoundError instead of the original exception. Left failing intentionally; not pushed until a fix is decided. Co-Authored-By: Claude Sonnet 5 --- .../test/BaseExceptionHandlerTest.groovy | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/BaseExceptionHandlerTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/BaseExceptionHandlerTest.groovy index c2877d03868..9fe24a4e167 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/BaseExceptionHandlerTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/BaseExceptionHandlerTest.groovy @@ -143,6 +143,31 @@ abstract class BaseExceptionHandlerTest extends DDSpecification { exitStatus.get() == 0 } + def "exception on classloader that cannot resolve BlockingException"() { + setup: + int initLogEvents = testAppender.list.size() + URL[] classpath = [ + SomeClass.getProtectionDomain().getCodeSource().getLocation(), + GroovyObject.getProtectionDomain().getCodeSource().getLocation(), + ] + // Fully isolated loader: unlike BlockingTestClassLoader, this does NOT special-case + // BlockingExceptionHandler/BlockingException, so neither is resolvable from here. + URLClassLoader loader = new URLClassLoader(classpath, null, null) + + when: + loader.loadClass(BlockingException.getName()) + then: + thrown ClassNotFoundException + + when: + Class someClazz = loader.loadClass(SomeClass.getName()) + someClazz.getMethod("isInstrumented").invoke(null) + then: + noExceptionThrown() + testAppender.list.size() == initLogEvents + 1 + exitStatus.get() == 0 + } + def "exception handler sets the correct stack size"() { when: SomeClass.smallStack() From c1f1295025599396e4c2e55f520895996f1d11cf Mon Sep 17 00:00:00 2001 From: Douglas Q Hawkins Date: Fri, 21 Aug 2026 11:38:51 -0400 Subject: [PATCH 3/3] Fix eatException handler resolving BlockingException via instanceof Replace the INSTANCEOF check with a class-name comparison so the catch handler no longer forces resolution of BlockingException via the instrumented class's own classloader - the same NoClassDefFoundError risk the try/catch exists to guard against. Co-Authored-By: Claude Sonnet 5 --- .../tooling/bytebuddy/ExceptionHandlers.java | 30 +++++++++++++-- .../test/BaseExceptionHandlerTest.groovy | 31 +++++++++++++--- .../test/AppSecInvisibleClassLoader.java | 37 +++++++++++++++++++ 3 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/test/AppSecInvisibleClassLoader.java diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java index 57578d2e822..cb766ad9562 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java @@ -56,7 +56,10 @@ public Size apply(final MethodVisitor mv, final Implementation.Context context) // .debug("Failed to handle exception in instrumentation for (" + adviceName + // ")", t); // } catch (Throwable t2) { - // if (t2 instanceof BlockingException) throw t2; + // if (t2.getClass().getName().equals("datadog.appsec.api.blocking.BlockingException")) + // { + // throw t2; + // } // } // // and the same with .error(...) followed by System.exit(1) when exitOnFailure is true. @@ -67,7 +70,10 @@ public Size apply(final MethodVisitor mv, final Implementation.Context context) // swallowed like any other instrumentation error instead of replacing the original // exception and escaping into the instrumented method's caller. The catch handler // re-throws only when the caught exception really is the BlockingException the call - // was meant to propagate. + // was meant to propagate. It compares by class name rather than using `instanceof` + // because `instanceof` would itself need to resolve BlockingException via the + // instrumented class's own classloader - the same NoClassDefFoundError risk this whole + // try/catch exists to guard against. final Label logStart = new Label(); final Label logEnd = new Label(); final Label eatException = new Label(); @@ -168,8 +174,26 @@ public Size apply(final MethodVisitor mv, final Implementation.Context context) mv.visitFrame(Opcodes.F_SAME1, 0, null, 1, new Object[] {"java/lang/Throwable"}); } if (appSecEnabled) { + // Compare by class name instead of `instanceof`: `instanceof` would resolve + // BlockingException via the instrumented class's own classloader, which can throw + // NoClassDefFoundError right here - uncaught - on a classloader that can't see the + // appsec module. A name comparison never triggers that resolution. mv.visitInsn(Opcodes.DUP); - mv.visitTypeInsn(Opcodes.INSTANCEOF, "datadog/appsec/api/blocking/BlockingException"); + mv.visitMethodInsn( + Opcodes.INVOKEVIRTUAL, + "java/lang/Object", + "getClass", + "()Ljava/lang/Class;", + false); + mv.visitMethodInsn( + Opcodes.INVOKEVIRTUAL, "java/lang/Class", "getName", "()Ljava/lang/String;", false); + mv.visitLdcInsn("datadog.appsec.api.blocking.BlockingException"); + mv.visitMethodInsn( + Opcodes.INVOKEVIRTUAL, + "java/lang/String", + "equals", + "(Ljava/lang/Object;)Z", + false); mv.visitJumpInsn(Opcodes.IFEQ, notBlocking); mv.visitInsn(Opcodes.ATHROW); mv.visitLabel(notBlocking); diff --git a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/BaseExceptionHandlerTest.groovy b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/BaseExceptionHandlerTest.groovy index 9fe24a4e167..e161d368cb7 100644 --- a/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/BaseExceptionHandlerTest.groovy +++ b/dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/BaseExceptionHandlerTest.groovy @@ -68,6 +68,14 @@ abstract class BaseExceptionHandlerTest extends DDSpecification { .advice( isMethod().and(named("blockingException")), BlockingExceptionAdvice.getName())) + .type(named(BaseExceptionHandlerTest.getName() + '$SomeOtherClass')) + .transform( + new AgentBuilder.Transformer.ForAdvice() + .with(new AgentBuilder.LocationStrategy.Simple(ClassFileLocator.ForClassLoader.of(BadAdvice.getClassLoader()))) + .withExceptionHandler(ExceptionHandlers.exceptionHandlerFor(BadAdvice.getName())) + .advice( + isMethod().and(named("isInstrumented")), + BadAdvice.getName())) ByteBuddyAgent.install() transformer = builder.installOn(ByteBuddyAgent.getInstrumentation()) @@ -150,9 +158,8 @@ abstract class BaseExceptionHandlerTest extends DDSpecification { SomeClass.getProtectionDomain().getCodeSource().getLocation(), GroovyObject.getProtectionDomain().getCodeSource().getLocation(), ] - // Fully isolated loader: unlike BlockingTestClassLoader, this does NOT special-case - // BlockingExceptionHandler/BlockingException, so neither is resolvable from here. - URLClassLoader loader = new URLClassLoader(classpath, null, null) + URLClassLoader loader = new AppSecInvisibleClassLoader( + classpath, BaseExceptionHandlerTest.getClassLoader(), SomeOtherClass.getName()) when: loader.loadClass(BlockingException.getName()) @@ -160,12 +167,16 @@ abstract class BaseExceptionHandlerTest extends DDSpecification { thrown ClassNotFoundException when: - Class someClazz = loader.loadClass(SomeClass.getName()) + Class someClazz = loader.loadClass(SomeOtherClass.getName()) + then: + someClazz.getClassLoader() == loader + + when: someClazz.getMethod("isInstrumented").invoke(null) then: noExceptionThrown() testAppender.list.size() == initLogEvents + 1 - exitStatus.get() == 0 + exitStatus.get() == expectedFailureExitStatus() } def "exception handler sets the correct stack size"() { @@ -218,6 +229,16 @@ abstract class BaseExceptionHandlerTest extends DDSpecification { } } + // Deliberately not instrumented with BlockingExceptionAdvice, unlike SomeClass: that advice's + // own bytecode constructs a real BlockingException, which would make the JVM verifier resolve + // BlockingException while linking the whole class - defeating the point of testing a + // classloader that can't see it. + static class SomeOtherClass { + static boolean isInstrumented() { + return false + } + } + private static class NoExitSecurityManager extends SecurityManager { private final AtomicInteger status diff --git a/dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/test/AppSecInvisibleClassLoader.java b/dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/test/AppSecInvisibleClassLoader.java new file mode 100644 index 00000000000..72f86d9f29d --- /dev/null +++ b/dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/test/AppSecInvisibleClassLoader.java @@ -0,0 +1,37 @@ +package datadog.trace.agent.test; + +import datadog.appsec.api.blocking.BlockingException; +import java.net.URL; +import java.net.URLClassLoader; + +/** + * A {@link URLClassLoader} that delegates to the given parent for everything - including {@code + * datadog.trace.bootstrap.*} and slf4j, which in production are visible from any classloader - + * except {@link BlockingException} (which in production is only visible if the appsec module is + * present) and the given target class name, which this loader defines locally so it gets + * instrumented as if loaded by an isolated (e.g. plugin/OSGi-style) classloader. + */ +final class AppSecInvisibleClassLoader extends URLClassLoader { + private final String isolatedClassName; + + AppSecInvisibleClassLoader(URL[] classpath, ClassLoader parent, String isolatedClassName) { + super(classpath, parent); + this.isolatedClassName = isolatedClassName; + } + + @Override + protected synchronized Class loadClass(String name, boolean resolve) + throws ClassNotFoundException { + Class found = findLoadedClass(name); + if (found == null) { + if (name.equals(BlockingException.class.getName())) { + throw new ClassNotFoundException(name); + } + found = name.equals(isolatedClassName) ? findClass(name) : super.loadClass(name, resolve); + } + if (resolve) { + resolveClass(found); + } + return found; + } +}