Bytecode version upgrade for invokedynamic dispatch (#9239)

javaagent-tooling/build.gradle.kts

@@ -39,6 +39,7 @@ dependencies {
   implementation("io.opentelemetry.contrib:opentelemetry-aws-xray-propagator")
 
   api("net.bytebuddy:byte-buddy-dep")
+  implementation("org.ow2.asm:asm-tree")
 
   annotationProcessor("com.google.auto.service:auto-service")
   compileOnly("com.google.auto.service:auto-service-annotations")
@@ -77,6 +78,17 @@ testing {
         compileOnly("com.google.code.findbugs:annotations")
       }
     }
+
+    val testPatchBytecodeVersion by registering(JvmTestSuite::class) {
+      dependencies {
+        implementation(project(":javaagent-bootstrap"))
+        implementation(project(":javaagent-tooling"))
+        implementation("net.bytebuddy:byte-buddy-dep")
+
+        // Used by byte-buddy but not brought in as a transitive dependency.
+        compileOnly("com.google.code.findbugs:annotations")
+      }
+    }
   }
 }
 

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/PatchByteCodeVersionTransformer.java

@@ -0,0 +1,103 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.tooling.instrumentation.indy;
+
+import static org.objectweb.asm.ClassWriter.COMPUTE_FRAMES;
+
+import java.security.ProtectionDomain;
+import net.bytebuddy.ClassFileVersion;
+import net.bytebuddy.agent.builder.AgentBuilder;
+import net.bytebuddy.asm.Advice;
+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.dynamic.DynamicType;
+import net.bytebuddy.implementation.Implementation;
+import net.bytebuddy.pool.TypePool;
+import net.bytebuddy.utility.JavaModule;
+import org.objectweb.asm.ClassVisitor;
+import org.objectweb.asm.MethodVisitor;
+import org.objectweb.asm.Opcodes;
+import org.objectweb.asm.commons.JSRInlinerAdapter;
+
+/**
+ * Patches the class file version to 51 (Java 7) in order to support injecting {@code INVOKEDYNAMIC}
+ * instructions via {@link Advice.WithCustomMapping#bootstrap} which is important for indy plugins.
+ */
+public class PatchByteCodeVersionTransformer implements AgentBuilder.Transformer {
+
+  private static boolean isAtLeastJava7(TypeDescription typeDescription) {
+    ClassFileVersion classFileVersion = typeDescription.getClassFileVersion();
+    return classFileVersion != null && classFileVersion.getJavaVersion() >= 7;
+  }
+
+  @Override
+  public DynamicType.Builder<?> transform(
+      DynamicType.Builder<?> builder,
+      TypeDescription typeDescription,
+      ClassLoader classLoader,
+      JavaModule javaModule,
+      ProtectionDomain protectionDomain) {
+
+    if (isAtLeastJava7(typeDescription)) {
+      // we can avoid the expensive stack frame re-computation if stack frames are already present
+      // in the bytecode.
+      return builder;
+    }
+    return builder.visit(
+        new AsmVisitorWrapper.AbstractBase() {
+          @Override
+          public ClassVisitor wrap(
+              TypeDescription typeDescription,
+              ClassVisitor classVisitor,
+              Implementation.Context context,
+              TypePool typePool,
+              FieldList<FieldDescription.InDefinedShape> fieldList,
+              MethodList<?> methodList,
+              int writerFlags,
+              int readerFlags) {
+
+            return new ClassVisitor(Opcodes.ASM7, classVisitor) {
+
+              @Override
+              public void visit(
+                  int version,
+                  int access,
+                  String name,
+                  String signature,
+                  String superName,
+                  String[] interfaces) {
+
+                super.visit(Opcodes.V1_7, access, name, signature, superName, interfaces);
+              }
+
+              @Override
+              public MethodVisitor visitMethod(
+                  int access,
+                  String name,
+                  String descriptor,
+                  String signature,
+                  String[] exceptions) {
+
+                MethodVisitor methodVisitor =
+                    super.visitMethod(access, name, descriptor, signature, exceptions);
+                return new JSRInlinerAdapter(
+                    methodVisitor, access, name, descriptor, signature, exceptions);
+              }
+            };
+          }
+
+          @Override
+          public int mergeWriter(int flags) {
+            // class files with version < Java 7 don't require a stack frame map
+            // as we're patching the version to at least 7, we have to compute the frames
+            return flags | COMPUTE_FRAMES;
+          }
+        });
+  }
+}

javaagent-tooling/src/testPatchBytecodeVersion/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/ComputeFramesAsmVisitorWrapper.java

