Fix method parameters JVM bug

Prevent classfiles with Method Parameters attribute
(javac -parameters) to be (re)transformed when on JDK < 19.
Spring 6+ or SpringBoot3+ rely exclusively on method parameters
to get param name and if the attribute is not present throw an
exception and returns 500 on endpoints.
see OpenJDK bug JDK-8240908.
we are scanning method with at least on parameter to detect if the
class was compiled with method parameters attribute and if the JDK
is < 19 we prevent instrumentation to happen.
Even at load time we prevent it because we need to call retransform
to remove instrumentation. Therefore the attribute can be strip at
that time.

Author: jpbempel

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationUpdater.java

@@ -13,6 +13,7 @@
 import com.datadog.debugger.probe.Sampling;
 import com.datadog.debugger.sink.DebuggerSink;
 import com.datadog.debugger.util.ExceptionHelper;
+import datadog.environment.JavaVirtualMachine;
 import datadog.trace.api.Config;
 import datadog.trace.bootstrap.debugger.DebuggerContext;
 import datadog.trace.bootstrap.debugger.ProbeId;
@@ -21,6 +22,9 @@
 import datadog.trace.relocate.api.RatelimitedLogger;
 import datadog.trace.util.TagsHelper;
 import java.lang.instrument.Instrumentation;
