Fix StackOverflowError in caused by missing re-entrancy guard

dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-common/src/main/java/datadog/trace/instrumentation/servlet/dispatcher/RequestDispatcherInstrumentation.java

@@ -27,6 +27,7 @@
 import datadog.context.ContextScope;
 import datadog.trace.agent.tooling.Instrumenter;
 import datadog.trace.agent.tooling.InstrumenterModule;
+import datadog.trace.bootstrap.CallDepthThreadLocalMap;
 import datadog.trace.bootstrap.InstrumentationContext;
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
 import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext;
@@ -100,6 +101,11 @@ public static ContextScope start(
         // Don't want to generate a new top-level span
         return null;
       }
+
+      final int depth = CallDepthThreadLocalMap.incrementCallDepth(RequestDispatcher.class);
+      if (depth > 0) {
+        return null;
+      }
       final AgentSpanContext parent;
       if (servletSpan == null || (parentSpan != null && servletSpan.isSameTrace(parentSpan))) {
         // Use the parentSpan if the servletSpan is null or part of the same trace.
@@ -147,6 +153,7 @@ public static void stop(
       if (scope == null) {
         return;
       }
+      CallDepthThreadLocalMap.reset(RequestDispatcher.class);
 
       if (requestContext != null) {
         request.setAttribute(DD_CONTEXT_ATTRIBUTE, requestContext);

dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-common/src/test/java/RequestDispatcherRecursionTest.java

@@ -0,0 +1,195 @@
+import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import datadog.trace.agent.test.AbstractInstrumentationTest;
+import java.io.IOException;
+import java.lang.reflect.Proxy;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+import javax.servlet.RequestDispatcher;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Regression test for the SAP Hybris/Spartacus StackOverflow reported in v1.63.0.
+ *
+ * <p>Root cause: {@code RequestDispatcherAdvice.start()} calls {@code injectContext()} which calls
+ * {@code request.setAttribute()} for each propagation header. In some request wrappers (Hybris
+ * internals, SAP Commerce), {@code setAttribute()} internally triggers another {@code
+ * RequestDispatcher.forward()} call. Without a re-entrancy guard the advice recurses until {@code
+ * StackOverflowError}, visible in production as many "Failed to handle exception in instrumentation
+ * for ApplicationDispatcher" log entries.
+ *
+ * <p>The fix is a {@code CallDepthThreadLocalMap} guard in {@code RequestDispatcherAdvice.start()}
+ * analogous to the one already present in {@code AsyncContextInstrumentation}.
+ */
+class RequestDispatcherRecursionTest extends AbstractInstrumentationTest {
+
+  /** Minimal {@code RequestDispatcher} stub — instrumented by ByteBuddy via interface match. */
+  static class TestDispatcher implements RequestDispatcher {
+    @Override
+    public void forward(ServletRequest req, ServletResponse resp)
+        throws ServletException, IOException {}
+
+    @Override
+    public void include(ServletRequest req, ServletResponse resp)
+        throws ServletException, IOException {}
+  }
+
+  /**
+   * {@code RequestDispatcher} with a non-trivial body simulating {@code ApplicationDispatcher}
+   * (Tomcat / SAP Commerce), which traverses a filter and valve chain adding significant call
+   * depth.
+   */
+  static class RealisticDispatcher implements RequestDispatcher {
+    @Override
+    public void forward(ServletRequest req, ServletResponse resp)
+        throws ServletException, IOException {
+      simulateFilterChain(200);
+    }
+
+    @Override
+    public void include(ServletRequest req, ServletResponse resp)
+        throws ServletException, IOException {}
+
+    private static void simulateFilterChain(int depth) {
+      if (depth > 0) simulateFilterChain(depth - 1);
+    }
+  }
+
+  /** Creates a proxy stub for {@code iface} where all methods return null / 0 / false. */
+  @SuppressWarnings("unchecked")
+  static <T> T nullStub(Class<T> iface) {
+    return (T)
+        Proxy.newProxyInstance(
+            iface.getClassLoader(),
+            new Class<?>[] {iface},
+            (proxy, method, args) -> {
+              Class<?> ret = method.getReturnType();
+              if (ret == boolean.class) return false;
+              if (ret == int.class || ret == long.class) return 0;
+              return null;
+            });
+  }
+
+  @Test
+  void forward_doesNotRecurse_whenSetAttributeTriggersRedispatch() throws Exception {
+    TestDispatcher dispatcher = new TestDispatcher();
+    HttpServletResponse response = nullStub(HttpServletResponse.class);
+
+    AtomicInteger currentDepth = new AtomicInteger(0);
+    AtomicInteger maxDepth = new AtomicInteger(0);
+
+    // Hybris-style wrapper: setAttribute() re-triggers a dispatch. Capped at depth 3
+    // to bound total calls (≈ N^3, N ≈ 5 headers) and avoid OOM without the fix.
+    final int MAX_SETATTRIBUTE_DEPTH = 3;
+    HttpServletRequest recursiveRequest =
+        new HttpServletRequestWrapper(nullStub(HttpServletRequest.class)) {
+          @Override
+          public void setAttribute(String name, Object value) {
+            int depth = currentDepth.incrementAndGet();
+            maxDepth.updateAndGet(d -> Math.max(d, depth));
+            try {
+              super.setAttribute(name, value);
+              if (depth < MAX_SETATTRIBUTE_DEPTH) {
+                dispatcher.forward(this, response);
+              }
+            } catch (Exception | StackOverflowError ignored) {
+            } finally {
+              currentDepth.decrementAndGet();
+            }
+          }
+        };
+
+    // runUnderTrace provides an active span; without one the advice exits before injectContext().
+    runUnderTrace(
+        "test",
+        () -> {
+          dispatcher.forward(recursiveRequest, response);
+          return null;
+        });
+
+    assertTrue(
+        maxDepth.get() >= 1,
+        "advice did not call setAttribute() — check that TestDispatcher is instrumented"
+            + " by ByteBuddy and that runUnderTrace creates an active span");
+
+    assertEquals(
+        1,
+        maxDepth.get(),
+        "setAttribute() was called recursively to depth "
+            + maxDepth.get()
+            + ". RequestDispatcherAdvice.start() needs a CallDepthThreadLocalMap guard "
+            + "(see AsyncContextInstrumentation for the pattern).");
+  }
+
+  /**
+   * Regression test for the production SOE scenario with a realistic dispatcher body.
+   *
+   * <p>Without the fix, the recursive {@code setAttribute()} pattern in Hybris would fill the call
+   * stack, and the extra frames from {@code simulateFilterChain()} would tip it into a {@code
+   * StackOverflowError} from the method <em>body</em> — captured by {@code @Advice.Thrown} → {@code
+   * DECORATE.onError()} → {@code span.setTag("error.stack")}.
+   *
+   * <p>With the {@code CallDepthThreadLocalMap} fix, re-entrant {@code forward()} calls return
+   * immediately from the enter advice (depth &gt; 0), so the stack never saturates and no SOE is
+   * thrown. The test verifies exactly this: {@code capturedSoe} must be {@code null}.
+   */
+  @Test
+  void forward_noSoe_withRealisticDispatcherBodyAndSetAttributeRecursion() throws Exception {
+    AtomicReference<StackOverflowError> capturedSoe = new AtomicReference<>();
+    AtomicBoolean soeSeen = new AtomicBoolean(false); // stops re-dispatching once SOE is caught
+    AtomicBoolean setAttributeInvoked = new AtomicBoolean(false); // guards against false-positive
+
+    RealisticDispatcher dispatcher = new RealisticDispatcher();
+    HttpServletResponse response = nullStub(HttpServletResponse.class);
+
+    // Hybris-style wrapper: setAttribute() triggers a new forward(); no depth cap.
+    HttpServletRequest recursiveRequest =
+        new HttpServletRequestWrapper(nullStub(HttpServletRequest.class)) {
+          @Override
+          public void setAttribute(String name, Object value) {
+            super.setAttribute(name, value);
+            setAttributeInvoked.set(true);
+            if (soeSeen.get()) return; // already captured — stop recursing
+            try {
+              dispatcher.forward(this, response);
+            } catch (StackOverflowError soe) {
+              capturedSoe.compareAndSet(null, soe);
+              soeSeen.set(true);
+              throw soe;
+            } catch (Exception ignored) {
+            }
+          }
+        };
+
+    try {
+      runUnderTrace(
+          "test",
+          () -> {
+            dispatcher.forward(recursiveRequest, response);
+            return null;
+          });
+    } catch (StackOverflowError ignored) {
+      // SOE may reach this level if it is not fully absorbed at inner levels.
+    }
+
+    assertTrue(
+        setAttributeInvoked.get(),
+        "advice did not call setAttribute() — check that RealisticDispatcher is instrumented"
+            + " by ByteBuddy and that runUnderTrace creates an active span");
+
+    assertNull(
+        capturedSoe.get(),
+        "StackOverflowError was thrown — fix regression: CallDepthThreadLocalMap guard "
+            + "was removed from RequestDispatcherAdvice.start()");
+  }
+}