From a1f10ce9bc30c273703c5c5d51a57ec690c9b27b Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Mon, 29 Nov 2021 22:09:28 +0200 Subject: [PATCH] Remove virtual field interfaces from reflection results (#4722) * Remove virtual field interfaces from reflection results * fix java8 and openj9 --- .../build.gradle.kts | 1 + .../src/test/groovy/ReflectionTest.groovy | 29 +++-- .../src/test/java/TestClass.java | 4 +- .../reflection/ClassInstrumentation.java | 105 ++++++++++++++++++ .../internal/reflection/ReflectionHelper.java | 24 ++++ .../ReflectionIgnoredTypesConfigurer.java | 1 + .../ReflectionInstrumentationModule.java | 4 +- .../bootstrap/VirtualFieldAccessorMarker.java | 9 ++ .../FieldAccessorInterfacesGenerator.java | 2 + 9 files changed, 164 insertions(+), 15 deletions(-) create mode 100644 instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ClassInstrumentation.java create mode 100644 javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/VirtualFieldAccessorMarker.java diff --git a/instrumentation/internal/internal-reflection/javaagent-integration-tests/build.gradle.kts b/instrumentation/internal/internal-reflection/javaagent-integration-tests/build.gradle.kts index 505254a5bbfa..96bd30660ff2 100644 --- a/instrumentation/internal/internal-reflection/javaagent-integration-tests/build.gradle.kts +++ b/instrumentation/internal/internal-reflection/javaagent-integration-tests/build.gradle.kts @@ -4,4 +4,5 @@ plugins { dependencies { testInstrumentation(project(":instrumentation:internal:internal-reflection:javaagent")) + testCompileOnly(project(":javaagent-bootstrap")) } diff --git a/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/groovy/ReflectionTest.groovy b/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/groovy/ReflectionTest.groovy index 9280e3d018a8..b9fb0843f831 100644 --- a/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/groovy/ReflectionTest.groovy +++ b/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/groovy/ReflectionTest.groovy @@ -4,7 +4,8 @@ */ import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification - +import io.opentelemetry.javaagent.bootstrap.VirtualFieldAccessorMarker +import io.opentelemetry.javaagent.bootstrap.VirtualFieldInstalledMarker import java.lang.reflect.Field import java.lang.reflect.Method @@ -37,16 +38,20 @@ class ReflectionTest extends AgentInstrumentationSpecification { methodFound == false and: - def interfaceClass = TestClass.getInterfaces().find { - it.getName().contains("VirtualFieldAccessor\$") - } - interfaceClass != null - def interfaceMethodFound = false - for (Method method : interfaceClass.getDeclaredMethods()) { - if (method.getName().contains("__opentelemetry")) { - interfaceMethodFound = true - } - } - interfaceMethodFound == false + // although marker interfaces are removed from getInterfaces() result class is still assignable + // to them + VirtualFieldInstalledMarker.isAssignableFrom(TestClass) + VirtualFieldAccessorMarker.isAssignableFrom(TestClass) + TestClass.getInterfaces().length == 2 + TestClass.getInterfaces() == [Runnable, Serializable] + } + + def "test generated serialVersionUID"() { + // expected value is computed with serialver utility that comes with jdk + expect: + ObjectStreamClass.lookup(TestClass).getSerialVersionUID() == -1508684692096503670L + + and: + TestClass.getDeclaredFields().length == 0 } } diff --git a/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/java/TestClass.java b/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/java/TestClass.java index 7c59dcd45e90..20d1fef8e12e 100644 --- a/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/java/TestClass.java +++ b/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/java/TestClass.java @@ -3,7 +3,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -public class TestClass implements Runnable { +import java.io.Serializable; + +public class TestClass implements Runnable, Serializable { @Override public void run() {} diff --git a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ClassInstrumentation.java b/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ClassInstrumentation.java new file mode 100644 index 000000000000..60870f214e02 --- /dev/null +++ b/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ClassInstrumentation.java @@ -0,0 +1,105 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.internal.reflection; + +import static net.bytebuddy.matcher.ElementMatchers.named; + +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.MethodVisitor; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.Type; + +public class ClassInstrumentation implements TypeInstrumentation { + @Override + public ElementMatcher typeMatcher() { + return named("java.lang.Class"); + } + + @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 fields, + MethodList methods, + int writerFlags, + int readerFlags) { + return new ClassClassVisitor(classVisitor); + } + })); + } + + private static class ClassClassVisitor extends ClassVisitor { + + ClassClassVisitor(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); + if ("getInterfaces".equals(name) + && ("()[Ljava/lang/Class;".equals(descriptor) + || "(Z)[Ljava/lang/Class;".equals(descriptor))) { + mv = + new MethodVisitor(api, mv) { + @Override + public void visitMethodInsn( + int opcode, String owner, String name, String descriptor, boolean isInterface) { + super.visitMethodInsn(opcode, owner, name, descriptor, isInterface); + // filter the result of call to getInterfaces0, which is used on hotspot, and + // J9VMInternals.getInterfaces which is used on openj9 + if (((opcode == Opcodes.INVOKEVIRTUAL || opcode == Opcodes.INVOKESPECIAL) + && "getInterfaces0".equals(name) + && "()[Ljava/lang/Class;".equals(descriptor)) + || (opcode == Opcodes.INVOKESTATIC + && "getInterfaces".equals(name) + && "java/lang/J9VMInternals".equals(owner) + && "(Ljava/lang/Class;)[Ljava/lang/Class;".equals(descriptor))) { + mv.visitVarInsn(Opcodes.ALOAD, 0); + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + Type.getInternalName(ReflectionHelper.class), + "filterInterfaces", + "([Ljava/lang/Class;Ljava/lang/Class;)[Ljava/lang/Class;", + false); + } + } + }; + } + return mv; + } + } +} diff --git a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionHelper.java b/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionHelper.java index f1738c075b50..723a70aecbd1 100644 --- a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionHelper.java +++ b/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionHelper.java @@ -5,6 +5,7 @@ package io.opentelemetry.javaagent.instrumentation.internal.reflection; +import io.opentelemetry.javaagent.bootstrap.VirtualFieldAccessorMarker; import io.opentelemetry.javaagent.bootstrap.VirtualFieldInstalledMarker; import java.lang.reflect.Field; import java.lang.reflect.Method; @@ -18,6 +19,7 @@ private ReflectionHelper() {} public static Field[] filterFields(Class containingClass, Field[] fields) { if (fields.length == 0 || !VirtualFieldInstalledMarker.class.isAssignableFrom(containingClass)) { + // nothing to filter when class does not have any added virtual fields return fields; } List result = new ArrayList<>(fields.length); @@ -36,6 +38,7 @@ public static Method[] filterMethods(Class containingClass, Method[] methods) return methods; } else if (containingClass.isInterface() && containingClass.isSynthetic() + && VirtualFieldAccessorMarker.class.isAssignableFrom(containingClass) && containingClass.getName().contains("VirtualFieldAccessor$")) { // hide all methods from virtual field accessor interfaces return new Method[0]; @@ -55,4 +58,25 @@ public static Method[] filterMethods(Class containingClass, Method[] methods) } return result.toArray(new Method[0]); } + + @SuppressWarnings("unused") + public static Class[] filterInterfaces(Class[] interfaces, Class containingClass) { + if (interfaces.length == 0 + || !VirtualFieldInstalledMarker.class.isAssignableFrom(containingClass)) { + // nothing to filter when class does not have any added virtual fields + return interfaces; + } + List> result = new ArrayList<>(interfaces.length); + for (Class interfaceClass : interfaces) { + // filter out virtual field marker and accessor interfaces + if (interfaceClass == VirtualFieldInstalledMarker.class + || (VirtualFieldAccessorMarker.class.isAssignableFrom(interfaceClass) + && interfaceClass.isSynthetic() + && interfaceClass.getName().contains("VirtualFieldAccessor$"))) { + continue; + } + result.add(interfaceClass); + } + return result.toArray(new Class[0]); + } } diff --git a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionIgnoredTypesConfigurer.java b/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionIgnoredTypesConfigurer.java index d6f4db1911bd..3026656e67f1 100644 --- a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionIgnoredTypesConfigurer.java +++ b/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionIgnoredTypesConfigurer.java @@ -17,5 +17,6 @@ public class ReflectionIgnoredTypesConfigurer implements IgnoredTypesConfigurer public void configure(Config config, IgnoredTypesBuilder builder) { builder.allowClass("jdk.internal.reflect.Reflection"); builder.allowClass("sun.reflect.Reflection"); + builder.allowClass("java.lang.Class"); } } diff --git a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentationModule.java b/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentationModule.java index d907d279f670..9b5690181d8e 100644 --- a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentationModule.java +++ b/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionInstrumentationModule.java @@ -5,7 +5,7 @@ package io.opentelemetry.javaagent.instrumentation.internal.reflection; -import static java.util.Collections.singletonList; +import static java.util.Arrays.asList; import com.google.auto.service.AutoService; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; @@ -26,6 +26,6 @@ public boolean defaultEnabled() { @Override public List typeInstrumentations() { - return singletonList(new ReflectionInstrumentation()); + return asList(new ClassInstrumentation(), new ReflectionInstrumentation()); } } diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/VirtualFieldAccessorMarker.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/VirtualFieldAccessorMarker.java new file mode 100644 index 000000000000..2107acdef3d5 --- /dev/null +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/VirtualFieldAccessorMarker.java @@ -0,0 +1,9 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.bootstrap; + +/** A marker interface implemented by virtual field accessor classes. */ +public interface VirtualFieldAccessorMarker {} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/field/FieldAccessorInterfacesGenerator.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/field/FieldAccessorInterfacesGenerator.java index 8ae9a45baa9c..36fe1383307b 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/field/FieldAccessorInterfacesGenerator.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/field/FieldAccessorInterfacesGenerator.java @@ -9,6 +9,7 @@ import static io.opentelemetry.javaagent.tooling.field.GeneratedVirtualFieldNames.getRealGetterName; import static io.opentelemetry.javaagent.tooling.field.GeneratedVirtualFieldNames.getRealSetterName; +import io.opentelemetry.javaagent.bootstrap.VirtualFieldAccessorMarker; import io.opentelemetry.javaagent.tooling.muzzle.VirtualFieldMappings; import java.util.HashMap; import java.util.Map; @@ -54,6 +55,7 @@ private DynamicType.Unloaded makeFieldAccessorInterface( .makeInterface() .merge(SyntheticState.SYNTHETIC) .name(getFieldAccessorInterfaceName(typeName, fieldTypeName)) + .implement(VirtualFieldAccessorMarker.class) .defineMethod( getRealGetterName(typeName, fieldTypeName), fieldTypeDesc,