From e46eaa259035b5d05dc0430a16bfe441207415d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20=C3=81lvarez=20=C3=81lvarez?= Date: Sun, 29 Sep 2024 17:13:19 +0200 Subject: [PATCH] Fix stack overflow in spock making junit ignore test failures (#7674) Fix stack overflow on TooManyInvocationsError --- .../agent/tooling/InstrumenterModule.java | 11 +++- .../lang/StringBuilderCallSiteTest.groovy | 2 +- .../owasp/esapi/EncoderCallSiteTest.groovy | 2 +- ...portValueInstrumentationForkedTest.groovy} | 8 +-- .../datadog/trace/agent/test/SpockRunner.java | 55 +++++++++++++++++++ ...TooManyInvocationsErrorListenerTest.groovy | 38 +++++++++++++ .../datadog/trace/api/InstrumenterConfig.java | 8 +++ 7 files changed, 115 insertions(+), 9 deletions(-) rename dd-java-agent/instrumentation/tomcat-appsec-7/src/test/groovy/datadog/trace/instrumentation/tomcat7/{ErrorReportValueInstrumentationTest.groovy => ErrorReportValueInstrumentationForkedTest.groovy} (80%) create mode 100644 dd-java-agent/testing/src/test/groovy/TooManyInvocationsErrorListenerTest.groovy diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/InstrumenterModule.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/InstrumenterModule.java index f1afd39e2f7..253eacc8331 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/InstrumenterModule.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/InstrumenterModule.java @@ -249,9 +249,14 @@ public List typeInstrumentations() { @Override public boolean isApplicable(Set enabledSystems) { - return enabledSystems.contains(TargetSystem.IAST) - || (isOptOutEnabled() - && InstrumenterConfig.get().getAppSecActivation() == ProductActivation.FULLY_ENABLED); + if (enabledSystems.contains(TargetSystem.IAST)) { + return true; + } + final InstrumenterConfig cfg = InstrumenterConfig.get(); + if (!isOptOutEnabled() || cfg.isIastFullyDisabled()) { + return false; + } + return cfg.getAppSecActivation() == ProductActivation.FULLY_ENABLED; } /** diff --git a/dd-java-agent/instrumentation/java-lang/src/test/groovy/datadog/trace/instrumentation/java/lang/StringBuilderCallSiteTest.groovy b/dd-java-agent/instrumentation/java-lang/src/test/groovy/datadog/trace/instrumentation/java/lang/StringBuilderCallSiteTest.groovy index f8a25369ebb..109262fab2f 100644 --- a/dd-java-agent/instrumentation/java-lang/src/test/groovy/datadog/trace/instrumentation/java/lang/StringBuilderCallSiteTest.groovy +++ b/dd-java-agent/instrumentation/java-lang/src/test/groovy/datadog/trace/instrumentation/java/lang/StringBuilderCallSiteTest.groovy @@ -57,7 +57,7 @@ class StringBuilderCallSiteTest extends AgentTestRunner { if (param.class == String) { 1 * iastModule.onStringBuilderAppend(target, (String) param) } else { - 1 * iastModule.onStringBuilderAppend(target, param.toString()) + 1 * iastModule.onStringBuilderAppend(target, { it -> it.toString() == param.toString() } ) } _ * TEST_PROFILING_CONTEXT_INTEGRATION._ 0 * _ diff --git a/dd-java-agent/instrumentation/owasp-esapi-2/src/test/groovy/datadog/trace/instrumentation/owasp/esapi/EncoderCallSiteTest.groovy b/dd-java-agent/instrumentation/owasp-esapi-2/src/test/groovy/datadog/trace/instrumentation/owasp/esapi/EncoderCallSiteTest.groovy index 800b7653a77..c90ad8e96bc 100644 --- a/dd-java-agent/instrumentation/owasp-esapi-2/src/test/groovy/datadog/trace/instrumentation/owasp/esapi/EncoderCallSiteTest.groovy +++ b/dd-java-agent/instrumentation/owasp-esapi-2/src/test/groovy/datadog/trace/instrumentation/owasp/esapi/EncoderCallSiteTest.groovy @@ -25,7 +25,7 @@ class EncoderCallSiteTest extends AgentTestRunner { testSuite.&"$method".call(args) then: - 1 * module.taintObjectIfTainted(_, _, false, mark) + 1 * module.taintStringIfTainted(_, _, false, mark) 0 * module._ where: diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/test/groovy/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentationTest.groovy b/dd-java-agent/instrumentation/tomcat-appsec-7/src/test/groovy/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentationForkedTest.groovy similarity index 80% rename from dd-java-agent/instrumentation/tomcat-appsec-7/src/test/groovy/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentationTest.groovy rename to dd-java-agent/instrumentation/tomcat-appsec-7/src/test/groovy/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentationForkedTest.groovy index 3cc5ec706c8..9d176479d73 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/test/groovy/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/test/groovy/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentationForkedTest.groovy @@ -7,7 +7,7 @@ import org.apache.catalina.connector.Request import org.apache.catalina.connector.Response import org.apache.catalina.valves.ErrorReportValve -class ErrorReportValueInstrumentationTest extends AgentTestRunner { +class ErrorReportValueInstrumentationForkedTest extends AgentTestRunner { void 'test vulnerability detection'() { given: @@ -37,7 +37,7 @@ class ErrorReportValueInstrumentationTest extends AgentTestRunner { } } -class AppSecErrorReportValueInstrumentationTest extends ErrorReportValueInstrumentationTest { +class AppSecErrorReportValueInstrumentationForkedTest extends ErrorReportValueInstrumentationForkedTest { @Override protected void configurePreAgent() { @@ -51,7 +51,7 @@ class AppSecErrorReportValueInstrumentationTest extends ErrorReportValueInstrume } } -class IastErrorReportValueInstrumentationTest extends ErrorReportValueInstrumentationTest { +class IastErrorReportValueInstrumentationForkedTest extends ErrorReportValueInstrumentationForkedTest { @Override protected void configurePreAgent() { @@ -65,7 +65,7 @@ class IastErrorReportValueInstrumentationTest extends ErrorReportValueInstrument } } -class IastDisabledErrorReportValueInstrumentationTest extends ErrorReportValueInstrumentationTest { +class IastDisabledErrorReportValueInstrumentationForkedTest extends ErrorReportValueInstrumentationForkedTest { @Override protected void configurePreAgent() { diff --git a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/SpockRunner.java b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/SpockRunner.java index 2e1703aa4d5..bb2f9ab0b3c 100644 --- a/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/SpockRunner.java +++ b/dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/SpockRunner.java @@ -9,13 +9,18 @@ import java.lang.reflect.Method; import java.util.Arrays; import java.util.HashSet; +import java.util.List; import java.util.Set; import java.util.TreeSet; import java.util.jar.JarFile; import net.bytebuddy.agent.ByteBuddyAgent; import net.bytebuddy.dynamic.ClassFileLocator; import org.junit.platform.runner.JUnitPlatform; +import org.junit.runner.notification.Failure; +import org.junit.runner.notification.RunListener; import org.junit.runner.notification.RunNotifier; +import org.spockframework.mock.IMockInvocation; +import org.spockframework.mock.TooManyInvocationsError; /** * Runs a spock test in an agent-friendly way. @@ -129,10 +134,13 @@ private static Class shadowTestClass(final Class clazz) { @Override public void run(final RunNotifier notifier) { final ClassLoader contextLoader = Thread.currentThread().getContextClassLoader(); + final RunListener listener = new TooManyInvocationsErrorListener(); try { Thread.currentThread().setContextClassLoader(customLoader); + notifier.addFirstListener(listener); super.run(notifier); } finally { + notifier.removeListener(listener); Thread.currentThread().setContextClassLoader(contextLoader); } } @@ -222,4 +230,51 @@ protected Class loadClass(final String name, final boolean resolve) } } } + + /** + * This class tries to fix {@link TooManyInvocationsError} exceptions when the assertion error is + * caught by a mock triggering a stack overflow while composing the failure message. + */ + @SuppressWarnings("ResultOfMethodCallIgnored") + @RunListener.ThreadSafe + private static class TooManyInvocationsErrorListener extends RunListener { + + @Override + public void testFailure(final Failure failure) throws Exception { + if (failure.getException() instanceof TooManyInvocationsError) { + final TooManyInvocationsError assertion = (TooManyInvocationsError) failure.getException(); + try { + // try to trigger an error (e.g. stack overflow) + assertion.getMessage(); + } catch (final Throwable e) { + fixTooManyInvocationsError(assertion); + } + } + } + + private void fixTooManyInvocationsError(final TooManyInvocationsError error) { + final List accepted = error.getAcceptedInvocations(); + for (final IMockInvocation invocation : accepted) { + try { + invocation.toString(); + } catch (final Throwable t) { + final List arguments = invocation.getArguments(); + for (int i = 0; i < arguments.size(); i++) { + final Object arg = arguments.get(i); + if (arg instanceof AssertionError) { + final AssertionError updatedAssertion = + new AssertionError( + "'" + + arg.getClass().getName() + + "' hidden due to '" + + t.getClass().getName() + + "'", + t); + invocation.getArguments().set(i, updatedAssertion); + } + } + } + } + } + } } diff --git a/dd-java-agent/testing/src/test/groovy/TooManyInvocationsErrorListenerTest.groovy b/dd-java-agent/testing/src/test/groovy/TooManyInvocationsErrorListenerTest.groovy new file mode 100644 index 00000000000..265a63e8fc0 --- /dev/null +++ b/dd-java-agent/testing/src/test/groovy/TooManyInvocationsErrorListenerTest.groovy @@ -0,0 +1,38 @@ +import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.agent.test.SpockRunner +import org.junit.runner.Description +import org.junit.runner.notification.Failure +import org.spockframework.mock.IMockInteraction +import org.spockframework.mock.IMockMethod +import org.spockframework.mock.IMockObject +import org.spockframework.mock.IResponseGenerator +import org.spockframework.mock.TooManyInvocationsError +import org.spockframework.mock.runtime.MockInvocation + +class TooManyInvocationsErrorListenerTest extends AgentTestRunner { + + @SuppressWarnings('GroovyAccessibility') + void 'test that listener modifies failure'() { + setup: + final error = new TooManyInvocationsError(Stub(IMockInteraction), []) + error.acceptedInvocations.add(new MockInvocation(Stub(IMockObject), + Stub(IMockMethod), + [error], + Stub(IResponseGenerator))) + final failure = new Failure(new Description(TooManyInvocationsErrorListenerTest, 'test'), error) + + when: + failure.getMessage() + + then: + thrown(StackOverflowError) + + when: + final listener = new SpockRunner.TooManyInvocationsErrorListener() + listener.testFailure(failure) + failure.getMessage() + + then: + noExceptionThrown() + } +} diff --git a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java index f0aa03e83f8..78152292a48 100644 --- a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java +++ b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java @@ -108,6 +108,7 @@ public class InstrumenterConfig { private final boolean ciVisibilityEnabled; private final ProductActivation appSecActivation; private final ProductActivation iastActivation; + private final boolean iastFullyDisabled; private final boolean usmEnabled; private final boolean telemetryEnabled; @@ -194,6 +195,8 @@ private InstrumenterConfig() { iastActivation = ProductActivation.fromString( configProvider.getStringNotEmpty(IAST_ENABLED, DEFAULT_IAST_ENABLED)); + final Boolean iastEnabled = configProvider.getBoolean(IAST_ENABLED); + iastFullyDisabled = iastEnabled != null && !iastEnabled; usmEnabled = configProvider.getBoolean(USM_ENABLED, DEFAULT_USM_ENABLED); telemetryEnabled = configProvider.getBoolean(TELEMETRY_ENABLED, DEFAULT_TELEMETRY_ENABLED); } else { @@ -201,6 +204,7 @@ private InstrumenterConfig() { ciVisibilityEnabled = false; appSecActivation = ProductActivation.FULLY_DISABLED; iastActivation = ProductActivation.FULLY_DISABLED; + iastFullyDisabled = true; telemetryEnabled = false; usmEnabled = false; } @@ -321,6 +325,10 @@ public ProductActivation getIastActivation() { return iastActivation; } + public boolean isIastFullyDisabled() { + return iastFullyDisabled; + } + public boolean isUsmEnabled() { return usmEnabled; }