@@ -0,0 +1,36 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.tooling.instrumentation.indy;
+
+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.jar.asm.ClassWriter;
+import net.bytebuddy.pool.TypePool;
+import org.objectweb.asm.ClassVisitor;
+
+public class ComputeFramesAsmVisitorWrapper extends AsmVisitorWrapper.AbstractBase {
+  @Override
+  public int mergeWriter(int flags) {
+    return super.mergeWriter(flags | ClassWriter.COMPUTE_FRAMES);
+  }
+
+  @Override
+  public ClassVisitor wrap(
+      TypeDescription instrumentedType,
+      ClassVisitor classVisitor,
+      Implementation.Context implementationContext,
+      TypePool typePool,
+      FieldList<FieldDescription.InDefinedShape> fields,
+      MethodList<?> methods,
+      int writerFlags,
+      int readerFlags) {
+    return classVisitor;
+  }
+}

javaagent-tooling/src/testPatchBytecodeVersion/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/OldBytecode.java

@@ -0,0 +1,150 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.tooling.instrumentation.indy;
+
+import java.lang.reflect.Modifier;
+import net.bytebuddy.ByteBuddy;
+import net.bytebuddy.ClassFileVersion;
+import net.bytebuddy.asm.AsmVisitorWrapper;
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.dynamic.DynamicType;
+import net.bytebuddy.dynamic.scaffold.InstrumentedType;
+import net.bytebuddy.implementation.Implementation;
+import net.bytebuddy.implementation.bytecode.ByteCodeAppender;
+import org.objectweb.asm.Label;
+import org.objectweb.asm.MethodVisitor;
+import org.objectweb.asm.Opcodes;
+import org.objectweb.asm.Type;
+
+public class OldBytecode {
+
+  private OldBytecode() {}
+
+  /**
+   * Generates and run a simple class with a {@link #toString()} implementation as if it had been
+   * compiled on an older java compiler
+   *
+   * @param className class name
+   * @param version bytecode version
+   * @return "toString"
+   */
+  public static String generateAndRun(String className, ClassFileVersion version) {
+    try (DynamicType.Unloaded<Object> unloadedClass = makeClass(className, version)) {
+      Class<?> generatedClass = unloadedClass.load(OldBytecode.class.getClassLoader()).getLoaded();
+
+      return generatedClass.getConstructor().newInstance().toString();
+
+    } catch (Throwable e) {
+      throw new RuntimeException(e);
+    }
+  }
+
+  private static DynamicType.Unloaded<Object> makeClass(
+      String className, ClassFileVersion version) {
+    return new ByteBuddy(version)
+        .subclass(Object.class)
+        // required otherwise stack frames aren't computed when needed
+        .visit(
+            version.isAtLeast(ClassFileVersion.JAVA_V7)
+                ? new ComputeFramesAsmVisitorWrapper()
+                : AsmVisitorWrapper.NoOp.INSTANCE)
+        .name(className)
+        .defineMethod("toString", String.class, Modifier.PUBLIC)
+        .intercept(new ToStringMethod())
+        .make();
+  }
+
+  private static class ToStringMethod implements Implementation, ByteCodeAppender {
+
+    @Override
+    public ByteCodeAppender appender(Target implementationTarget) {
+      return this;
+    }
+
+    @Override
+    public InstrumentedType prepare(InstrumentedType instrumentedType) {
+      return instrumentedType;
+    }
+
+    @Override
+    public Size apply(
+        MethodVisitor methodVisitor,
+        Context implementationContext,
+        MethodDescription instrumentedMethod) {
+
+      // Bytecode archeology:
+      //
+      // JSR and RET bytecode instructions were used to create "subroutines". Those were used
+      // in try/catch blocks as an attempt to avoid some bytecode duplication, this was later
+      // replaced with inlining.
+      // Starting from Java 5, no java compiler is expected to issue bytecode containing them and
+      // the JVM bytecode validation will reject it.
+      //
+      // Java 7 bytecode introduced the concept of "stack map frames", which describe the types of
+      // the objects that are stored on the stack during method body execution.
+      //
+      // As a consequence, the code below allows to test the following combinations:
+      // - java 1 to java 4 bytecode with JSR/RET opcodes
+      // - java 5 and java 6 bytecode without stack map frames
+      // - java 7 and later bytecode with stack map frames, those are automatically added by the
+      //   ComputeFramesAsmVisitorWrapper.
+      //
+      boolean useJsrRet =
+          implementationContext.getClassFileVersion().isLessThan(ClassFileVersion.JAVA_V5);
+
+      if (useJsrRet) {
+        // return "toString";
+        //
+        // using obsolete JSR/RET instructions
+        Label target = new Label();
+        methodVisitor.visitJumpInsn(Opcodes.JSR, target);
+
+        methodVisitor.visitVarInsn(Opcodes.ALOAD, 1);
+        methodVisitor.visitInsn(Opcodes.ARETURN);
+        methodVisitor.visitLabel(target);
+        methodVisitor.visitVarInsn(Opcodes.ASTORE, 2);
+        methodVisitor.visitLdcInsn("toString");
+        methodVisitor.visitVarInsn(Opcodes.ASTORE, 1);
+        methodVisitor.visitVarInsn(Opcodes.RET, 2);
+        return new Size(1, 3);
+      } else {
+        // try {
+        //   return "toString";
+        // } catch (Throwable e) {
+        //   return e.getMessage();
+        // }
+        //
+        // the Throwable exception is added to stack map frames with java7+, and needs to be
+        // added when upgrading the bytecode
+        Label start = new Label();
+        Label end = new Label();
+        Label handler = new Label();
+
+        methodVisitor.visitTryCatchBlock(
+            start, end, handler, Type.getInternalName(Throwable.class));
+        methodVisitor.visitLabel(start);
+        methodVisitor.visitLdcInsn("toString");
+        methodVisitor.visitLabel(end);
+
+        methodVisitor.visitInsn(Opcodes.ARETURN);
+
+        methodVisitor.visitLabel(handler);
+        methodVisitor.visitVarInsn(Opcodes.ASTORE, 1);
+        methodVisitor.visitVarInsn(Opcodes.ALOAD, 1);
+
+        methodVisitor.visitMethodInsn(
+            Opcodes.INVOKEVIRTUAL,
+            Type.getInternalName(Throwable.class),
+            "getMessage",
+            Type.getMethodDescriptor(Type.getType(String.class)),
+            false);
+        methodVisitor.visitInsn(Opcodes.ARETURN);
+
+        return new Size(1, 2);
+      }
+    }
+  }
+}

