Recover from duplicate class definition errors (#5185)

* Recover from duplicate class definition errors

* fix hotspot8

* Suppress dupicate class definiton errors only when helper classes were injected

* exit define class context when there is an exception, update pseudocode in comment

instrumentation/internal/internal-class-loader/javaagent-integration-tests/build.gradle.kts

@@ -3,6 +3,7 @@ plugins {
 }
 
 dependencies {
+  compileOnly("org.apache.commons:commons-lang3:3.12.0")
   testImplementation("org.apache.commons:commons-lang3:3.12.0")
 
   testInstrumentation(project(":instrumentation:internal:internal-class-loader:javaagent"))

instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestFailableCallable.java

@@ -0,0 +1,10 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package instrumentation;
+
+import org.apache.commons.lang3.function.FailableCallable;
+
+public interface TestFailableCallable<R, E extends Throwable> extends FailableCallable<R, E> {}

instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule2.java

@@ -0,0 +1,64 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package instrumentation;
+
+import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
+import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
+import static java.util.Collections.singletonList;
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static net.bytebuddy.matcher.ElementMatchers.none;
+
+import com.google.auto.service.AutoService;
+import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
+import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
+import java.util.List;
+import net.bytebuddy.asm.Advice;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+
+@AutoService(InstrumentationModule.class)
+public class TestInstrumentationModule2 extends InstrumentationModule {
+  public TestInstrumentationModule2() {
+    super("test-instrumentation2");
+  }
+
+  @Override
+  public List<TypeInstrumentation> typeInstrumentations() {
+    return singletonList(new TestTypeInstrumentation());
+  }
+
+  @Override
+  public boolean isHelperClass(String className) {
+    return "instrumentation.TestFailableCallable".equals(className);
+  }
+
+  public static class TestTypeInstrumentation implements TypeInstrumentation {
+    @Override
+    public ElementMatcher<ClassLoader> classLoaderOptimization() {
+      return hasClassesNamed("org.apache.commons.lang3.function.FailableCallable");
+    }
+
+    @Override
+    public ElementMatcher<TypeDescription> typeMatcher() {
+      return implementsInterface(named("org.apache.commons.lang3.function.FailableCallable"));
+    }
+
+    @Override
+    public void transform(TypeTransformer transformer) {
+      transformer.applyAdviceToMethod(
+          none(), TestInstrumentationModule2.class.getName() + "$TestAdvice");
+    }
+  }
+
+  @SuppressWarnings("unused")
+  public static class TestAdvice {
+    @Advice.OnMethodEnter(suppress = Throwable.class)
+    public static String onEnter() {
+      return TestFailableCallable.class.getName();
+    }
+  }
+}

instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/test/groovy/RegressionTest.groovy

@@ -0,0 +1,19 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification
+
+class RegressionTest extends AgentInstrumentationSpecification {
+
+  // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/5155
+  // loading a class that is extended/implemented by a helper class causes
+  // java.lang.LinkageError: loader 'app' (instance of jdk.internal.loader.ClassLoaders$AppClassLoader) attempted duplicate interface definition for org.apache.commons.lang3.function.FailableCallable
+  // this test verifies that the duplicate class definition LinkageError is not thrown into
+  // application code
+  def "test no duplicate class definition"() {
+    expect:
+    Class.forName("org.apache.commons.lang3.function.FailableCallable") != null
+  }
+}

instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentationModule.java

@@ -6,6 +6,7 @@
 package io.opentelemetry.javaagent.instrumentation.internal.classloader;
 
 import static java.util.Arrays.asList;
+import static java.util.Collections.singletonList;
 
 import com.google.auto.service.AutoService;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
@@ -29,8 +30,17 @@ public class ClassLoaderInstrumentationModule extends InstrumentationModule {
     return className.equals("io.opentelemetry.javaagent.tooling.Constants");
   }
 
+  @Override
+  public List<String> getAdditionalHelperClassNames() {
+    return singletonList(
+        "io.opentelemetry.javaagent.instrumentation.internal.classloader.DefineClassUtil");
+  }
+
   @Override
   public List<TypeInstrumentation> typeInstrumentations() {
-    return asList(new ClassLoaderInstrumentation(), new ResourceInjectionInstrumentation());
+    return asList(
+        new ClassLoaderInstrumentation(),
+        new ResourceInjectionInstrumentation(),
+        new DefineClassInstrumentation());
   }
 }

instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/DefineClassInstrumentation.java

@@ -0,0 +1,187 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.internal.classloader;
+
+import static net.bytebuddy.matcher.ElementMatchers.named;
+
+import io.opentelemetry.javaagent.bootstrap.DefineClassContext;
+import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
+import net.bytebuddy.asm.AsmVisitorWrapper;
+import net.bytebuddy.description.field.FieldDescription;
+import net.bytebuddy.description.field.FieldList;
+import net.bytebuddy.description.method.MethodList;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.implementation.Implementation;
+import net.bytebuddy.matcher.ElementMatcher;
+import net.bytebuddy.pool.TypePool;
+import org.objectweb.asm.ClassVisitor;
+import org.objectweb.asm.ClassWriter;
+import org.objectweb.asm.Label;
+import org.objectweb.asm.MethodVisitor;
+import org.objectweb.asm.Opcodes;
+import org.objectweb.asm.Type;
+
+public class DefineClassInstrumentation implements TypeInstrumentation {
+
+  @Override
+  public ElementMatcher<TypeDescription> typeMatcher() {
+    return named("java.lang.ClassLoader");
+  }
+
+  @Override
+  public void transform(TypeTransformer transformer) {
+    transformer.applyTransformer(
+        (builder, typeDescription, classLoader, module) ->
+            builder.visit(
+                new AsmVisitorWrapper() {
+                  @Override
+                  public int mergeWriter(int flags) {
+                    return flags | ClassWriter.COMPUTE_MAXS;
+                  }
+
+                  @Override
+                  public int mergeReader(int flags) {
+                    return flags;
+                  }
+
+                  @Override
+                  public ClassVisitor wrap(
+                      TypeDescription instrumentedType,
+                      ClassVisitor classVisitor,
+                      Implementation.Context implementationContext,
+                      TypePool typePool,
+                      FieldList<FieldDescription.InDefinedShape> fields,
+                      MethodList<?> methods,
+                      int writerFlags,
+                      int readerFlags) {
+                    return new ClassLoaderClassVisitor(classVisitor);
+                  }
+                }));
+  }
+
+  private static class ClassLoaderClassVisitor extends ClassVisitor {
+
+    ClassLoaderClassVisitor(ClassVisitor cv) {
+      super(Opcodes.ASM7, cv);
+    }
+
+    @Override
+    public MethodVisitor visitMethod(
+        int access, String name, String descriptor, String signature, String[] exceptions) {
+      MethodVisitor mv = super.visitMethod(access, name, descriptor, signature, exceptions);
+      // apply the following transformation to defineClass method
+      /*
+      DefineClassContext.enter();
+      try {
+        // original method body
+        DefineClassContext.exit();
+        return result;
+      } catch (LinkageError error) {
+        boolean helpersInjected = DefineClassContext.exitAndGet();
+        Class<?> loaded = findLoadedClass(className);
+        return DefineClassUtil.handleLinkageError(error, helpersInjected, loaded);
+      } catch (Throwable throwable) {
+        DefineClassContext.exit();
+        throw throwable;
+      }
+       */
+      if ("defineClass".equals(name)
+          && ("(Ljava/lang/String;[BIILjava/security/ProtectionDomain;)Ljava/lang/Class;"
+                  .equals(descriptor)
+              || "(Ljava/lang/String;Ljava/nio/ByteBuffer;Ljava/security/ProtectionDomain;)Ljava/lang/Class;"
+                  .equals(descriptor))) {
+        mv =
+            new MethodVisitor(api, mv) {
+              Label start = new Label();
+              Label end = new Label();
+              Label handler = new Label();
+
+              @Override
+              public void visitCode() {
+                mv.visitMethodInsn(
+                    Opcodes.INVOKESTATIC,
+                    Type.getInternalName(DefineClassContext.class),
+                    "enter",
+                    "()V",
+                    false);
+                mv.visitTryCatchBlock(start, end, end, "java/lang/LinkageError");
+                // catch other exceptions
+                mv.visitTryCatchBlock(start, end, handler, null);
+                mv.visitLabel(start);
+
+                super.visitCode();
+              }
+
+              @Override
+              public void visitInsn(int opcode) {
+                if (opcode == Opcodes.ARETURN) {
+                  mv.visitMethodInsn(
+                      Opcodes.INVOKESTATIC,
+                      Type.getInternalName(DefineClassContext.class),
+                      "exit",
+                      "()V",
+                      false);
+                }
+                super.visitInsn(opcode);
+              }
+
+              @Override
+              public void visitMaxs(int maxStack, int maxLocals) {
+                // handle LinkageError
+                mv.visitLabel(end);
+                mv.visitFrame(
+                    Opcodes.F_FULL,
+                    2,
+                    new Object[] {"java/lang/ClassLoader", "java/lang/String"},
+                    1,
+                    new Object[] {"java/lang/LinkageError"});
+                mv.visitMethodInsn(
+                    Opcodes.INVOKESTATIC,
+                    Type.getInternalName(DefineClassContext.class),
+                    "exitAndGet",
+                    "()Z",
+                    false);
+                mv.visitVarInsn(Opcodes.ALOAD, 0);
+                mv.visitVarInsn(Opcodes.ALOAD, 1);
+                mv.visitMethodInsn(
+                    Opcodes.INVOKEVIRTUAL,
+                    "java/lang/ClassLoader",
+                    "findLoadedClass",
+                    "(Ljava/lang/String;)Ljava/lang/Class;",
+                    false);
+                mv.visitMethodInsn(
+                    Opcodes.INVOKESTATIC,
+                    Type.getInternalName(DefineClassUtil.class),
+                    "handleLinkageError",
+                    "(Ljava/lang/LinkageError;ZLjava/lang/Class;)Ljava/lang/Class;",
+                    false);
+                mv.visitInsn(Opcodes.ARETURN);
+
+                // handle Throwable
+                mv.visitLabel(handler);
+                mv.visitFrame(
+                    Opcodes.F_FULL,
+                    2,
+                    new Object[] {"java/lang/ClassLoader", "java/lang/String"},
+                    1,
+                    new Object[] {"java/lang/Throwable"});
+                mv.visitMethodInsn(
+                    Opcodes.INVOKESTATIC,
+                    Type.getInternalName(DefineClassContext.class),
+                    "exit",
+                    "()V",
+                    false);
+                mv.visitInsn(Opcodes.ATHROW);
+
+                super.visitMaxs(maxStack, maxLocals);
+              }
+            };
+      }
+      return mv;
+    }
+  }
+}

instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/DefineClassUtil.java

@@ -0,0 +1,41 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.internal.classloader;
+
+@SuppressWarnings("unused")
+public final class DefineClassUtil {
+  private DefineClassUtil() {}
+
+  /**
+   * Handle LinkageError in ClassLoader.defineClass. Call to this method is inserted into
+   * ClassLoader.defineClass by DefineClassInstrumentation.
+   *
+   * @param linkageError LinkageError that happened in defineClass
+   * @param helpersInjected whether helpers were injected during defineClass call
+   * @param clazz Class that is being defined if it is already loaded
+   * @return give Class if LinkageError was a duplicate class definition error
+   */
+  public static Class<?> handleLinkageError(
+      LinkageError linkageError, boolean helpersInjected, Class<?> clazz) {
+    // only attempt to recover from duplicate class definition if helpers were injected during
+    // the defineClass call
+    if (!helpersInjected
+        // if exception was duplicate class definition we'll have access to the loaded class
+        || clazz == null
+        // duplicate class definition throws LinkageError, we can ignore its subclasses
+        || linkageError.getClass() != LinkageError.class) {
+      throw linkageError;
+    }
+    // check that the exception is a duplicate class or interface definition
+    String message = linkageError.getMessage();
+    if (message == null
+        || !(message.contains("duplicate interface definition")
+            || message.contains("duplicate class definition"))) {
+      throw linkageError;
+    }
+    return clazz;
+  }
+}

javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/DefineClassContext.java

@@ -0,0 +1,59 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.bootstrap;
+
+/** Context for tracking whether helper classes were injected during defining class. */
+public final class DefineClassContext {
+  private static final ThreadLocal<DefineClassContext> contextThreadLocal = new ThreadLocal<>();
+
+  private int counter;
+  private boolean helpersInjected;
+
+  private DefineClassContext() {}
+
+  /**
+   * Start defining class. Instrumentation inserts call to this method into ClassLoader.defineClass.
+   */
+  public static void enter() {
+    DefineClassContext context = contextThreadLocal.get();
+    if (context == null) {
+      context = new DefineClassContext();
+      contextThreadLocal.set(context);
+    }
+    context.counter++;
+  }
+
+  /**
+   * Finish defining class. Instrumentation inserts call to this method into
+   * ClassLoader.defineClass.
+   */
+  public static void exit() {
+    exitAndGet();
+  }
+
+  /**
+   * Finish defining class. Instrumentation inserts call to this method into
+   * ClassLoader.defineClass.
+   *
+   * @return true if helper classes were injected
+   */
+  public static boolean exitAndGet() {
+    DefineClassContext context = contextThreadLocal.get();
+    context.counter--;
+    if (context.counter == 0) {
+      contextThreadLocal.remove();
+    }
+    return context.helpersInjected;
+  }
+
+  /** Called when helper classes are injected. */
+  public static void helpersInjected() {
+    DefineClassContext context = contextThreadLocal.get();
+    if (context != null) {
+      context.helpersInjected = true;
+    }
+  }
+}

muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java

@@ -6,6 +6,7 @@
 package io.opentelemetry.javaagent.tooling;
 
 import io.opentelemetry.instrumentation.api.cache.Cache;
+import io.opentelemetry.javaagent.bootstrap.DefineClassContext;
 import io.opentelemetry.javaagent.bootstrap.HelperResources;
 import io.opentelemetry.javaagent.tooling.muzzle.HelperResource;
 import java.io.File;
@@ -195,6 +196,7 @@ public class HelperInjector implements Transformer {
         cl -> {
           try {
             logger.debug("Injecting classes onto classloader {} -> {}", cl, helperClassNames);
+            DefineClassContext.helpersInjected();
 
             Map<String, byte[]> classnameToBytes = getHelperMap();
             Map<String, Class<?>> classes;