diff --git a/dd-java-agent/agent-installer/src/main/java/datadog/trace/agent/tooling/CombiningTransformerBuilder.java b/dd-java-agent/agent-installer/src/main/java/datadog/trace/agent/tooling/CombiningTransformerBuilder.java index 1990b692092..6b6e4167cb9 100644 --- a/dd-java-agent/agent-installer/src/main/java/datadog/trace/agent/tooling/CombiningTransformerBuilder.java +++ b/dd-java-agent/agent-installer/src/main/java/datadog/trace/agent/tooling/CombiningTransformerBuilder.java @@ -274,7 +274,7 @@ private void addAdviceIfEnabled( } AgentBuilder.Transformer.ForAdvice forAdvice = new AgentBuilder.Transformer.ForAdvice(customMapping) - .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler()) + .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler(adviceClass)) .include(Utils.getBootstrapProxy()); ClassLoader adviceLoader = Utils.getExtendedClassLoader(); if (adviceShader != null) { 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 2fd96e2edf1..77ee0f8afab 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 @@ -7,6 +7,7 @@ import net.bytebuddy.asm.Advice.ExceptionHandler; import net.bytebuddy.implementation.Implementation; import net.bytebuddy.implementation.bytecode.StackManipulation; +import net.bytebuddy.implementation.bytecode.constant.TextConstant; import net.bytebuddy.jar.asm.Label; import net.bytebuddy.jar.asm.MethodVisitor; import net.bytebuddy.jar.asm.Opcodes; @@ -20,131 +21,163 @@ public class ExceptionHandlers { // Bootstrap ExceptionHandler.class will always be resolvable, so we'll use it in the log name private static final String HANDLER_NAME = ExceptionLogger.class.getName().replace('.', '/'); - private static final ExceptionHandler EXCEPTION_STACK_HANDLER = - new ExceptionHandler.Simple( - new StackManipulation() { - // Pops one Throwable off the stack. Maxes the stack to at least 3. - private final Size size = new StackManipulation.Size(-1, 3); - private final boolean appSecEnabled = - InstrumenterConfig.get().getAppSecActivation() != ProductActivation.FULLY_DISABLED; - private final boolean detailedErrors = - InstrumenterConfig.get().isDetailedInstrumentationErrors(); - - @Override - public boolean isValid() { - return true; - } - - @Override - public Size apply(final MethodVisitor mv, final Implementation.Context context) { - final String name = context.getInstrumentedType().getName(); - final boolean exitOnFailure = InstrumenterConfig.get().isInternalExitOnFailure(); - final String logMethod = exitOnFailure ? "error" : "debug"; - - // Writes the following bytecode if exitOnFailure is false: - // - // BlockingExceptionHandler.rethrowIfBlockingException(t); - // try { - // InstrumentationErrors.recordError(); - // org.slf4j.LoggerFactory.getLogger((Class)ExceptionLogger.class) - // .debug("Failed to handle exception in instrumentation for ...", t); - // } catch (Throwable t2) { - // } - // - // And the following bytecode if exitOnFailure is true: - // - // BlockingExceptionHandler.rethrowIfBlockingException(t); - // try { - // InstrumentationErrors.recordError(); - // org.slf4j.LoggerFactory.getLogger((Class)ExceptionLogger.class) - // .error("Failed to handle exception in instrumentation for ...", t); - // System.exit(1); - // } catch (Throwable t2) { - // } - // - final Label logStart = new Label(); - final Label logEnd = new Label(); - final Label eatException = 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); - - if (appSecEnabled) { - // rethrow blocking exceptions - // stack: (top) throwable - mv.visitMethodInsn( - Opcodes.INVOKESTATIC, - "datadog/trace/bootstrap/blocking/BlockingExceptionHandler", - "rethrowIfBlockingException", - "(Ljava/lang/Throwable;)Ljava/lang/Throwable;", - false); - } - - mv.visitTryCatchBlock(logStart, logEnd, eatException, "java/lang/Throwable"); - mv.visitLabel(logStart); - // record instrumentation error - if (detailedErrors) { - mv.visitMethodInsn( - Opcodes.INVOKESTATIC, - "datadog/trace/bootstrap/InstrumentationErrors", - "recordError", - "(Ljava/lang/Throwable;)Ljava/lang/Throwable;", - false); - } else { - mv.visitMethodInsn( - Opcodes.INVOKESTATIC, - "datadog/trace/bootstrap/InstrumentationErrors", - "recordError", - "()V", - false); - } - // stack: (top) throwable - mv.visitLdcInsn(Type.getType("L" + HANDLER_NAME + ";")); - mv.visitMethodInsn( - Opcodes.INVOKESTATIC, - LOG_FACTORY_NAME, - "getLogger", - "(Ljava/lang/Class;)L" + LOGGER_NAME + ";", - false); - mv.visitInsn(Opcodes.SWAP); // stack: (top) throwable,logger - mv.visitLdcInsn("Failed to handle exception in instrumentation for " + name); - mv.visitInsn(Opcodes.SWAP); // stack: (top) throwable,string,logger - mv.visitMethodInsn( - Opcodes.INVOKEINTERFACE, - LOGGER_NAME, - logMethod, - "(Ljava/lang/String;Ljava/lang/Throwable;)V", - true); - - if (exitOnFailure) { - mv.visitInsn(Opcodes.ICONST_1); - mv.visitMethodInsn(Opcodes.INVOKESTATIC, "java/lang/System", "exit", "(I)V", false); - } - mv.visitLabel(logEnd); - mv.visitJumpInsn(Opcodes.GOTO, handlerExit); - - // if the runtime can't reach our ExceptionHandler or logger, - // silently eat the exception - mv.visitLabel(eatException); - 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); - - mv.visitLabel(handlerExit); - if (frames) { - mv.visitFrame(Opcodes.F_SAME, 0, null, 0, null); - } - - return size; - } - }); - - public static ExceptionHandler defaultExceptionHandler() { - return EXCEPTION_STACK_HANDLER; + // Shared bytecode that turns the [throwable, adviceName] stack left by the TextConstant + // prefix into a logged + optionally swallowed exception. + private static final StackManipulation EXCEPTION_STACK_MANIPULATION = + new StackManipulation() { + // Pops the throwable and the advice-name String off the stack. + // Peak growth above the pair on entry is +2 (during message-building LDCs). + private final Size size = new StackManipulation.Size(-2, 2); + private final boolean appSecEnabled = + InstrumenterConfig.get().getAppSecActivation() != ProductActivation.FULLY_DISABLED; + private final boolean detailedErrors = + InstrumenterConfig.get().isDetailedInstrumentationErrors(); + + @Override + public boolean isValid() { + return true; + } + + @Override + public Size apply(final MethodVisitor mv, final Implementation.Context context) { + final String instrumentedTypeName = context.getInstrumentedType().getName(); + final boolean exitOnFailure = InstrumenterConfig.get().isInternalExitOnFailure(); + final String logMethod = exitOnFailure ? "error" : "debug"; + + // On entry the stack is: (bottom) throwable, adviceName (top) + // — adviceName was pushed by the preceding TextConstant. + // + // Emits the following Java-equivalent code when exitOnFailure is false: + // + // BlockingExceptionHandler.rethrowIfBlockingException(t); + // try { + // InstrumentationErrors.recordError(); + // org.slf4j.LoggerFactory.getLogger((Class) ExceptionLogger.class) + // .debug("Failed to handle exception in instrumentation for (" + adviceName + + // ")", t); + // } catch (Throwable t2) { + // } + // + // and the same with .error(...) followed by System.exit(1) when exitOnFailure is true. + final Label logStart = new Label(); + final Label logEnd = new Label(); + final Label eatException = 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); + + if (appSecEnabled) { + // Need throwable on top for rethrowIfBlockingException. + // stack: (top) adviceName, throwable -> top throwable + mv.visitInsn(Opcodes.SWAP); + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + "datadog/trace/bootstrap/blocking/BlockingExceptionHandler", + "rethrowIfBlockingException", + "(Ljava/lang/Throwable;)Ljava/lang/Throwable;", + false); + // restore: (top) throwable, adviceName -> top adviceName + 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. + mv.visitInsn(Opcodes.SWAP); + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + "datadog/trace/bootstrap/InstrumentationErrors", + "recordError", + "(Ljava/lang/Throwable;)Ljava/lang/Throwable;", + false); + mv.visitInsn(Opcodes.SWAP); + } else { + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + "datadog/trace/bootstrap/InstrumentationErrors", + "recordError", + "()V", + false); + } + + // Build the log message: + // "Failed to handle exception in instrumentation for (" + adviceName + ")" + // is a generation-time constant baked into the prefix LDC. + // stack: (top) adviceName, throwable + mv.visitLdcInsn( + "Failed to handle exception in instrumentation for " + instrumentedTypeName + " ("); + // stack: prefix (top), adviceName, throwable + mv.visitInsn(Opcodes.SWAP); + // stack: adviceName (top), prefix, throwable + mv.visitMethodInsn( + Opcodes.INVOKEVIRTUAL, + "java/lang/String", + "concat", + "(Ljava/lang/String;)Ljava/lang/String;", + false); + // stack: "Failed to ... (" (top), throwable + mv.visitLdcInsn(")"); + mv.visitMethodInsn( + Opcodes.INVOKEVIRTUAL, + "java/lang/String", + "concat", + "(Ljava/lang/String;)Ljava/lang/String;", + false); + // stack: message (top), throwable + mv.visitInsn(Opcodes.SWAP); + // stack: throwable(top), message + + mv.visitLdcInsn(Type.getType("L" + HANDLER_NAME + ";")); + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + LOG_FACTORY_NAME, + "getLogger", + "(Ljava/lang/Class;)L" + LOGGER_NAME + ";", + false); + // stack: logger (top), throwable, message + mv.visitInsn(Opcodes.DUP_X2); + // stack: logger (top), throwable, message, logger + mv.visitInsn(Opcodes.POP); + // stack: throwable (top), message, logger + mv.visitMethodInsn( + Opcodes.INVOKEINTERFACE, + LOGGER_NAME, + logMethod, + "(Ljava/lang/String;Ljava/lang/Throwable;)V", + true); + + if (exitOnFailure) { + mv.visitInsn(Opcodes.ICONST_1); + mv.visitMethodInsn(Opcodes.INVOKESTATIC, "java/lang/System", "exit", "(I)V", false); + } + mv.visitLabel(logEnd); + mv.visitJumpInsn(Opcodes.GOTO, handlerExit); + + // if the runtime can't reach our ExceptionHandler or logger, + // silently eat the exception + mv.visitLabel(eatException); + 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); + + mv.visitLabel(handlerExit); + if (frames) { + mv.visitFrame(Opcodes.F_SAME, 0, null, 0, null); + } + + return size; + } + }; + + public static ExceptionHandler defaultExceptionHandler(final String adviceClassName) { + return new ExceptionHandler.Simple( + new StackManipulation.Compound( + new TextConstant(adviceClassName), EXCEPTION_STACK_MANIPULATION)); } } 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 0c040e3b8f9..84b34f68530 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 @@ -51,21 +51,21 @@ abstract class BaseExceptionHandlerTest extends DDSpecification { .transform( new AgentBuilder.Transformer.ForAdvice() .with(new AgentBuilder.LocationStrategy.Simple(ClassFileLocator.ForClassLoader.of(BadAdvice.getClassLoader()))) - .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler()) + .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler(BadAdvice.getName())) .advice( isMethod().and(named("isInstrumented")), BadAdvice.getName())) .transform( new AgentBuilder.Transformer.ForAdvice() .with(new AgentBuilder.LocationStrategy.Simple(ClassFileLocator.ForClassLoader.of(BadAdvice.getClassLoader()))) - .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler()) + .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler(BadAdvice.NoOpAdvice.getName())) .advice( isMethod().and(namedOneOf("smallStack", "largeStack")), BadAdvice.NoOpAdvice.getName())) .transform( new AgentBuilder.Transformer.ForAdvice() .with(new AgentBuilder.LocationStrategy.Simple(ClassFileLocator.ForClassLoader.of(BadAdvice.getClassLoader()))) - .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler()) + .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler(BlockingExceptionAdvice.getName())) .advice( isMethod().and(named("blockingException")), BlockingExceptionAdvice.getName())) @@ -117,7 +117,7 @@ abstract class BaseExceptionHandlerTest extends DDSpecification { // Make sure the log event came from our error handler. // If the log message changes in the future, it's fine to just // update the test's hardcoded message - testAppender.list.get(testAppender.list.size() - 1).getMessage().startsWith("Failed to handle exception in instrumentation for") + testAppender.list.get(testAppender.list.size() - 1).getMessage() == "Failed to handle exception in instrumentation for ${SomeClass.getName()} (${BadAdvice.getName()})" exitStatus.get() == expectedFailureExitStatus() }