javaagent-tooling/src/testPatchBytecodeVersion/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/PatchBytecodeVersionTest.java

@@ -0,0 +1,173 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.tooling.instrumentation.indy;
+
+import static net.bytebuddy.matcher.ElementMatchers.isMethod;
+import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith;
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Stream;
+import net.bytebuddy.ClassFileVersion;
+import net.bytebuddy.agent.ByteBuddyAgent;
+import net.bytebuddy.agent.builder.AgentBuilder;
+import net.bytebuddy.agent.builder.ResettableClassFileTransformer;
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.dynamic.ClassFileLocator;
+import net.bytebuddy.matcher.ElementMatcher;
+import net.bytebuddy.utility.JavaModule;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+class PatchBytecodeVersionTest {
+
+  private static final String BYTEBUDDY_DUMP = "net.bytebuddy.dump";
+  public static final String ORIGINAL_SUFFIX = "-original";
+  public static final String CLASSNAME_PREFIX = "oldbytecode_";
+  private static ResettableClassFileTransformer transformer;
+
+  private static final Logger logger = LoggerFactory.getLogger(PatchBytecodeVersionTest.class);
+
+  private static Path tempDir;
+
+  @BeforeAll
+  static void setUp(@TempDir Path temp) {
+
+    assertThat(temp).isEmptyDirectory();
+    tempDir = temp;
+    System.setProperty(BYTEBUDDY_DUMP, temp.toString());
+
+    AgentBuilder builder =
+        new AgentBuilder.Default()
+            .disableClassFormatChanges()
+            .with(AgentBuilder.RedefinitionStrategy.RETRANSFORMATION)
+            .with(
+                new AgentBuilder.Listener.Adapter() {
+                  @Override
+                  public void onError(
+                      String typeName,
+                      ClassLoader classLoader,
+                      JavaModule module,
+                      boolean loaded,
+                      Throwable throwable) {
+                    logger.error("Transformation error", throwable);
+                  }
+                })
+            .type(nameStartsWith(CLASSNAME_PREFIX))
+            .transform(new PatchByteCodeVersionTransformer())
+            .transform(transformerFor(isMethod().and(named("toString"))));
+
+    ByteBuddyAgent.install();
+    transformer = builder.installOn(ByteBuddyAgent.getInstrumentation());
+  }
+
+  @AfterAll
+  static void tearDown() {
+    transformer.reset(
+        ByteBuddyAgent.getInstrumentation(), AgentBuilder.RedefinitionStrategy.RETRANSFORMATION);
+
+    System.clearProperty(BYTEBUDDY_DUMP);
+  }
+
+  @ParameterizedTest
+  @MethodSource("bytecodeVersions")
+  void upgradeBytecode(ClassFileVersion version) {
+
+    String className = CLASSNAME_PREFIX + version.getMinorMajorVersion();
+
+    int startCount = PatchTestAdvice.invocationCount.get();
+    assertThat(PatchTestAdvice.invocationCount.get()).isEqualTo(startCount);
+
+    assertThat(OldBytecode.generateAndRun(className, version)).isEqualTo("toString");
+
+    assertThat(PatchTestAdvice.invocationCount.get()).isEqualTo(startCount + 1);
+
+    Path instrumentedClass;
+    Path instrumentedClassOriginal;
+
+    try (Stream<Path> files =
+        Files.find(
+            tempDir,
+            1,
+            (path, attr) -> {
+              String fileName = path.getFileName().toString();
+              return Files.isRegularFile(path)
+                  && fileName.startsWith(className)
+                  && fileName.contains(ORIGINAL_SUFFIX);
+            })) {
+
+      instrumentedClassOriginal = files.findFirst().orElseThrow(IllegalStateException::new);
+      String upgradedClassFileName =
+          instrumentedClassOriginal.getFileName().toString().replace(ORIGINAL_SUFFIX, "");
+      instrumentedClass = instrumentedClassOriginal.resolveSibling(upgradedClassFileName);
+
+    } catch (IOException e) {
+      throw new IllegalStateException(e);
+    }
+
+    assertThat(instrumentedClass).exists();
+    assertThat(instrumentedClassOriginal).exists();
+
+    assertThat(getBytecodeVersion(instrumentedClassOriginal))
+        .describedAs(
+            "expected original bytecode for class '%s' in '%s' should have been compiled for %s",
+            className, instrumentedClassOriginal.toAbsolutePath(), version)
+        .isEqualTo(version);
+
+    if (version.isLessThan(ClassFileVersion.JAVA_V7)) {
+      assertThat(getBytecodeVersion(instrumentedClass))
+          .describedAs(
+              "expected instrumented bytecode for class '%s' in '%s' should have been upgraded to Java 7",
+              className, instrumentedClass.toAbsolutePath())
+          .isEqualTo(ClassFileVersion.JAVA_V7);
+    } else {
+      assertThat(getBytecodeVersion(instrumentedClass))
+          .describedAs("original bytecode version shouldn't be altered")
+          .isEqualTo(version);
+    }
+  }
+
+  static List<ClassFileVersion> bytecodeVersions() {
+    return Arrays.asList(
+        ClassFileVersion.JAVA_V1,
+        ClassFileVersion.JAVA_V2,
+        ClassFileVersion.JAVA_V3,
+        ClassFileVersion.JAVA_V4,
+        ClassFileVersion.JAVA_V5,
+        ClassFileVersion.JAVA_V6,
+        // Java 7 and later should not be upgraded
+        ClassFileVersion.JAVA_V7,
+        ClassFileVersion.JAVA_V8);
+  }
+
+  private static ClassFileVersion getBytecodeVersion(Path file) {
+    try {
+      byte[] bytecode = Files.readAllBytes(file);
+      return ClassFileVersion.ofMinorMajor((bytecode[5] << 16) | (bytecode[7] & 0xFF));
+    } catch (IOException e) {
+      throw new IllegalStateException(e);
+    }
+  }
+
+  private static AgentBuilder.Transformer.ForAdvice transformerFor(
+      ElementMatcher.Junction<MethodDescription> methodMatcher) {
+    return new AgentBuilder.Transformer.ForAdvice()
+        .with(
+            new AgentBuilder.LocationStrategy.Simple(
+                ClassFileLocator.ForClassLoader.of(PatchTestAdvice.class.getClassLoader())))
+        .advice(methodMatcher, PatchTestAdvice.class.getName());
+  }
+}

javaagent-tooling/src/testPatchBytecodeVersion/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/PatchTestAdvice.java

@@ -0,0 +1,20 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.tooling.instrumentation.indy;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import net.bytebuddy.asm.Advice;
+
+@SuppressWarnings("unused")
+public class PatchTestAdvice {
+
+  public static final AtomicInteger invocationCount = new AtomicInteger(0);
+
+  @Advice.OnMethodExit(suppress = Throwable.class)
+  public static void onExit() {
+    invocationCount.incrementAndGet();
+  }
+}