Fix method parameters JVM bug (#10521)

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.

disable Scala test for JDK < 21

Add special case for Record

canonical record constructor has method parameters attribute

add error messages

fix test

spotless

fix test

Detect that we are running on Spring6+

spotbugs

Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>

Author: jpbempel

dd-java-agent/agent-debugger/build.gradle

@@ -67,6 +67,7 @@ dependencies {
 
   testRuntimeOnly group: 'org.scala-lang', name: 'scala-compiler', version: libs.versions.scala213.get()
   testRuntimeOnly group: 'antlr', name: 'antlr', version: '2.7.7'
+  testRuntimeOnly group: 'org.springframework', name: 'spring-web', version: '6.0.0'
 }
 
 tasks.named("shadowJar", ShadowJar) {

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

@@ -12,6 +12,8 @@
 import com.datadog.debugger.probe.Sampled;
 import com.datadog.debugger.sink.DebuggerSink;
 import com.datadog.debugger.util.ExceptionHelper;
+import com.datadog.debugger.util.SpringHelper;
+import datadog.environment.JavaVirtualMachine;
 import datadog.trace.api.Config;
 import datadog.trace.bootstrap.debugger.DebuggerContext;
 import datadog.trace.bootstrap.debugger.ProbeId;
@@ -20,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;
@@ -40,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,
@@ -177,13 +184,63 @@ private void handleProbesChanges(ConfigurationComparer changes, Configuration ne
     }
     List<Class<?>> changedClasses =
         finder.getAllLoadedChangedClasses(instrumentation.getAllLoadedClasses(), changes);
+    changedClasses = detectMethodParameters(changes, 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(
+      ConfigurationComparer changes, 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()) {
+          if (!SpringHelper.isSpringUsingOnlyMethodParameters(instrumentation)) {
+            return changedClasses;
+          }
+          LOGGER.debug(
+              "Detecting method parameter: method={} param={}, Skipping retransforming this class",
+              method.getName(),
+              parameters[0].getName());
+          // skip the class: compiled with -parameters
+          reportError(
+              changes,
+              "Method Parameters detected, instrumentation not supported for "
+                  + changedClass.getTypeName());
+          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) {
@@ -197,6 +254,16 @@ private void reportReceived(ConfigurationComparer changes) {
     }
   }
 
+  private void reportError(ConfigurationComparer changes, String errorMsg) {
+    for (ProbeDefinition def : changes.getAddedDefinitions()) {
+      if (def instanceof ExceptionProbe) {
+        // do not report received for exception probes
+        continue;
+      }
+      sink.addError(def.getProbeId(), errorMsg);
+    }
+  }
+
   private void installNewDefinitions(Configuration newConfiguration) {
     DebuggerContext.initClassFilter(new DenyListHelper(newConfiguration.getDenyList()));
     if (appliedDefinitions.isEmpty()) {

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

@@ -463,6 +463,10 @@ public static DebuggerSink getSink() {
     return sink;
   }
 
+  static Instrumentation getInstrumentation() {
+    return instrumentation;
+  }
+
   private static DebuggerTransformer createTransformer(
       Config config,
       Configuration configuration,

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

@@ -5,6 +5,7 @@
 import static java.util.stream.Collectors.toList;
 
 import com.datadog.debugger.el.ProbeCondition;
+import com.datadog.debugger.instrumentation.ASMHelper;
 import com.datadog.debugger.instrumentation.DiagnosticMessage;
 import com.datadog.debugger.instrumentation.InstrumentationResult;
 import com.datadog.debugger.instrumentation.MethodInfo;
@@ -24,6 +25,8 @@
 import com.datadog.debugger.uploader.BatchUploader;
 import com.datadog.debugger.util.ClassFileLines;
 import com.datadog.debugger.util.DebuggerMetrics;
+import com.datadog.debugger.util.SpringHelper;
+import datadog.environment.JavaVirtualMachine;
 import datadog.environment.SystemProperties;
 import datadog.trace.agent.tooling.AgentStrategies;
 import datadog.trace.api.Config;
@@ -61,6 +64,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;
@@ -79,7 +83,7 @@
 public class DebuggerTransformer implements ClassFileTransformer {
   private static final Logger LOGGER = LoggerFactory.getLogger(DebuggerTransformer.class);
   private static final String CANNOT_FIND_METHOD = "Cannot find method %s::%s%s";
-  private static final String INSTRUMENTATION_FAILS = "Instrumentation fails for %s";
+  private static final String INSTRUMENTATION_FAILS = "Instrumentation failed for %s: %s";
   private static final String CANNOT_FIND_LINE = "No executable code was found at %s:L%s";
   private static final Pattern COMMA_PATTERN = Pattern.compile(",");
   private static final List<Class<?>> PROBE_ORDER =
@@ -90,6 +94,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 +263,7 @@ public byte[] transform(
         return null;
       }
       ClassNode classNode = parseClassFile(classFilePath, classfileBuffer);
+      checkMethodParameters(classNode);
       boolean transformed =
           performInstrumentation(loader, fullyQualifiedClassName, definitions, classNode);
       if (transformed) {
@@ -271,11 +277,51 @@ public byte[] transform(
           "type {} matched but no transformation for definitions: {}", classFilePath, definitions);
     } catch (Throwable ex) {
       LOGGER.warn("Cannot transform: ", ex);
-      reportInstrumentationFails(definitions, fullyQualifiedClassName);
+      reportInstrumentationFails(definitions, fullyQualifiedClassName, ex.toString());
     }
     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;
+    }
+    boolean isRecord = ASMHelper.isRecord(classNode);
+    // 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 (isRecord && methodNode.name.equals("<init>")) {
+        // skip record constructors, cannot rely on them because of the canonical one
+        // use the equals method for this
+        continue;
+      }
+      if (methodNode.parameters != null
+          && !methodNode.parameters.isEmpty()
+          && SpringHelper.isSpringUsingOnlyMethodParameters(DebuggerAgent.getInstrumentation())) {
+        throw new RuntimeException(
+            "Method Parameters attribute detected, instrumentation not supported");
+      } 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.");
@@ -492,7 +538,7 @@ private byte[] writeClassFile(
       classNode.accept(visitor);
     } catch (Throwable t) {
       LOGGER.error("Cannot write classfile for class: {} Exception: ", classFilePath, t);
-      reportInstrumentationFails(definitions, Strings.getClassName(classFilePath));
+      reportInstrumentationFails(definitions, Strings.getClassName(classFilePath), t.toString());
       return null;
     }
     byte[] data = writer.toByteArray();
@@ -616,8 +662,9 @@ private void reportLocationNotFound(
     // on a separate class files because probe was set on an inner/top-level class
   }
 
-  private void reportInstrumentationFails(List<ProbeDefinition> definitions, String className) {
-    String msg = String.format(INSTRUMENTATION_FAILS, className);
+  private void reportInstrumentationFails(
+      List<ProbeDefinition> definitions, String className, String errorMsg) {
+    String msg = String.format(INSTRUMENTATION_FAILS, className, errorMsg);
     reportErrorForAllProbes(definitions, msg);
   }
 

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ASMHelper.java

@@ -138,6 +138,10 @@ public static boolean isFinalField(Field field) {
     return Modifier.isFinal(field.getModifiers());
   }
 
+  public static boolean isRecord(ClassNode classNode) {
+    return (classNode.access & Opcodes.ACC_RECORD) != 0;
+  }
+
   public static void invokeStatic(
       InsnList insnList,
       org.objectweb.asm.Type owner,

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/sink/DebuggerSink.java

@@ -208,6 +208,10 @@ public void addBlocked(ProbeId probeId) {
     probeStatusSink.addBlocked(probeId);
   }
 
+  public void addError(ProbeId probeId, String msg) {
+    probeStatusSink.addError(probeId, msg);
+  }
+
   public void removeDiagnostics(ProbeId probeId) {
     probeStatusSink.removeDiagnostics(probeId);
   }

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/util/SpringHelper.java

@@ -0,0 +1,19 @@
+package com.datadog.debugger.util;
+
+import java.lang.instrument.Instrumentation;
+
+public class SpringHelper {
+
+  public static boolean isSpringUsingOnlyMethodParameters(Instrumentation inst) {
+    for (Class<?> clazz : inst.getAllLoadedClasses()) {
+      if ("org.springframework.web.bind.annotation.ControllerMappingReflectiveProcessor"
+          .equals(clazz.getName())) {
+        // If this class (coming from Spring web since version 6) is found loaded it means Spring
+        // supports only getting parameter names from the MethodParameter attribute
+        return true;
+      }
+    }
+    // class not found, probably no Spring
+    return false;
+  }
+}

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;
@@ -64,6 +65,7 @@
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.lang.instrument.Instrumentation;
 import java.net.URISyntaxException;
 import java.net.URL;
 import java.util.ArrayList;
@@ -598,6 +600,7 @@ public void methodProbeLineProbeMix() throws IOException, URISyntaxException {
   }
 
   @Test
+  @EnabledForJreRange(min = JRE.JAVA_21)
   public void sourceFileProbeScala() throws IOException, URISyntaxException {
     final String CLASS_NAME = "CapturedSnapshot101";
     final String FILE_NAME = CLASS_NAME + SCALA_EXT;
@@ -2948,6 +2951,74 @@ public void captureExpressionsWithCaptureLimits() throws IOException, URISyntaxE
     assertEquals("depth", fldValue.getNotCapturedReason());
   }
 
+  @Test
+  public void methodParametersAttribute() throws Exception {
+    final String CLASS_NAME = "CapturedSnapshot01";
+    Config config = mock(Config.class);
+    when(config.isDebuggerCodeOriginEnabled()).thenReturn(false);
+    when(config.isDebuggerExceptionEnabled()).thenReturn(false);
+    when(config.isDynamicInstrumentationEnabled()).thenReturn(false);
+    Instrumentation inst = mock(Instrumentation.class);
+    if (JavaVirtualMachine.isJavaVersion(17)) {
+      // on JDK 17 introduced Spring6 class
+      Class<?> springClass =
+          Class.forName(
+              "org.springframework.web.bind.annotation.ControllerMappingReflectiveProcessor");
+      when(inst.getAllLoadedClasses()).thenReturn(new Class[] {springClass});
+    } else {
+      when(inst.getAllLoadedClasses()).thenReturn(new Class[0]);
+    }
+    DebuggerAgent.run(config, inst, null);
+    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.isJavaVersion(17)) {
+      // on JDK 17 with Spring6 class, transformation cannot happen
+      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 failed for CapturedSnapshot01: java.lang.RuntimeException: Method Parameters attribute detected, instrumentation not supported",
+          strCaptor.getAllValues().get(0));
+    } else {
+      Snapshot snapshot = assertOneSnapshot(listener);
+      assertCaptureArgs(snapshot.getCaptures().getReturn(), "arg", String.class.getTypeName(), "1");
+    }
+  }
+
+  @Test
+  @EnabledForJreRange(min = JRE.JAVA_17)
+  public void methodParametersAttributeRecord() throws IOException, URISyntaxException {
+    final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot29";
+    final String RECORD_NAME = "com.datadog.debugger.MyRecord1";
+    TestSnapshotListener listener = installMethodProbeAtExit(RECORD_NAME, "<init>", null);
+    Map<String, byte[]> buffers =
+        compile(CLASS_NAME, SourceCompiler.DebugInfo.ALL, "17", Arrays.asList("-parameters"));
+    Class<?> testClass = loadClass(CLASS_NAME, buffers);
+    int result = Reflect.onClass(testClass).call("main", "1").get();
+    assertEquals(42, result);
+    if (JavaVirtualMachine.isJavaVersionAtLeast(19)) {
+      Snapshot snapshot = assertOneSnapshot(listener);
+      assertCaptureArgs(
+          snapshot.getCaptures().getReturn(), "firstName", String.class.getTypeName(), "john");
+    } 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 failed for com.datadog.debugger.MyRecord1: java.lang.RuntimeException: Method Parameters attribute detected, instrumentation not supported",
+          strCaptor.getAllValues().get(0));
+    }
+  }
+
   private TestSnapshotListener setupInstrumentTheWorldTransformer(
       String excludeFileName, String includeFileName) {
     Config config = mock(Config.class);