diff --git a/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java b/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java index c8126cf78bb5..e4044ababaa7 100644 --- a/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java +++ b/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java @@ -36,11 +36,6 @@ public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) "test-resources/test-resource.txt", "test-resources/test-resource-2.txt"); } - @Override - public boolean isIndyModule() { - return false; - } - public static class TestTypeInstrumentation implements TypeInstrumentation { @Override public ElementMatcher classLoaderOptimization() { diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationHelper.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationHelper.java new file mode 100644 index 000000000000..77da44883b58 --- /dev/null +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationHelper.java @@ -0,0 +1,84 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.internal.classloader; + +import static java.util.logging.Level.WARNING; + +import io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder; +import io.opentelemetry.javaagent.bootstrap.CallDepth; +import io.opentelemetry.javaagent.tooling.Constants; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; +import java.util.List; +import java.util.logging.Logger; + +public class BootDelegationHelper { + + private BootDelegationHelper() {} + + public static class Holder { + + public static final List bootstrapPackagesPrefixes = findBootstrapPackagePrefixes(); + + /** + * We have to make sure that {@link BootstrapPackagePrefixesHolder} is loaded from bootstrap + * class loader. After that we can use in {@link BootDelegationHelper}. + */ + private static List findBootstrapPackagePrefixes() { + try { + Class holderClass = + Class.forName( + "io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder", true, null); + MethodHandle methodHandle = + MethodHandles.publicLookup() + .findStatic( + holderClass, "getBoostrapPackagePrefixes", MethodType.methodType(List.class)); + //noinspection unchecked + return (List) methodHandle.invokeExact(); + } catch (Throwable e) { + Logger.getLogger(Holder.class.getName()) + .log(WARNING, "Unable to load bootstrap package prefixes from the bootstrap CL", e); + return Constants.BOOTSTRAP_PACKAGE_PREFIXES; + } + } + + private Holder() {} + } + + public static Class onEnter(String name) { + // need to use call depth here to prevent re-entry from call to Class.forName() below + // because on some JVMs (e.g. IBM's, though IBM bootstrap loader is explicitly excluded above) + // Class.forName() ends up calling loadClass() on the bootstrap loader which would then come + // back to this instrumentation over and over, causing a StackOverflowError + CallDepth callDepth = CallDepth.forClass(ClassLoader.class); + if (callDepth.getAndIncrement() > 0) { + callDepth.decrementAndGet(); + return null; + } + + try { + for (String prefix : Holder.bootstrapPackagesPrefixes) { + if (name.startsWith(prefix)) { + try { + System.out.println("trying bootstrap for class " + name); + return Class.forName(name, false, null); + } catch (ClassNotFoundException ignored) { + // Ignore + } + } + } + } finally { + // need to reset it right away, not waiting until method exit + // otherwise it will prevent this instrumentation from being applied when loadClass() + // ends up calling a ClassFileTransformer which ends up calling loadClass() further down the + // stack on one of our bootstrap packages (since the call depth check would then suppress + // the nested loadClass instrumentation) + callDepth.decrementAndGet(); + } + return null; + } +} diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java index 74ef07ba249c..29188bbf24be 100644 --- a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java @@ -6,28 +6,12 @@ package io.opentelemetry.javaagent.instrumentation.internal.classloader; import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.extendsClass; -import static java.util.logging.Level.WARNING; -import static net.bytebuddy.matcher.ElementMatchers.isMethod; -import static net.bytebuddy.matcher.ElementMatchers.isProtected; -import static net.bytebuddy.matcher.ElementMatchers.isPublic; -import static net.bytebuddy.matcher.ElementMatchers.isStatic; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.namedOneOf; import static net.bytebuddy.matcher.ElementMatchers.not; -import static net.bytebuddy.matcher.ElementMatchers.takesArgument; -import static net.bytebuddy.matcher.ElementMatchers.takesArguments; -import io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder; -import io.opentelemetry.javaagent.bootstrap.CallDepth; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import io.opentelemetry.javaagent.tooling.Constants; -import java.lang.invoke.MethodHandle; -import java.lang.invoke.MethodHandles; -import java.lang.invoke.MethodType; -import java.util.List; -import java.util.logging.Logger; -import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -56,93 +40,7 @@ public ElementMatcher typeMatcher() { @Override public void transform(TypeTransformer transformer) { - transformer.applyAdviceToMethod( - isMethod() - .and(named("loadClass")) - .and( - takesArguments(1) - .and(takesArgument(0, String.class)) - .or( - takesArguments(2) - .and(takesArgument(0, String.class)) - .and(takesArgument(1, boolean.class)))) - .and(isPublic().or(isProtected())) - .and(not(isStatic())), - BootDelegationInstrumentation.class.getName() + "$LoadClassAdvice"); - } - - public static class Holder { - - public static final List bootstrapPackagesPrefixes = findBootstrapPackagePrefixes(); - - /** - * We have to make sure that {@link BootstrapPackagePrefixesHolder} is loaded from bootstrap - * class loader. After that we can use in {@link LoadClassAdvice}. - */ - private static List findBootstrapPackagePrefixes() { - try { - Class holderClass = - Class.forName( - "io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder", true, null); - MethodHandle methodHandle = - MethodHandles.publicLookup() - .findStatic( - holderClass, "getBoostrapPackagePrefixes", MethodType.methodType(List.class)); - //noinspection unchecked - return (List) methodHandle.invokeExact(); - } catch (Throwable e) { - Logger.getLogger(Holder.class.getName()) - .log(WARNING, "Unable to load bootstrap package prefixes from the bootstrap CL", e); - return Constants.BOOTSTRAP_PACKAGE_PREFIXES; - } - } - - private Holder() {} - } - - @SuppressWarnings("unused") - public static class LoadClassAdvice { - - @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class) - public static Class onEnter(@Advice.Argument(0) String name) { - // need to use call depth here to prevent re-entry from call to Class.forName() below - // because on some JVMs (e.g. IBM's, though IBM bootstrap loader is explicitly excluded above) - // Class.forName() ends up calling loadClass() on the bootstrap loader which would then come - // back to this instrumentation over and over, causing a StackOverflowError - CallDepth callDepth = CallDepth.forClass(ClassLoader.class); - if (callDepth.getAndIncrement() > 0) { - callDepth.decrementAndGet(); - return null; - } - - try { - for (String prefix : Holder.bootstrapPackagesPrefixes) { - if (name.startsWith(prefix)) { - try { - return Class.forName(name, false, null); - } catch (ClassNotFoundException ignored) { - // Ignore - } - } - } - } finally { - // need to reset it right away, not waiting until onExit() - // otherwise it will prevent this instrumentation from being applied when loadClass() - // ends up calling a ClassFileTransformer which ends up calling loadClass() further down the - // stack on one of our bootstrap packages (since the call depth check would then suppress - // the nested loadClass instrumentation) - callDepth.decrementAndGet(); - } - return null; - } - - @Advice.OnMethodExit(onThrowable = Throwable.class) - public static void onExit( - @Advice.Return(readOnly = false) Class result, - @Advice.Enter Class resultFromBootstrapLoader) { - if (resultFromBootstrapLoader != null) { - result = resultFromBootstrapLoader; - } - } + // we must use inlined instrumentation with ASM to prevent stack overflow errors + transformer.applyTransformer(ClassLoaderAsmUtil.getBootDelegationTransformer()); } } diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderAsmUtil.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderAsmUtil.java new file mode 100644 index 000000000000..55975cdac5f2 --- /dev/null +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderAsmUtil.java @@ -0,0 +1,228 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.internal.classloader; + +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import io.opentelemetry.javaagent.bootstrap.DefineClassHelper; +import io.opentelemetry.javaagent.bootstrap.InjectedClassHelper; +import io.opentelemetry.javaagent.extension.instrumentation.internal.AsmApi; +import java.util.function.Function; +import net.bytebuddy.agent.builder.AgentBuilder; +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.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; + +class ClassLoaderAsmUtil { + + private ClassLoaderAsmUtil() {} + + public static AgentBuilder.Transformer getBootDelegationTransformer() { + return getTransformer( + classVisitor -> + new LoadClassVisitor( + classVisitor, + Type.getInternalName(BootDelegationHelper.class), + "onEnter", + "(Ljava/lang/String;)Ljava/lang/Class;", + false)); + } + + public static AgentBuilder.Transformer getInjectedClassTransformer() { + return getTransformer( + classVisitor -> + new LoadClassVisitor( + classVisitor, + Type.getInternalName(InjectedClassHelper.class), + "loadHelperClass", + "(Ljava/lang/ClassLoader;Ljava/lang/String;)Ljava/lang/Class;", + true)); + } + + public static AgentBuilder.Transformer getDefineClassTransformer() { + return getTransformer(classVisitor -> new DefineClassVisitor(classVisitor)); + } + + private static AgentBuilder.Transformer getTransformer( + Function wrapper) { + return (builder, typeDescription, classLoader, javaModule, protectionDomain) -> + builder.visit( + new AsmVisitorWrapper() { + + @Override + public int mergeWriter(int flags) { + return flags | ClassWriter.COMPUTE_MAXS | ClassWriter.COMPUTE_FRAMES; + } + + @Override + @CanIgnoreReturnValue + public int mergeReader(int flags) { + return flags; + } + + @Override + public ClassVisitor wrap( + TypeDescription instrumentedType, + ClassVisitor classVisitor, + Implementation.Context implementationContext, + TypePool typePool, + FieldList fields, + MethodList methods, + int writerFlags, + int readerFlags) { + return wrapper.apply(classVisitor); + } + }); + } + + private static class LoadClassVisitor extends ClassVisitor { + private final String className; + private final String methodName; + private final String methodDescriptor; + private final boolean loadThis; + + protected LoadClassVisitor( + ClassVisitor classVisitor, + String className, + String methodName, + String methodDescriptor, + boolean loadThis) { + super(AsmApi.VERSION, classVisitor); + this.className = className; + this.methodName = methodName; + this.methodDescriptor = methodDescriptor; + this.loadThis = loadThis; + } + + @Override + public MethodVisitor visitMethod( + int access, String name, String descriptor, String signature, String[] exceptions) { + MethodVisitor mv = super.visitMethod(access, name, descriptor, signature, exceptions); + + boolean v1 = "(Ljava/lang/String)Ljava/lang/Class;".equals(signature); + boolean v2 = "(Ljava/lang/String;Z)Ljava/lang/Class;".equals(signature); + if ("loadClass".equals(name) + && (access & (Opcodes.ACC_PUBLIC | Opcodes.ACC_PROTECTED)) > 0 + && (access & Opcodes.ACC_STATIC) == 0 + && (v1 || v2)) { + + mv = + new MethodVisitor(api, mv) { + @Override + public void visitCode() { + if (loadThis) { + mv.visitVarInsn(Opcodes.ALOAD, 0); + } + // load class name argument + mv.visitVarInsn(Opcodes.ALOAD, 1); + // invoke helper + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, className, methodName, methodDescriptor, false); + // duplicate helper return value on stack + mv.visitInsn(Opcodes.DUP); + // go to injected code end when returned value is null + Label endLabel = new Label(); + mv.visitJumpInsn(Opcodes.IFNULL, endLabel); + // if not null, return with value from helper + mv.visitInsn(Opcodes.ARETURN); + // injected code end + mv.visitLabel(endLabel); + + // insert original method code + super.visitCode(); + } + }; + } + return mv; + } + } + + private static class DefineClassVisitor extends ClassVisitor { + + protected DefineClassVisitor(ClassVisitor classVisitor) { + super(AsmApi.VERSION, classVisitor); + } + + @Override + public MethodVisitor visitMethod( + int access, String name, String descriptor, String signature, String[] exceptions) { + + String d1 = "(Ljava/lang/String;[BIILjava/security/ProtectionDomain;)Ljava/lang/Class;"; + String d2 = + "(Ljava/lang/String;Ljava/nio/ByteBuffer;Ljava/security/ProtectionDomain;)Ljava/lang/Class;"; + MethodVisitor mv = super.visitMethod(access, name, descriptor, signature, exceptions); + + boolean v1 = d1.equals(descriptor); + boolean v2 = d2.equals(descriptor); + if ("defineClass".equals(name) && (v1 || v2)) { + mv = + new MethodVisitor(api, mv) { + @Override + public void visitCode() { + Label beforeBody = new Label(); + Label afterBody = new Label(); + Label handler = new Label(); + + mv.visitTryCatchBlock(beforeBody, afterBody, handler, null); + + // enter advice + mv.visitVarInsn(Opcodes.ALOAD, 0); + mv.visitVarInsn(Opcodes.ALOAD, 1); + mv.visitVarInsn(Opcodes.ALOAD, 2); + if (v1) { + mv.visitVarInsn(Opcodes.ILOAD, 3); + mv.visitVarInsn(Opcodes.ILOAD, 4); + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + Type.getInternalName(DefineClassHelper.class), + "beforeDefineClass", + "(Ljava/lang/ClassLoader;Ljava/lang/String;[BII)" + + "Lio/opentelemetry/javaagent/bootstrap/DefineClassHelper$Handler$DefineClassContext;", + false); + } else { + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + Type.getInternalName(DefineClassHelper.class), + "beforeDefineClass", + "(Ljava/lang/ClassLoader;Ljava/lang/String;Ljava/nio/ByteBuffer;)" + + "Lio/opentelemetry/javaagent/bootstrap/DefineClassHelper$Handler$DefineClassContext;", + false); + } + // helper return value on top of stack + + // start of try block + mv.visitLabel(beforeBody); + + // original method body + super.visitCode(); + + // end of try block + mv.visitLabel(afterBody); + + // finally block (exit advice) + mv.visitLabel(handler); + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + Type.getInternalName(DefineClassHelper.class), + "afterDefineClass", + "(Lio/opentelemetry/javaagent/bootstrap/DefineClassHelper$Handler$DefineClassContext;)V", + false); + } + }; + } + return mv; + } + } +} diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentationModule.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentationModule.java index 91fbb3b4baf9..fa588a91eac8 100644 --- a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentationModule.java +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentationModule.java @@ -10,11 +10,14 @@ 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.internal.ExperimentalInstrumentationModule; import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties; +import java.util.Collections; import java.util.List; @AutoService(InstrumentationModule.class) -public class ClassLoaderInstrumentationModule extends InstrumentationModule { +public class ClassLoaderInstrumentationModule extends InstrumentationModule + implements ExperimentalInstrumentationModule { public ClassLoaderInstrumentationModule() { super("internal-class-loader"); } @@ -25,11 +28,6 @@ public boolean defaultEnabled(ConfigProperties config) { return true; } - @Override - public boolean isIndyModule() { - return false; - } - @Override public boolean isHelperClass(String className) { return className.equals("io.opentelemetry.javaagent.tooling.Constants"); @@ -43,4 +41,10 @@ public List typeInstrumentations() { new ResourceInjectionInstrumentation(), new DefineClassInstrumentation()); } + + @Override + public List injectedClassNames() { + return Collections.singletonList( + "io.opentelemetry.javaagent.instrumentation.internal.classloader.BootDelegationHelper"); + } } diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/DefineClassInstrumentation.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/DefineClassInstrumentation.java index 1e3b63acf1a8..2e2da9db8402 100644 --- a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/DefineClassInstrumentation.java +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/DefineClassInstrumentation.java @@ -6,15 +6,9 @@ package io.opentelemetry.javaagent.instrumentation.internal.classloader; import static net.bytebuddy.matcher.ElementMatchers.named; -import static net.bytebuddy.matcher.ElementMatchers.takesArguments; -import io.opentelemetry.javaagent.bootstrap.DefineClassHelper; -import io.opentelemetry.javaagent.bootstrap.DefineClassHelper.Handler.DefineClassContext; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import java.nio.ByteBuffer; -import java.security.ProtectionDomain; -import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -27,50 +21,8 @@ public ElementMatcher typeMatcher() { @Override public void transform(TypeTransformer transformer) { - transformer.applyAdviceToMethod( - named("defineClass") - .and( - takesArguments( - String.class, byte[].class, int.class, int.class, ProtectionDomain.class)), - DefineClassInstrumentation.class.getName() + "$DefineClassAdvice"); - transformer.applyAdviceToMethod( - named("defineClass") - .and(takesArguments(String.class, ByteBuffer.class, ProtectionDomain.class)), - DefineClassInstrumentation.class.getName() + "$DefineClassWithThreeArgsAdvice"); - } - - @SuppressWarnings("unused") - public static class DefineClassAdvice { - @Advice.OnMethodEnter(suppress = Throwable.class) - public static DefineClassContext onEnter( - @Advice.This ClassLoader classLoader, - @Advice.Argument(0) String className, - @Advice.Argument(1) byte[] classBytes, - @Advice.Argument(2) int offset, - @Advice.Argument(3) int length) { - return DefineClassHelper.beforeDefineClass( - classLoader, className, classBytes, offset, length); - } - - @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - public static void onExit(@Advice.Enter DefineClassContext context) { - DefineClassHelper.afterDefineClass(context); - } - } - - @SuppressWarnings("unused") - public static class DefineClassWithThreeArgsAdvice { - @Advice.OnMethodEnter(suppress = Throwable.class) - public static DefineClassContext onEnter( - @Advice.This ClassLoader classLoader, - @Advice.Argument(0) String className, - @Advice.Argument(1) ByteBuffer classBytes) { - return DefineClassHelper.beforeDefineClass(classLoader, className, classBytes); - } - - @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - public static void onExit(@Advice.Enter DefineClassContext context) { - DefineClassHelper.afterDefineClass(context); - } + // getDefineClassTransformer + // we must use inlined instrumentation with ASM to prevent stack overflow errors + transformer.applyTransformer(ClassLoaderAsmUtil.getDefineClassTransformer()); } } diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/LoadInjectedClassInstrumentation.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/LoadInjectedClassInstrumentation.java index e39f0eef8459..6e50ac42af58 100644 --- a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/LoadInjectedClassInstrumentation.java +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/LoadInjectedClassInstrumentation.java @@ -6,19 +6,10 @@ package io.opentelemetry.javaagent.instrumentation.internal.classloader; import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.extendsClass; -import static net.bytebuddy.matcher.ElementMatchers.isMethod; -import static net.bytebuddy.matcher.ElementMatchers.isProtected; -import static net.bytebuddy.matcher.ElementMatchers.isPublic; -import static net.bytebuddy.matcher.ElementMatchers.isStatic; import static net.bytebuddy.matcher.ElementMatchers.named; -import static net.bytebuddy.matcher.ElementMatchers.not; -import static net.bytebuddy.matcher.ElementMatchers.takesArgument; -import static net.bytebuddy.matcher.ElementMatchers.takesArguments; -import io.opentelemetry.javaagent.bootstrap.InjectedClassHelper; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -35,41 +26,7 @@ public ElementMatcher typeMatcher() { @Override public void transform(TypeTransformer transformer) { - transformer.applyAdviceToMethod( - isMethod() - .and(named("loadClass")) - .and( - takesArguments(1) - .and(takesArgument(0, String.class)) - .or( - takesArguments(2) - .and(takesArgument(0, String.class)) - .and(takesArgument(1, boolean.class)))) - .and(isPublic().or(isProtected())) - .and(not(isStatic())), - LoadInjectedClassInstrumentation.class.getName() + "$LoadClassAdvice"); - } - - @SuppressWarnings("unused") - public static class LoadClassAdvice { - - @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class) - public static Class onEnter( - @Advice.This ClassLoader classLoader, @Advice.Argument(0) String name) { - Class helperClass = InjectedClassHelper.loadHelperClass(classLoader, name); - if (helperClass != null) { - return helperClass; - } - - return null; - } - - @Advice.OnMethodExit(onThrowable = Throwable.class) - public static void onExit( - @Advice.Return(readOnly = false) Class result, @Advice.Enter Class loadedClass) { - if (loadedClass != null) { - result = loadedClass; - } - } + // we must use inlined instrumentation with ASM to prevent stack overflow errors + transformer.applyTransformer(ClassLoaderAsmUtil.getInjectedClassTransformer()); } } diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ResourceInjectionInstrumentation.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ResourceInjectionInstrumentation.java index de47454033a8..9f625f04cdca 100644 --- a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ResourceInjectionInstrumentation.java +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ResourceInjectionInstrumentation.java @@ -60,38 +60,38 @@ public void transform(TypeTransformer transformer) { @SuppressWarnings("unused") public static class GetResourceAdvice { + @Advice.AssignReturned.ToReturned @Advice.OnMethodExit(suppress = Throwable.class) - public static void onExit( + public static URL onExit( @Advice.This ClassLoader classLoader, @Advice.Argument(0) String name, - @Advice.Return(readOnly = false) URL resource) { + @Advice.Return URL resource) { + if (resource != null) { - return; + return resource; } - URL helper = HelperResources.loadOne(classLoader, name); - if (helper != null) { - resource = helper; - } + return HelperResources.loadOne(classLoader, name); } } @SuppressWarnings("unused") public static class GetResourcesAdvice { + @Advice.AssignReturned.ToReturned @Advice.OnMethodExit(suppress = Throwable.class) - public static void onExit( + public static Enumeration onExit( @Advice.This ClassLoader classLoader, @Advice.Argument(0) String name, - @Advice.Return(readOnly = false) Enumeration resources) { + @Advice.Return Enumeration resources) { + List helpers = HelperResources.loadAll(classLoader, name); if (helpers.isEmpty()) { - return; + return resources; } if (!resources.hasMoreElements()) { - resources = Collections.enumeration(helpers); - return; + return Collections.enumeration(helpers); } List result = Collections.list(resources); @@ -109,29 +109,34 @@ public static void onExit( } } - resources = Collections.enumeration(result); + return Collections.enumeration(result); } } @SuppressWarnings("unused") public static class GetResourceAsStreamAdvice { + @Advice.AssignReturned.ToReturned @Advice.OnMethodExit(suppress = Throwable.class) - public static void onExit( + public static InputStream onExit( @Advice.This ClassLoader classLoader, @Advice.Argument(0) String name, - @Advice.Return(readOnly = false) InputStream inputStream) { + @Advice.Return InputStream inputStream) { + if (inputStream != null) { - return; + return inputStream; } URL helper = HelperResources.loadOne(classLoader, name); - if (helper != null) { - try { - inputStream = helper.openStream(); - } catch (IOException ignored) { - // ClassLoader.getResourceAsStream also ignores io exceptions from opening the stream - } + if (helper == null) { + return null; + } + + try { + return helper.openStream(); + } catch (IOException ignored) { + // ClassLoader.getResourceAsStream also ignores io exceptions from opening the stream + return null; } } } diff --git a/instrumentation/internal/internal-eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/osgi/EclipseOsgiInstrumentation.java b/instrumentation/internal/internal-eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/osgi/EclipseOsgiInstrumentation.java index e4a8bda14df1..bdbb385fb134 100644 --- a/instrumentation/internal/internal-eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/osgi/EclipseOsgiInstrumentation.java +++ b/instrumentation/internal/internal-eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/osgi/EclipseOsgiInstrumentation.java @@ -52,13 +52,15 @@ public static boolean onEnter(@Advice.Argument(0) String packageName) { return InClassLoaderMatcher.get() && !packageName.startsWith("io.opentelemetry."); } + @Advice.AssignReturned.ToReturned @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - public static void onExit( - @Advice.Return(readOnly = false) boolean result, - @Advice.Enter boolean inClassLoaderMatcher) { + public static boolean onExit( + @Advice.Return boolean result, @Advice.Enter boolean inClassLoaderMatcher) { + if (inClassLoaderMatcher) { - result = false; + return false; } + return result; } } } diff --git a/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/main/java/instrumentation/TestTypeInstrumentation.java b/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/main/java/instrumentation/TestTypeInstrumentation.java index e8c05c8c1b1d..b7a586958134 100644 --- a/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/main/java/instrumentation/TestTypeInstrumentation.java +++ b/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/main/java/instrumentation/TestTypeInstrumentation.java @@ -32,21 +32,21 @@ public void transform(TypeTransformer transformer) { @SuppressWarnings("unused") public static class TestAdvice { + @Advice.AssignReturned.ToReturned @Advice.OnMethodExit - public static void methodExit( - @Advice.This Runnable test, @Advice.Return(readOnly = false) String result) { + public static String methodExit(@Advice.This Runnable test, @Advice.Return String result) { VirtualField.find(Runnable.class, String.class).set(test, "instrumented"); - result = "instrumented"; + return "instrumented"; } } @SuppressWarnings("unused") public static class Test2Advice { + @Advice.AssignReturned.ToReturned @Advice.OnMethodExit - public static void methodExit( - @Advice.This Runnable test, @Advice.Return(readOnly = false) String result) { - result = VirtualField.find(Runnable.class, String.class).get(test); + public static String methodExit(@Advice.This Runnable test, @Advice.Return String result) { + return VirtualField.find(Runnable.class, String.class).get(test); } } } diff --git a/instrumentation/internal/internal-url-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestTypeInstrumentation.java b/instrumentation/internal/internal-url-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestTypeInstrumentation.java index fe84d88b82f1..d70ff1606f76 100644 --- a/instrumentation/internal/internal-url-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestTypeInstrumentation.java +++ b/instrumentation/internal/internal-url-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestTypeInstrumentation.java @@ -34,9 +34,10 @@ public void transform(TypeTransformer transformer) { @SuppressWarnings("unused") public static class GetHostNameAdvice { + @Advice.AssignReturned.ToReturned @Advice.OnMethodExit - public static void methodExit(@Advice.Return(readOnly = false) String hostName) { - hostName = "not-the-host-name"; + public static String methodExit(@Advice.Return String hostName) { + return "not-the-host-name"; } } } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java index ac1f46a99e3a..c2123833eae9 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java @@ -67,6 +67,13 @@ static byte[] transform(byte[] bytes) { })); TransformationContext context = new TransformationContext(); + if (justDelegateAdvice) { + // when delegating, advice return type should not be changed. For example we must not + // introduce skipOnIndex when skipOn is used in annotations as the advice return type + // is not an array + context.disableReturnTypeChange(); + } + ClassVisitor cv = new ClassVisitor(AsmApi.VERSION, cw) {