Bytecode version upgrade for invokedynamic dispatch

javaagent-tooling/build.gradle.kts

@@ -8,6 +8,13 @@ plugins {
 
 group = "io.opentelemetry.javaagent"
 
+// known libraries compiled with pre java7 bytecode (<51)
+// this kind of dependency MUST NOT be upgraded as the test expects an old bytecode version
+val oldJavaDependencies = arrayOf(
+  "org.apache.commons:commons-lang3:3.5", // java 6
+  "javax.servlet:servlet-api:2.5"
+) // java 5
+
 dependencies {
   implementation(project(":javaagent-bootstrap"))
   implementation(project(":javaagent-extension-api"))
@@ -39,6 +46,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")
@@ -51,6 +59,10 @@ dependencies {
   testImplementation(project(":testing-common"))
   testImplementation("com.google.guava:guava")
   testImplementation("org.junit-pioneer:junit-pioneer")
+
+  oldJavaDependencies.forEach {
+    testCompileOnly(it)
+  }
 }
 
 testing {
@@ -77,6 +89,21 @@ 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")
+
+        oldJavaDependencies.forEach {
+          implementation(it)
+        }
+      }
+    }
   }
 }
 

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

@@ -0,0 +1,115 @@
+/*
+ * 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 (and somewhat dangerous) stack frame re-computation if stack
+      // frames are already
+      // present in the bytecode, which also allows eagerly loading types that might be present in
+      // the method
+      // body, but not yet loaded by the JVM.
+      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) {
+              private boolean patchVersion;
+
+              @Override
+              public void visit(
+                  int version,
+                  int access,
+                  String name,
+                  String signature,
+                  String superName,
+                  String[] interfaces) {
+                if (ClassFileVersion.ofMinorMajor(version).isLessThan(ClassFileVersion.JAVA_V7)) {
+                  patchVersion = true;
+                  //
+                  version = Opcodes.V1_7;
+                }
+                super.visit(version, 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);
+                if (patchVersion) {
+                  return new JSRInlinerAdapter(
+                      methodVisitor, access, name, descriptor, signature, exceptions);
+                } else {
+                  return methodVisitor;
+                }
+              }
+            };
+          }
+
+          @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/PatchBytecodeVersionTest.java

@@ -0,0 +1,181 @@
+/*
+ * 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.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.stream.Collectors;
+import java.util.stream.Stream;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServlet;
+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.apache.commons.lang3.StringUtils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.objectweb.asm.Opcodes;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+class PatchBytecodeVersionTest {
+
+  private static final String BYTEBUDDY_DUMP = "net.bytebuddy.dump";
+  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);
+                  }
+                })
+            // commons lang 3
+            .type(named("org.apache.commons.lang3.StringUtils"))
+            .transform(new PatchByteCodeVersionTransformer())
+            .transform(transformerFor(isMethod().and(named("startsWith"))))
+            // servlet 2.5
+            .type(named("javax.servlet.GenericServlet"))
+            .transform(new PatchByteCodeVersionTransformer())
+            .transform(transformerFor(isMethod().and(named("init"))));
+
+    ByteBuddyAgent.install();
+    transformer = builder.installOn(ByteBuddyAgent.getInstrumentation());
+  }
+
+  @AfterAll
+  static void tearDown() {
+    transformer.reset(
+        ByteBuddyAgent.getInstrumentation(), AgentBuilder.RedefinitionStrategy.RETRANSFORMATION);
+
+    System.clearProperty(BYTEBUDDY_DUMP);
+  }
+
+  @Test
+  void patchJava6() {
+    testVersionUpgrade(
+        Opcodes.V1_6,
+        () -> StringUtils.startsWith("", ""),
+        2,
+        "org.apache.commons.lang3.StringUtils");
+  }
+
+  @Test
+  void patchJava5() {
+    testVersionUpgrade(
+        Opcodes.V1_5,
+        () -> {
+          try {
+            new HttpServlet() {}.init();
+          } catch (ServletException e) {
+            throw new RuntimeException(e);
+          }
+        },
+        1,
+        "javax.servlet.GenericServlet");
+  }
+
+  private static void testVersionUpgrade(
+      int originalVersion, Runnable task, int expectedCountIncrement, String className) {
+    if (originalVersion >= Opcodes.V1_7) {
+      throw new IllegalArgumentException("must use pre-java7 bytecode");
+    }
+
+    int startCount = PatchTestAdvice.invocationCount.get();
+    assertThat(PatchTestAdvice.invocationCount.get()).isEqualTo(startCount);
+
+    task.run();
+
+    assertThat(PatchTestAdvice.invocationCount.get())
+        .isEqualTo(startCount + expectedCountIncrement);
+
+    Path instrumentedClass = null;
+    Path instrumentedClassOriginal = null;
+    try (Stream<Path> files =
+        Files.find(
+            tempDir,
+            1,
+            (path, attr) ->
+                Files.isRegularFile(path) && path.getFileName().toString().startsWith(className))) {
+
+      for (Path path : files.collect(Collectors.toList())) {
+        if (path.getFileName().toString().contains("-original")) {
+          instrumentedClassOriginal = path;
+        } else {
+          instrumentedClass = path;
+        }
+      }
+
+    } catch (IOException e) {
+      throw new IllegalStateException(e);
+    }
+
+    assertThat(instrumentedClass).exists();
+    assertThat(instrumentedClassOriginal).exists();
+
+    assertThat(getBytecodeVersion(instrumentedClassOriginal))
+        .describedAs(
+            "expected original bytecode for class '%s' should have been compiled for Java %d, see folder %s for bytecode dumps",
+            className, 7 - (Opcodes.V1_7 - originalVersion), tempDir)
+        .isEqualTo(originalVersion);
+
+    assertThat(getBytecodeVersion(instrumentedClass))
+        .describedAs(
+            "expected instrumented bytecode for class '%s' should have been upgraded to Java 7, see folder %s for bytecode dumps",
+            className, tempDir)
+        .isEqualTo(Opcodes.V1_7);
+  }
+
+  private static int getBytecodeVersion(Path file) {
+    byte[] bytecode;
+    try {
+      bytecode = Files.readAllBytes(file);
+      return bytecode[7];
+    } 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();
+  }
+}