Fix method parameters JVM bug

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,60 @@ 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()) {
+          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) {
@@ -198,6 +251,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/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,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 +63,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 +82,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 +93,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 +262,7 @@ public byte[] transform(
         return null;
       }
       ClassNode classNode = parseClassFile(classFilePath, classfileBuffer);
+      checkMethodParameters(classNode);
       boolean transformed =
           performInstrumentation(loader, fullyQualifiedClassName, definitions, classNode);
       if (transformed) {
@@ -271,11 +276,49 @@ 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()) {
+        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.");
@@ -491,7 +534,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();
@@ -615,8 +658,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/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;
@@ -598,6 +599,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;
@@ -2807,6 +2809,58 @@ 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 failed for CapturedSnapshot01: java.lang.RuntimeException: Method Parameters attribute detected, instrumentation not supported",
+          strCaptor.getAllValues().get(0));
+    }
+  }
+
+  @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);