+import java.lang.reflect.Method;
+import java.lang.reflect.Parameter;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.EnumMap;
@@ -41,6 +45,8 @@
  */
 public class ConfigurationUpdater implements DebuggerContext.ProbeResolver, ConfigurationAcceptor {
 
+  private static final boolean JAVA_AT_LEAST_19 = JavaVirtualMachine.isJavaVersionAtLeast(19);
+
   public interface TransformerSupplier {
     DebuggerTransformer supply(
         Config tracerConfig,
@@ -178,13 +184,55 @@ private void handleProbesChanges(ConfigurationComparer changes, Configuration ne
     }
     List<Class<?>> changedClasses =
         finder.getAllLoadedChangedClasses(instrumentation.getAllLoadedClasses(), changes);
+    changedClasses = detectMethodParameters(changedClasses);
     retransformClasses(changedClasses);
     // ensures that we have at least re-transformed 1 class
     if (changedClasses.size() > 0) {
       LOGGER.debug("Re-transformation done");
     }
   }
 
+  /*
+   * Because of this bug (https://bugs.openjdk.org/browse/JDK-8240908), classes compiled with
+   * method parameters (javac -parameters) strip this attribute once retransformed
+   * Spring 6/Spring boot 3 rely exclusively on this attribute and may throw an exception
+   * if no attribute found.
+   */
+  private List<Class<?>> detectMethodParameters(List<Class<?>> changedClasses) {
+    if (JAVA_AT_LEAST_19) {
+      // bug is fixed since JDK19, no need to perform detection
+      return changedClasses;
+    }
+    List<Class<?>> result = new ArrayList<>();
+    for (Class<?> changedClass : changedClasses) {
+      Method[] declaredMethods = changedClass.getDeclaredMethods();
+      boolean addClass = true;
+      // capping scanning of methods to 100 to avoid generated class with thousand of methods
+      // assuming that in those first 100 methods there is at least one with at least one parameter
+      for (int methodIdx = 0; methodIdx < declaredMethods.length && methodIdx < 100; methodIdx++) {
+        Method method = declaredMethods[methodIdx];
+        Parameter[] parameters = method.getParameters();
+        if (parameters.length == 0) {
+          continue;
+        }
+        if (parameters[0].isNamePresent()) {
+          LOGGER.debug(
+              "Detecting method parameter: method={} param={}, Skipping retransforming this class",
+              method.getName(),
+              parameters[0].getName());
+          // skip the class: compiled with -parameters
+          addClass = false;
+        }
+        // we found at leat a method with one parameter if name is not present we can stop there
+        break;
+      }
+      if (addClass) {
+        result.add(changedClass);
+      }
+    }
+    return result;
+  }
+
   private void reportReceived(ConfigurationComparer changes) {
     for (ProbeDefinition def : changes.getAddedDefinitions()) {
       if (def instanceof ExceptionProbe) {

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java

@@ -24,6 +24,7 @@
 import com.datadog.debugger.uploader.BatchUploader;
 import com.datadog.debugger.util.ClassFileLines;
 import com.datadog.debugger.util.DebuggerMetrics;
+import datadog.environment.JavaVirtualMachine;
 import datadog.environment.SystemProperties;
 import datadog.trace.agent.tooling.AgentStrategies;
 import datadog.trace.api.Config;
@@ -61,6 +62,7 @@
 import org.objectweb.asm.ClassWriter;
 import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.Opcodes;
+import org.objectweb.asm.Type;
 import org.objectweb.asm.commons.JSRInlinerAdapter;
 import org.objectweb.asm.tree.ClassNode;
 import org.objectweb.asm.tree.MethodNode;
@@ -90,6 +92,7 @@ public class DebuggerTransformer implements ClassFileTransformer {
           SpanDecorationProbe.class,
           SpanProbe.class);
   private static final String JAVA_IO_TMPDIR = "java.io.tmpdir";
+  private static final boolean JAVA_AT_LEAST_19 = JavaVirtualMachine.isJavaVersionAtLeast(19);
 
   public static Path DUMP_PATH = Paths.get(SystemProperties.get(JAVA_IO_TMPDIR), "debugger");
 
@@ -258,6 +261,7 @@ public byte[] transform(
         return null;
       }
       ClassNode classNode = parseClassFile(classFilePath, classfileBuffer);
+      checkMethodParameters(classNode);
       boolean transformed =
           performInstrumentation(loader, fullyQualifiedClassName, definitions, classNode);
       if (transformed) {
@@ -276,6 +280,38 @@ public byte[] transform(
     return null;
   }
 
+  /*
+   * Because of this bug (https://bugs.openjdk.org/browse/JDK-8240908), classes compiled with
+   * method parameters (javac -parameters) strip this attribute once retransformed
+   * Spring 6/Spring boot 3 rely exclusively on this attribute and may throw an exception
+   * if no attribute found.
+   * Note: Even if the attribute is preserved when transforming at load time, the fact that we have
+   * instrumented the class, we will retransform for removing the instrumentation and then the
+   * attribute is stripped. That's why we are preventing it even at load time.
+   */
+  private void checkMethodParameters(ClassNode classNode) {
+    if (JAVA_AT_LEAST_19) {
+      // bug is fixed since JDK19, no need to perform check
+      return;
+    }
+    // capping scanning of methods to 100 to avoid generated class with thousand of methods
+    // assuming that in those first 100 methods there is at least one with at least one parameter
+    for (int methodIdx = 0; methodIdx < classNode.methods.size() && methodIdx < 100; methodIdx++) {
+      MethodNode methodNode = classNode.methods.get(methodIdx);
+      int argumentCount = Type.getArgumentCount(methodNode.desc);
+      if (argumentCount == 0) {
+        continue;
+      }
+      if (methodNode.parameters != null && !methodNode.parameters.isEmpty()) {
+        throw new RuntimeException(
+            "Method Parameters attribute detected, cannot instrument class " + classNode.name);
+      } else {
+        // we found at leat a method with one parameter if name is not present we can stop there
+        break;
+      }
+    }
+  }
+
   private boolean skipInstrumentation(ClassLoader loader, String classFilePath) {
     if (definitionMatcher.isEmpty()) {
       LOGGER.debug("No debugger definitions present.");

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java

@@ -44,6 +44,7 @@
 import com.datadog.debugger.util.MoshiSnapshotTestHelper;
 import com.datadog.debugger.util.TestSnapshotListener;
 import com.datadog.debugger.util.TestTraceInterceptor;
+import datadog.environment.JavaVirtualMachine;
 import datadog.trace.agent.tooling.TracerInstaller;
 import datadog.trace.api.Config;
 import datadog.trace.api.interceptor.MutableSpan;
@@ -2807,6 +2808,29 @@ public void captureExpressionsWithCaptureLimits() throws IOException, URISyntaxE
     assertEquals("depth", fldValue.getNotCapturedReason());
   }
 
+  @Test
+  public void methodParametersAttribute() throws IOException, URISyntaxException {
+    final String CLASS_NAME = "CapturedSnapshot01";
+    TestSnapshotListener listener =
+        installMethodProbeAtExit(CLASS_NAME, "main", "int (java.lang.String)");
+    Map<String, byte[]> buffers =
+        compile(CLASS_NAME, SourceCompiler.DebugInfo.ALL, "8", Arrays.asList("-parameters"));
+    Class<?> testClass = loadClass(CLASS_NAME, buffers);
+    int result = Reflect.onClass(testClass).call("main", "1").get();
+    assertEquals(3, result);
+    if (JavaVirtualMachine.isJavaVersionAtLeast(19)) {
+      Snapshot snapshot = assertOneSnapshot(listener);
+      assertCaptureArgs(snapshot.getCaptures().getReturn(), "arg", String.class.getTypeName(), "1");
+    } else {
+      assertEquals(0, listener.snapshots.size());
+      ArgumentCaptor<ProbeId> probeIdCaptor = ArgumentCaptor.forClass(ProbeId.class);
+      ArgumentCaptor<String> strCaptor = ArgumentCaptor.forClass(String.class);
+      verify(probeStatusSink, times(1)).addError(probeIdCaptor.capture(), strCaptor.capture());
+      assertEquals(PROBE_ID.getId(), probeIdCaptor.getAllValues().get(0).getId());
+      assertEquals("Instrumentation fails for CapturedSnapshot01", strCaptor.getAllValues().get(0));
+    }
+  }
+
   private TestSnapshotListener setupInstrumentTheWorldTransformer(
       String excludeFileName, String includeFileName) {
     Config config = mock(Config.class);

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/ConfigurationUpdaterTest.java

@@ -13,6 +13,8 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
+import static utils.InstrumentationTestHelper.compile;
+import static utils.InstrumentationTestHelper.loadClass;
 
 import com.datadog.debugger.el.DSL;
 import com.datadog.debugger.el.ProbeCondition;
@@ -23,11 +25,14 @@
 import com.datadog.debugger.probe.SpanProbe;
 import com.datadog.debugger.sink.DebuggerSink;
 import com.datadog.debugger.sink.ProbeStatusSink;
+import datadog.environment.JavaVirtualMachine;
 import datadog.trace.api.Config;
 import datadog.trace.bootstrap.debugger.ProbeId;
 import datadog.trace.bootstrap.debugger.ProbeImplementation;
+import java.io.IOException;
 import java.lang.instrument.Instrumentation;
 import java.lang.instrument.UnmodifiableClassException;
+import java.net.URISyntaxException;
 import java.util.*;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Function;
@@ -39,6 +44,7 @@
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
+import utils.SourceCompiler;
 
 @ExtendWith(MockitoExtension.class)
 public class ConfigurationUpdaterTest {
@@ -623,6 +629,30 @@ public void handleException() {
     verify(probeStatusSink).addError(eq(ProbeId.from(PROBE_ID.getId() + ":0")), eq(ex));
   }
 
+  @Test
+  public void methodParametersAttribute()
+      throws IOException, URISyntaxException, UnmodifiableClassException {
+    final String CLASS_NAME = "CapturedSnapshot01";
+    Map<String, byte[]> buffers =
+        compile(CLASS_NAME, SourceCompiler.DebugInfo.ALL, "8", Arrays.asList("-parameters"));
+    Class<?> testClass = loadClass(CLASS_NAME, buffers);
+    when(inst.getAllLoadedClasses()).thenReturn(new Class[] {testClass});
+    ConfigurationUpdater configurationUpdater = createConfigUpdater(debuggerSinkWithMockStatusSink);
+    configurationUpdater.accept(
+        REMOTE_CONFIG,
+        singletonList(
+            LogProbe.builder().probeId(PROBE_ID).where("CapturedSnapshot01", "main").build()));
+    if (JavaVirtualMachine.isJavaVersionAtLeast(19)) {
+      ArgumentCaptor<Class<?>[]> captor = ArgumentCaptor.forClass(Class[].class);
+      verify(inst, times(1)).retransformClasses(captor.capture());
+      List<Class<?>[]> allValues = captor.getAllValues();
+      assertEquals(testClass, allValues.get(0));
+    } else {
+      verify(inst).getAllLoadedClasses();
+      verify(inst, times(0)).retransformClasses(any());
+    }
+  }
+
   private DebuggerTransformer createTransformer(
       Config tracerConfig,
       Configuration configuration,

dd-java-agent/agent-debugger/src/test/java/utils/InstrumentationTestHelper.java

@@ -12,6 +12,7 @@
 import java.net.URLClassLoader;
 import java.nio.file.Files;
 import java.nio.file.Paths;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -52,8 +53,17 @@ public static Map<String, byte[]> compile(String className, String version)
   public static Map<String, byte[]> compile(
       String className, SourceCompiler.DebugInfo debugInfo, String version)
       throws IOException, URISyntaxException {
+    return compile(className, debugInfo, version, Collections.emptyList());
+  }
+
+  public static Map<String, byte[]> compile(
+      String className,
+      SourceCompiler.DebugInfo debugInfo,
+      String version,
+      List<String> additionalOptions)
+      throws IOException, URISyntaxException {
     String classSource = getFixtureContent("/" + className.replace('.', '/') + ".java");
-    return SourceCompiler.compile(className, classSource, debugInfo, version);
+    return SourceCompiler.compile(className, classSource, debugInfo, version, additionalOptions);
   }
 
   public static Class<?> loadClass(String className, String classFileName) throws IOException {

dd-java-agent/agent-debugger/src/test/java/utils/SourceCompiler.java

@@ -18,15 +18,19 @@ public enum DebugInfo {
   }
 
   public static Map<String, byte[]> compile(
-      String className, String source, DebugInfo debug, String version) {
+      String className,
+      String source,
+      DebugInfo debug,
+      String version,
+      List<String> additionalOptions) {
     JavaCompiler jc = ToolProvider.getSystemJavaCompiler();
     if (jc == null) throw new RuntimeException("Compiler unavailable");
 
     JavaSourceFromString jsfs = new JavaSourceFromString(className, source);
 
     Iterable<? extends JavaFileObject> fileObjects = Collections.singletonList(jsfs);
 
-    List<String> options = new ArrayList<>();
+    List<String> options = new ArrayList<>(additionalOptions);
     switch (debug) {
       case ALL:
         {