From ad9d1433c0817b5905db34c95a261d88f53351c9 Mon Sep 17 00:00:00 2001 From: mateuszrzeszutek Date: Fri, 11 Sep 2020 16:24:35 +0200 Subject: [PATCH 1/6] Muzzle should fail on unimplemented abstract methods --- .../muzzle/HelperReferenceWrapper.java | 278 ++++++++++++++++++ .../tooling/muzzle/MuzzleVisitor.java | 28 +- .../javaagent/tooling/muzzle/Reference.java | 40 ++- .../muzzle/ReferenceCreationPredicate.java | 47 +++ .../tooling/muzzle/ReferenceCreator.java | 145 ++++++--- .../tooling/muzzle/ReferenceMatcher.java | 123 ++++++-- .../ReferenceCreationPredicateTest.groovy | 45 +++ .../muzzle/HelperReferenceWrapperTest.groovy | 135 +++++++++ .../groovy/muzzle/ReferenceCreatorTest.groovy | 152 +++++++--- .../groovy/muzzle/ReferenceMatcherTest.groovy | 154 ++++++++-- .../instrumentation/TestHelperClasses.java | 64 ++++ .../HelperReferenceWrapperTestClasses.java | 33 +++ .../src/test/java/muzzle/TestClasses.java | 7 + 13 files changed, 1087 insertions(+), 164 deletions(-) create mode 100644 javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java create mode 100644 javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreationPredicate.java create mode 100644 javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreationPredicateTest.groovy create mode 100644 testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy create mode 100644 testing-common/src/test/java/io/opentelemetry/instrumentation/TestHelperClasses.java create mode 100644 testing-common/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java new file mode 100644 index 000000000000..f96825652fef --- /dev/null +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java @@ -0,0 +1,278 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.opentelemetry.javaagent.tooling.muzzle; + +import static java.util.Collections.emptyList; +import static java.util.Collections.singleton; +import static net.bytebuddy.description.method.MethodDescription.CONSTRUCTOR_INTERNAL_NAME; + +import com.google.common.base.Function; +import com.google.common.base.Objects; +import com.google.common.base.Predicate; +import com.google.common.collect.FluentIterable; +import com.google.common.collect.Iterables; +import io.opentelemetry.javaagent.tooling.muzzle.Reference.Flag; +import java.util.Map; +import net.bytebuddy.description.method.MethodDescription.InDefinedShape; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.pool.TypePool; +import net.bytebuddy.pool.TypePool.Resolution; + +/** This class provides a common interface for {@link Reference} and {@link TypeDescription}. */ +public interface HelperReferenceWrapper { + boolean isAbstract(); + + /** + * @return true if the wrapped type extends any class other than {@link Object} or implements any + * interface. + */ + boolean hasSuperTypes(); + + /** + * @return An iterable containing the wrapped type's super class (if exists) and implemented + * interfaces. + */ + Iterable getSuperTypes(); + + /** @return An iterable with all non-private, non-static methods declared in the wrapped type. */ + Iterable getMethods(); + + final class Method { + private final boolean isAbstract; + private final String declaringClass; + private final String name; + private final String descriptor; + + public Method(boolean isAbstract, String declaringClass, String name, String descriptor) { + this.isAbstract = isAbstract; + this.declaringClass = declaringClass; + this.name = name; + this.descriptor = descriptor; + } + + public boolean isAbstract() { + return isAbstract; + } + + public String getName() { + return name; + } + + public String getDescriptor() { + return descriptor; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Method method = (Method) o; + return Objects.equal(name, method.name) && Objects.equal(descriptor, method.descriptor); + } + + @Override + public int hashCode() { + return Objects.hashCode(name, descriptor); + } + + @Override + public String toString() { + return declaringClass + "#" + name + descriptor; + } + } + + class Factory { + private final TypePool classpathPool; + private final Map helperReferences; + + public Factory(TypePool classpathPool, Map helperReferences) { + this.classpathPool = classpathPool; + this.helperReferences = helperReferences; + } + + public HelperReferenceWrapper create(Reference reference) { + return new ReferenceType(reference); + } + + private HelperReferenceWrapper create(String className) { + if (helperReferences.containsKey(className)) { + return new ReferenceType(helperReferences.get(className)); + } + Resolution resolution = classpathPool.describe(className); + if (resolution.isResolved()) { + return new ClasspathType(resolution.resolve()); + } + throw new IllegalStateException("Missing class " + className); + } + + private final class ReferenceType implements HelperReferenceWrapper { + private final Reference reference; + + private ReferenceType(Reference reference) { + this.reference = reference; + } + + @Override + public boolean isAbstract() { + return reference.getFlags().contains(Flag.ABSTRACT); + } + + @Override + public boolean hasSuperTypes() { + return hasActualSuperType() || reference.getInterfaces().size() > 0; + } + + // Uses guava iterables to avoid unnecessary collection copying + @Override + public Iterable getSuperTypes() { + Iterable superClass = emptyList(); + if (hasActualSuperType()) { + superClass = singleton(Factory.this.create(reference.getSuperName())); + } + + Iterable interfaces = + FluentIterable.from(reference.getInterfaces()).transform(toWrapper()); + + return Iterables.concat(superClass, interfaces); + } + + private boolean hasActualSuperType() { + return reference.getSuperName() != null + && !reference.getSuperName().equals(Object.class.getName()); + } + + private Function toWrapper() { + return new Function() { + @Override + public HelperReferenceWrapper apply(String interfaceName) { + return Factory.this.create(interfaceName); + } + }; + } + + // Uses guava iterables to avoid unnecessary collection copying + @Override + public Iterable getMethods() { + return FluentIterable.from(reference.getMethods()) + .filter(isOverrideable()) + .transform(toMethod()); + } + + private Predicate isOverrideable() { + return new Predicate() { + @Override + public boolean apply(Reference.Method input) { + return !(input.getFlags().contains(Flag.STATIC) + || input.getFlags().contains(Flag.PRIVATE_OR_HIGHER) + || CONSTRUCTOR_INTERNAL_NAME.equals(input.getName())); + } + }; + } + + private Function toMethod() { + return new Function() { + @Override + public Method apply(Reference.Method method) { + return new Method( + method.getFlags().contains(Flag.ABSTRACT), + reference.getClassName(), + method.getName(), + method.getDescriptor()); + } + }; + } + } + + private static final class ClasspathType implements HelperReferenceWrapper { + private final TypeDescription type; + + private ClasspathType(TypeDescription type) { + this.type = type; + } + + @Override + public boolean isAbstract() { + return type.isAbstract(); + } + + @Override + public boolean hasSuperTypes() { + return hasActualSuperType() || type.getInterfaces().size() > 0; + } + + private boolean hasActualSuperType() { + return type.getSuperClass() != null + && !type.getSuperClass().asErasure().equals(TypeDescription.OBJECT); + } + + // Uses guava iterables to avoid unnecessary collection copying + @Override + public Iterable getSuperTypes() { + Iterable superClass = emptyList(); + if (hasActualSuperType()) { + superClass = + singleton( + (HelperReferenceWrapper) new ClasspathType(type.getSuperClass().asErasure())); + } + + Iterable interfaces = + Iterables.transform(type.getInterfaces().asErasures(), toWrapper()); + + return Iterables.concat(superClass, interfaces); + } + + private static Function toWrapper() { + return new Function() { + @Override + public HelperReferenceWrapper apply(TypeDescription input) { + return new ClasspathType(input); + } + }; + } + + // Uses guava iterables to avoid unnecessary collection copying + @Override + public Iterable getMethods() { + return FluentIterable.from(type.getDeclaredMethods()) + .filter(isOverrideable()) + .transform(toMethod()); + } + + private static Predicate isOverrideable() { + return new Predicate() { + @Override + public boolean apply(InDefinedShape input) { + return !(input.isStatic() || input.isPrivate() || input.isConstructor()); + } + }; + } + + private Function toMethod() { + return new Function() { + @Override + public Method apply(InDefinedShape method) { + return new Method( + method.isAbstract(), + type.getName(), + method.getInternalName(), + method.getDescriptor()); + } + }; + } + } + } +} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleVisitor.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleVisitor.java index 1f724609a99d..351964a7386e 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleVisitor.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/MuzzleVisitor.java @@ -133,28 +133,18 @@ public MethodVisitor visitMethod( } public Reference[] generateReferences() { - // track sources we've generated references from to avoid recursion - Set referenceSources = new HashSet<>(); Map references = new HashMap<>(); - Set adviceClassNames = new HashSet<>(); - - for (String adviceClassName : instrumenter.transformers().values()) { - adviceClassNames.add(adviceClassName); - } + Set adviceClassNames = new HashSet<>(instrumenter.transformers().values()); for (String adviceClass : adviceClassNames) { - if (!referenceSources.contains(adviceClass)) { - referenceSources.add(adviceClass); - for (Map.Entry entry : - ReferenceCreator.createReferencesFrom( - adviceClass, ReferenceMatcher.class.getClassLoader()) - .entrySet()) { - if (references.containsKey(entry.getKey())) { - references.put( - entry.getKey(), references.get(entry.getKey()).merge(entry.getValue())); - } else { - references.put(entry.getKey(), entry.getValue()); - } + for (Map.Entry entry : + ReferenceCreator.createReferencesFrom( + adviceClass, ReferenceMatcher.class.getClassLoader()) + .entrySet()) { + if (references.containsKey(entry.getKey())) { + references.put(entry.getKey(), references.get(entry.getKey()).merge(entry.getValue())); + } else { + references.put(entry.getKey(), entry.getValue()); } } } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java index 7abb2af54433..dfca7f6a9b59 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java @@ -21,6 +21,7 @@ import java.io.StringWriter; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.EnumSet; import java.util.HashSet; import java.util.List; @@ -295,17 +296,17 @@ String getMismatchDetails() { public static class MissingMethod extends Mismatch { private final String className; - private final String method; + private final String methodSignature; - public MissingMethod(Source[] sources, String className, String method) { + public MissingMethod(Source[] sources, String className, String methodSignature) { super(sources); this.className = className; - this.method = method; + this.methodSignature = methodSignature; } @Override String getMismatchDetails() { - return "Missing method " + className + "#" + method; + return "Missing method " + className + "#" + methodSignature; } } } @@ -363,18 +364,18 @@ public boolean matches(int asmFlags) { NON_FINAL { @Override public boolean contradicts(Flag anotherFlag) { - return anotherFlag == FINAL; + return anotherFlag == FINAL || anotherFlag == ABSTRACT; } @Override public boolean matches(int asmFlags) { - return (Opcodes.ACC_FINAL & asmFlags) == 0; + return ((Opcodes.ACC_ABSTRACT | Opcodes.ACC_FINAL) & asmFlags) == 0; } }, FINAL { @Override public boolean contradicts(Flag anotherFlag) { - return anotherFlag == NON_FINAL; + return anotherFlag == NON_FINAL || anotherFlag == ABSTRACT; } @Override @@ -382,6 +383,17 @@ public boolean matches(int asmFlags) { return (Opcodes.ACC_FINAL & asmFlags) != 0; } }, + ABSTRACT { + @Override + public boolean contradicts(Flag anotherFlag) { + return anotherFlag == NON_FINAL || anotherFlag == FINAL; + } + + @Override + public boolean matches(int asmFlags) { + return (Opcodes.ACC_ABSTRACT & asmFlags) != 0; + } + }, STATIC { @Override public boolean contradicts(Flag anotherFlag) { @@ -615,16 +627,30 @@ public Builder withSuperName(String superName) { return this; } + public Builder withInterfaces(Collection interfaceNames) { + interfaces.addAll(interfaceNames); + return this; + } + public Builder withInterface(String interfaceName) { interfaces.add(interfaceName); return this; } + public Builder withSource(String sourceName) { + return withSource(sourceName, 0); + } + public Builder withSource(String sourceName, int line) { sources.add(new Source(sourceName, line)); return this; } + public Builder withFlags(Collection flags) { + this.flags.addAll(flags); + return this; + } + public Builder withFlag(Flag flag) { flags.add(flag); return this; diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreationPredicate.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreationPredicate.java new file mode 100644 index 000000000000..eab3f4990c6b --- /dev/null +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreationPredicate.java @@ -0,0 +1,47 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.opentelemetry.javaagent.tooling.muzzle; + +/** + * Defines a set of packages for which we'll create references. + * + *

For now we're hardcoding this to the instrumentation and javaagent-tooling packages so we only + * create references from the method advice and helper classes. + */ +final class ReferenceCreationPredicate { + private static final String REFERENCE_CREATION_PACKAGE = "io.opentelemetry.instrumentation."; + + private static final String JAVA_AGENT_PACKAGE = "io.opentelemetry.javaagent.tooling."; + + private static final String[] REFERENCE_CREATION_PACKAGE_EXCLUDES = { + "io.opentelemetry.instrumentation.api.", "io.opentelemetry.instrumentation.auto.api." + }; + + static boolean shouldCreateReferenceFor(String className) { + if (!className.startsWith(REFERENCE_CREATION_PACKAGE)) { + return className.startsWith(JAVA_AGENT_PACKAGE); + } + for (String exclude : REFERENCE_CREATION_PACKAGE_EXCLUDES) { + if (className.startsWith(exclude)) { + return false; + } + } + return true; + } + + private ReferenceCreationPredicate() {} +} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java index 5743679da4fc..8b1fb437b29c 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java @@ -16,7 +16,12 @@ package io.opentelemetry.javaagent.tooling.muzzle; +import static com.google.common.base.Preconditions.checkNotNull; +import static io.opentelemetry.javaagent.tooling.muzzle.ReferenceCreationPredicate.shouldCreateReferenceFor; + import io.opentelemetry.javaagent.tooling.Utils; +import io.opentelemetry.javaagent.tooling.muzzle.Reference.Flag; +import io.opentelemetry.javaagent.tooling.muzzle.Reference.Source; import java.io.IOException; import java.io.InputStream; import java.util.ArrayDeque; @@ -43,18 +48,6 @@ // - inner class // - cast opcodes in method bodies public class ReferenceCreator extends ClassVisitor { - /** - * Classes in this namespace will be scanned and used to create references. - * - *

For now we're hardcoding this to the instrumentation package so we only create references - * from the method advice and helper classes. - */ - private static final String REFERENCE_CREATION_PACKAGE = "io.opentelemetry.instrumentation."; - - private static final String[] REFERENCE_CREATION_PACKAGE_EXCLUDES = { - "io.opentelemetry.instrumentation.api.", "io.opentelemetry.instrumentation.auto.api." - }; - /** * Generate all references reachable from a given class. * @@ -71,13 +64,20 @@ public static Map createReferencesFrom( Queue instrumentationQueue = new ArrayDeque<>(); instrumentationQueue.add(entryPointClassName); + boolean isEntryPoint = true; + while (!instrumentationQueue.isEmpty()) { String className = instrumentationQueue.remove(); visitedSources.add(className); - InputStream in = loader.getResourceAsStream(Utils.getResourceName(className)); - try { - ReferenceCreator cv = new ReferenceCreator(); - // only start from method bodies on the first pass + + try (InputStream in = + checkNotNull( + loader.getResourceAsStream(Utils.getResourceName(className)), + "Couldn't find class file %s", + className)) { + + // only start from method bodies for entry point class (skips class/method references) + ReferenceCreator cv = new ReferenceCreator(isEntryPoint); ClassReader reader = new ClassReader(in); reader.accept(cv, ClassReader.SKIP_FRAMES); @@ -85,7 +85,7 @@ public static Map createReferencesFrom( for (Map.Entry entry : instrumentationReferences.entrySet()) { String key = entry.getKey(); // Don't generate references created outside of the instrumentation package. - if (!visitedSources.contains(entry.getKey()) && isInReferenceCreationPackage(key)) { + if (!visitedSources.contains(entry.getKey()) && shouldCreateReferenceFor(key)) { instrumentationQueue.add(key); } if (references.containsKey(key)) { @@ -97,29 +97,13 @@ public static Map createReferencesFrom( } catch (IOException e) { throw new IllegalStateException("Error reading class " + className, e); - } finally { - if (in != null) { - try { - in.close(); - } catch (IOException e) { - throw new IllegalStateException("Error closing class " + className, e); - } - } } - } - return references; - } - private static boolean isInReferenceCreationPackage(String className) { - if (!className.startsWith(REFERENCE_CREATION_PACKAGE)) { - return false; - } - for (String exclude : REFERENCE_CREATION_PACKAGE_EXCLUDES) { - if (className.startsWith(exclude)) { - return false; + if (isEntryPoint) { + isEntryPoint = false; } } - return true; + return references; } /** @@ -191,12 +175,14 @@ private static Type underlyingType(Type type) { return type; } + private final boolean skipClassReferenceGeneration; private final Map references = new HashMap<>(); private String refSourceClassName; private Type refSourceType; - private ReferenceCreator() { + private ReferenceCreator(boolean skipClassReferenceGeneration) { super(Opcodes.ASM7); + this.skipClassReferenceGeneration = skipClassReferenceGeneration; } public Map getReferences() { @@ -224,9 +210,32 @@ public void visit( String[] interfaces) { refSourceClassName = Utils.getClassName(name); refSourceType = Type.getType("L" + name + ";"); - // Additional references we could check - // - supertype of class and visible from this package - // - interfaces of class and visible from this package + + // class references are not generated for advice classes, only for helper classes + if (!skipClassReferenceGeneration) { + String fixedSuperClassName = Utils.getClassName(superName); + + addReference( + new Reference.Builder(fixedSuperClassName).withSource(refSourceClassName).build()); + + List fixedInterfaceNames = new ArrayList<>(interfaces.length); + for (String interfaceName : interfaces) { + String fixedInterfaceName = Utils.getClassName(interfaceName); + fixedInterfaceNames.add(fixedInterfaceName); + + addReference( + new Reference.Builder(fixedInterfaceName).withSource(refSourceClassName).build()); + } + + addReference( + new Reference.Builder(refSourceClassName) + .withSource(refSourceClassName) + .withSuperName(fixedSuperClassName) + .withInterfaces(fixedInterfaceNames) + .withFlag(computeTypeManifestationFlag(access)) + .build()); + } + super.visit(version, access, name, signature, superName, interfaces); } @@ -244,12 +253,68 @@ public FieldVisitor visitField( @Override public MethodVisitor visitMethod( int access, String name, String descriptor, String signature, String[] exceptions) { + + // declared method references are not generated for advice classes, only for helper classes + if (!skipClassReferenceGeneration) { + Type methodType = Type.getMethodType(descriptor); + + Reference.Flag[] methodFlags = + new Reference.Flag[] { + computeVisibilityFlag(access), + computeOwnershipFlag(access), + computeTypeManifestationFlag(access) + }; + + addReference( + new Reference.Builder(refSourceClassName) + .withMethod( + new Source[0], + methodFlags, + name, + methodType.getReturnType(), + methodType.getArgumentTypes()) + .build()); + } + // Additional references we could check // - Classes in signature (return type, params) and visible from this package return new AdviceReferenceMethodVisitor( super.visitMethod(access, name, descriptor, signature, exceptions)); } + /** @see net.bytebuddy.description.modifier.Visibility */ + private static Flag computeVisibilityFlag(int access) { + if (Flag.PUBLIC.matches(access)) { + return Flag.PUBLIC; + } else if (Flag.PROTECTED_OR_HIGHER.matches(access)) { + return Flag.PROTECTED_OR_HIGHER; + } else if (Flag.PACKAGE_OR_HIGHER.matches(access)) { + return Flag.PACKAGE_OR_HIGHER; + } else { + return Flag.PRIVATE_OR_HIGHER; + } + } + + /** @see net.bytebuddy.description.modifier.Ownership */ + private static Flag computeOwnershipFlag(int access) { + if (Flag.STATIC.matches(access)) { + return Flag.STATIC; + } else { + return Flag.NON_STATIC; + } + } + + /** @see net.bytebuddy.description.modifier.TypeManifestation */ + private static Flag computeTypeManifestationFlag(int access) { + if (Flag.ABSTRACT.matches(access)) { + return Flag.ABSTRACT; + } else if (Flag.FINAL.matches(access)) { + return Flag.FINAL; + } else { + return Flag.NON_FINAL; + } + } + private class AdviceReferenceMethodVisitor extends MethodVisitor { private int currentLineNumber = -1; diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java index 50d14effc262..cb57bac814ff 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java @@ -16,18 +16,25 @@ package io.opentelemetry.javaagent.tooling.muzzle; +import static io.opentelemetry.javaagent.tooling.muzzle.ReferenceCreationPredicate.shouldCreateReferenceFor; import static net.bytebuddy.dynamic.loading.ClassLoadingStrategy.BOOTSTRAP_LOADER; +import com.google.common.collect.Sets; import io.opentelemetry.javaagent.bootstrap.WeakCache; import io.opentelemetry.javaagent.tooling.AgentTooling; import io.opentelemetry.javaagent.tooling.Utils; +import io.opentelemetry.javaagent.tooling.muzzle.HelperReferenceWrapper.Factory; +import io.opentelemetry.javaagent.tooling.muzzle.HelperReferenceWrapper.Method; import io.opentelemetry.javaagent.tooling.muzzle.Reference.Mismatch; import io.opentelemetry.javaagent.tooling.muzzle.Reference.Source; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.concurrent.Callable; import net.bytebuddy.description.field.FieldDescription; @@ -39,7 +46,7 @@ public final class ReferenceMatcher { private final WeakCache mismatchCache = AgentTooling.newWeakCache(); - private final Reference[] references; + private final Map references; private final Set helperClassNames; public ReferenceMatcher(Reference... references) { @@ -47,12 +54,15 @@ public ReferenceMatcher(Reference... references) { } public ReferenceMatcher(String[] helperClassNames, Reference[] references) { - this.references = references; + this.references = new HashMap<>(references.length); + for (Reference reference : references) { + this.references.put(reference.getClassName(), reference); + } this.helperClassNames = new HashSet<>(Arrays.asList(helperClassNames)); } - public Reference[] getReferences() { - return references; + public Collection getReferences() { + return references.values(); } /** @@ -77,13 +87,9 @@ public Boolean call() { } private boolean doesMatch(ClassLoader loader) { - for (Reference reference : references) { - // Don't reference-check helper classes. - // They will be injected by the instrumentation's HelperInjector. - if (!helperClassNames.contains(reference.getClassName())) { - if (!checkMatch(reference, loader).isEmpty()) { - return false; - } + for (Reference reference : references.values()) { + if (!checkMatch(reference, loader).isEmpty()) { + return false; } } @@ -103,12 +109,8 @@ public List getMismatchedReferenceSources(ClassLoader loader List mismatches = Collections.emptyList(); - for (Reference reference : references) { - // Don't reference-check helper classes. - // They will be injected by the instrumentation's HelperInjector. - if (!helperClassNames.contains(reference.getClassName())) { - mismatches = lazyAddAll(mismatches, checkMatch(reference, loader)); - } + for (Reference reference : references.values()) { + mismatches = lazyAddAll(mismatches, checkMatch(reference, loader)); } return mismatches; @@ -117,21 +119,29 @@ public List getMismatchedReferenceSources(ClassLoader loader /** * Check a reference against a classloader's classpath. * - * @param loader * @return A list of mismatched sources. A list of size 0 means the reference matches the class. */ - private static List checkMatch(Reference reference, ClassLoader loader) { + private List checkMatch(Reference reference, ClassLoader loader) { TypePool typePool = AgentTooling.poolStrategy() .typePool(AgentTooling.locationStrategy().classFileLocator(loader), loader); try { - TypePool.Resolution resolution = typePool.describe(reference.getClassName()); - if (!resolution.isResolved()) { - return Collections.singletonList( - new Mismatch.MissingClass( - reference.getSources().toArray(new Source[0]), reference.getClassName())); + // helper classes get their own check: whether they implement all abstract methods + if (shouldCreateReferenceFor(reference.getClassName())) { + return checkHelperClassMatch(reference, typePool); + } else if (helperClassNames.contains(reference.getClassName())) { + // skip muzzle check for those helper classes that are not in instrumentation packages; e.g. + // some instrumentations inject guava types as helper classes + return Collections.emptyList(); + } else { + TypePool.Resolution resolution = typePool.describe(reference.getClassName()); + if (!resolution.isResolved()) { + return Collections.singletonList( + new Mismatch.MissingClass( + reference.getSources().toArray(new Source[0]), reference.getClassName())); + } + return checkMatch(reference, resolution.resolve()); } - return checkMatch(reference, resolution.resolve()); } catch (Exception e) { if (e.getMessage().startsWith("Cannot resolve type description for ")) { // bytebuddy throws an illegal state exception with this message if it cannot resolve types @@ -147,7 +157,64 @@ private static List checkMatch(Reference reference, ClassLoa } } - public static List checkMatch( + // for helper classes we make sure that all abstract methods from super classes and interfaces are + // implemented + private List checkHelperClassMatch(Reference helperClass, TypePool typePool) { + List mismatches = Collections.emptyList(); + + HelperReferenceWrapper helperWrapper = new Factory(typePool, references).create(helperClass); + + if (!helperWrapper.hasSuperTypes() || helperWrapper.isAbstract()) { + return mismatches; + } + + // treat the helper type as a bag of methods: collect all methods defined in the helper class, + // all superclasses and interfaces and check if all abstract methods are implemented somewhere + Set abstractMethods = new HashSet<>(); + Set plainMethods = new HashSet<>(); + collectMethodsFromTypeHierarchy(helperWrapper, abstractMethods, plainMethods); + + Set unimplementedMethods = + Sets.difference(abstractMethods, plainMethods); + for (HelperReferenceWrapper.Method unimplementedMethod : unimplementedMethods) { + mismatches = + lazyAdd( + mismatches, + new Reference.Mismatch.MissingMethod( + helperClass.getSources().toArray(new Reference.Source[0]), + helperClass.getClassName(), + unimplementedMethod.toString())); + } + + return mismatches; + } + + private static void collectMethodsFromTypeHierarchy( + HelperReferenceWrapper type, Set abstractMethods, Set plainMethods) { + + for (HelperReferenceWrapper.Method method : type.getMethods()) { + if (shouldIgnore(method)) { + continue; + } + (method.isAbstract() ? abstractMethods : plainMethods).add(method); + } + + for (HelperReferenceWrapper superType : type.getSuperTypes()) { + collectMethodsFromTypeHierarchy(superType, abstractMethods, plainMethods); + } + } + + // ignore Object methods - sometimes defined by interfaces which causes them to be treated as + // abstract + private static boolean shouldIgnore(HelperReferenceWrapper.Method superMethod) { + return "hashCode".equals(superMethod.getName()) && "()I".equals(superMethod.getDescriptor()) + || "equals".equals(superMethod.getName()) + && "(Ljava/lang/Object;)Z".equals(superMethod.getDescriptor()) + || "toString".equals(superMethod.getName()) + && "()Ljava/lang/String;".equals(superMethod.getDescriptor()); + } + + private static List checkMatch( Reference reference, TypeDescription typeOnClasspath) { List mismatches = Collections.emptyList(); @@ -205,8 +272,8 @@ public static List checkMatch( mismatches, new Reference.Mismatch.MissingMethod( methodRef.getSources().toArray(new Reference.Source[0]), - methodRef.getName(), - methodRef.getDescriptor())); + reference.getClassName(), + methodRef.getName() + methodRef.getDescriptor())); } else { for (Reference.Flag flag : methodRef.getFlags()) { if (!flag.matches(methodDescription.getModifiers())) { diff --git a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreationPredicateTest.groovy b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreationPredicateTest.groovy new file mode 100644 index 000000000000..87a5b56d8b03 --- /dev/null +++ b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreationPredicateTest.groovy @@ -0,0 +1,45 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.opentelemetry.javaagent.tooling.muzzle + +import spock.lang.Specification +import spock.lang.Unroll + +class ReferenceCreationPredicateTest extends Specification { + @Unroll + def "should create reference for #desc"() { + expect: + ReferenceCreationPredicate.shouldCreateReferenceFor(className) + + where: + desc | className + "Instrumentation class" | "io.opentelemetry.instrumentation.some_instrumentation.Advice" + "javaagent-tooling class" | "io.opentelemetry.javaagent.tooling.Constants" + } + + @Unroll + def "should not create reference for #desc"() { + expect: + !ReferenceCreationPredicate.shouldCreateReferenceFor(className) + + where: + desc | className + "Java SDK class" | "java.util.ArrayList" + "instrumentation-api class" | "io.opentelemetry.instrumentation.api.InstrumentationVersion" + "auto-api class" | "io.opentelemetry.instrumentation.auto.api.ContextStore" + } +} diff --git a/testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy b/testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy new file mode 100644 index 000000000000..21d4dba464e8 --- /dev/null +++ b/testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy @@ -0,0 +1,135 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package muzzle + +import io.opentelemetry.javaagent.tooling.muzzle.HelperReferenceWrapper +import io.opentelemetry.javaagent.tooling.muzzle.Reference +import net.bytebuddy.jar.asm.Type +import net.bytebuddy.pool.TypePool +import spock.lang.Shared +import spock.lang.Specification + +class HelperReferenceWrapperTest extends Specification { + + @Shared + def baseHelperClass = new Reference.Builder(HelperReferenceWrapperTest.name + '$BaseHelper') + .withSuperName(HelperReferenceWrapperTestClasses.AbstractClasspathType.name) + .withFlag(Reference.Flag.ABSTRACT) + .withMethod(new Reference.Source[0], new Reference.Flag[0], "foo", Type.VOID_TYPE) + .withMethod(new Reference.Source[0], [Reference.Flag.ABSTRACT] as Reference.Flag[], "abstract", Type.INT_TYPE) + .build() + + @Shared + def helperClass = new Reference.Builder(HelperReferenceWrapperTest.name + '$Helper') + .withSuperName(baseHelperClass.className) + .withInterface(HelperReferenceWrapperTestClasses.Interface2.name) + .withMethod(new Reference.Source[0], new Reference.Flag[0], "bar", Type.VOID_TYPE) + .build() + + def "should wrap helper types"() { + given: + def typePool = TypePool.Default.of(HelperReferenceWrapperTest.classLoader) + def references = [ + (helperClass.className) : helperClass, + (baseHelperClass.className): baseHelperClass + ] + + when: + def helperWrapper = new HelperReferenceWrapper.Factory(typePool, references).create(helperClass) + + then: + with(helperWrapper) { helper -> + !helper.abstract + + with(helper.methods.toList()) { + it.size() == 1 + with(it[0]) { + !it.abstract + it.name == "bar" + it.descriptor == "()V" + } + } + + helper.hasSuperTypes() + with(helper.superTypes.toList()) { + it.size() == 2 + with(it[0]) { baseHelper -> + baseHelper.abstract + + with(baseHelper.methods.toList()) { + it.size() == 2 + with(it[0]) { + !it.abstract + it.name == 'foo' + it.descriptor == '()V' + } + with(it[1]) { + it.abstract + it.name == 'abstract' + it.descriptor == '()I' + } + } + + baseHelper.hasSuperTypes() + with(baseHelper.superTypes.toList()) { + it.size() == 1 + with(it[0]) { abstractClasspathType -> + abstractClasspathType.abstract + + abstractClasspathType.methods.empty + + abstractClasspathType.hasSuperTypes() + with(abstractClasspathType.superTypes.toList()) { + it.size() == 1 + with(it[0]) { interface1 -> + interface1.abstract + + with(interface1.methods.toList()) { + it.size() == 1 + with(it[0]) { + it.abstract + it.name == "foo" + it.descriptor == "()V" + } + } + + !interface1.hasSuperTypes() + interface1.superTypes.empty + } + } + } + } + } + with(it[1]) { interface2 -> + interface2.abstract + + with(interface2.methods.toList()) { + it.size() == 1 + with(it[0]) { + it.abstract + it.name == "bar" + it.descriptor == "()V" + } + } + + !interface2.hasSuperTypes() + interface2.superTypes.empty + } + } + } + } +} diff --git a/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy index cd1a008452a6..d3f76e08f36b 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy @@ -16,10 +16,12 @@ package muzzle +import static muzzle.TestClasses.HelperAdvice import static muzzle.TestClasses.LdcAdvice import static muzzle.TestClasses.MethodBodyAdvice import io.opentelemetry.auto.test.AgentTestRunner +import io.opentelemetry.instrumentation.TestHelperClasses import io.opentelemetry.javaagent.tooling.muzzle.Reference import io.opentelemetry.javaagent.tooling.muzzle.ReferenceCreator import spock.lang.Ignore @@ -27,82 +29,145 @@ import spock.lang.Ignore class ReferenceCreatorTest extends AgentTestRunner { def "method body creates references"() { setup: - Map references = ReferenceCreator.createReferencesFrom(MethodBodyAdvice.getName(), this.getClass().getClassLoader()) + def references = ReferenceCreator.createReferencesFrom(MethodBodyAdvice.name, this.class.classLoader) expect: - references.get('muzzle.TestClasses$MethodBodyAdvice$A') != null - references.get('muzzle.TestClasses$MethodBodyAdvice$B') != null - references.get('muzzle.TestClasses$MethodBodyAdvice$SomeInterface') != null - references.get('muzzle.TestClasses$MethodBodyAdvice$SomeImplementation') != null - references.keySet().size() == 4 + references.keySet() == [ + MethodBodyAdvice.A.name, + MethodBodyAdvice.B.name, + MethodBodyAdvice.SomeInterface.name, + MethodBodyAdvice.SomeImplementation.name + ] as Set + + def bRef = references[MethodBodyAdvice.B.name] + def aRef = references[MethodBodyAdvice.A.name] // interface flags - references.get('muzzle.TestClasses$MethodBodyAdvice$B').getFlags().contains(Reference.Flag.NON_INTERFACE) - references.get('muzzle.TestClasses$MethodBodyAdvice$SomeInterface').getFlags().contains(Reference.Flag.INTERFACE) + bRef.flags.contains(Reference.Flag.NON_INTERFACE) + references[MethodBodyAdvice.SomeInterface.name].flags.contains(Reference.Flag.INTERFACE) // class access flags - references.get('muzzle.TestClasses$MethodBodyAdvice$A').getFlags().contains(Reference.Flag.PACKAGE_OR_HIGHER) - references.get('muzzle.TestClasses$MethodBodyAdvice$B').getFlags().contains(Reference.Flag.PACKAGE_OR_HIGHER) + aRef.flags.contains(Reference.Flag.PACKAGE_OR_HIGHER) + bRef.flags.contains(Reference.Flag.PACKAGE_OR_HIGHER) // method refs - Set bMethods = references.get('muzzle.TestClasses$MethodBodyAdvice$B').getMethods() - findMethod(bMethods, "aMethod", "(Ljava/lang/String;)Ljava/lang/String;") != null - findMethod(bMethods, "aMethodWithPrimitives", "(Z)V") != null - findMethod(bMethods, "aStaticMethod", "()V") != null - findMethod(bMethods, "aMethodWithArrays", "([Ljava/lang/String;)[Ljava/lang/Object;") != null - - findMethod(bMethods, "aMethod", "(Ljava/lang/String;)Ljava/lang/String;").getFlags().contains(Reference.Flag.NON_STATIC) - findMethod(bMethods, "aStaticMethod", "()V").getFlags().contains(Reference.Flag.STATIC) + assertMethod bRef, 'aMethod', '(Ljava/lang/String;)Ljava/lang/String;', + Reference.Flag.PROTECTED_OR_HIGHER, + Reference.Flag.NON_STATIC + assertMethod bRef, 'aMethodWithPrimitives', '(Z)V', + Reference.Flag.PROTECTED_OR_HIGHER, + Reference.Flag.NON_STATIC + assertMethod bRef, 'aStaticMethod', '()V', + Reference.Flag.PROTECTED_OR_HIGHER, + Reference.Flag.STATIC + assertMethod bRef, 'aMethodWithArrays', '([Ljava/lang/String;)[Ljava/lang/Object;', + Reference.Flag.PROTECTED_OR_HIGHER, + Reference.Flag.NON_STATIC // field refs - references.get('muzzle.TestClasses$MethodBodyAdvice$B').getFields().isEmpty() - Set aFieldRefs = references.get('muzzle.TestClasses$MethodBodyAdvice$A').getFields() - findField(aFieldRefs, "b").getFlags().contains(Reference.Flag.PACKAGE_OR_HIGHER) - findField(aFieldRefs, "b").getFlags().contains(Reference.Flag.NON_STATIC) - findField(aFieldRefs, "staticB").getFlags().contains(Reference.Flag.PACKAGE_OR_HIGHER) - findField(aFieldRefs, "staticB").getFlags().contains(Reference.Flag.STATIC) - aFieldRefs.size() == 2 + bRef.fields.isEmpty() + aRef.fields.size() == 2 + assertField aRef, 'b', Reference.Flag.PACKAGE_OR_HIGHER, Reference.Flag.NON_STATIC + assertField aRef, 'staticB', Reference.Flag.PACKAGE_OR_HIGHER, Reference.Flag.STATIC } def "protected ref test"() { setup: - Map references = ReferenceCreator.createReferencesFrom(MethodBodyAdvice.B2.getName(), this.getClass().getClassLoader()) + def references = ReferenceCreator.createReferencesFrom(MethodBodyAdvice.B2.name, this.class.classLoader) expect: - Set bMethods = references.get('muzzle.TestClasses$MethodBodyAdvice$B').getMethods() - findMethod(bMethods, "protectedMethod", "()V") != null - findMethod(bMethods, "protectedMethod", "()V").getFlags().contains(Reference.Flag.PROTECTED_OR_HIGHER) + assertMethod references[MethodBodyAdvice.B.name], 'protectedMethod', '()V', + Reference.Flag.PROTECTED_OR_HIGHER, + Reference.Flag.NON_STATIC } def "ldc creates references"() { setup: - Map references = ReferenceCreator.createReferencesFrom(LdcAdvice.getName(), this.getClass().getClassLoader()) + def references = ReferenceCreator.createReferencesFrom(LdcAdvice.name, this.class.classLoader) expect: - references.get('muzzle.TestClasses$MethodBodyAdvice$A') != null + references[MethodBodyAdvice.A.name] != null } def "instanceof creates references"() { setup: - Map references = ReferenceCreator.createReferencesFrom(TestClasses.InstanceofAdvice.getName(), this.getClass().getClassLoader()) + def references = ReferenceCreator.createReferencesFrom(TestClasses.InstanceofAdvice.name, this.class.classLoader) expect: - references.get('muzzle.TestClasses$MethodBodyAdvice$A') != null + references[MethodBodyAdvice.A.name] != null } // TODO: remove ignore when we drop java 7 support. @Ignore def "invokedynamic creates references"() { setup: - Map references = ReferenceCreator.createReferencesFrom(TestClasses.InDyAdvice.getName(), this.getClass().getClassLoader()) + def references = ReferenceCreator.createReferencesFrom(TestClasses.InDyAdvice.name, this.class.classLoader) expect: - references.get('muzzle.TestClasses$MethodBodyAdvice$SomeImplementation') != null - references.get('muzzle.TestClasses$MethodBodyAdvice$B') != null + references['muzzle.TestClasses$MethodBodyAdvice$SomeImplementation'] != null + references['muzzle.TestClasses$MethodBodyAdvice$B'] != null + } + + def "should create references for helper classes"() { + when: + def references = ReferenceCreator.createReferencesFrom(HelperAdvice.name, this.class.classLoader) + + then: + references.keySet() == [ + TestHelperClasses.Helper.name, + TestHelperClasses.HelperSuperClass.name, + TestHelperClasses.HelperInterface.name + ] as Set + + with(references[TestHelperClasses.HelperSuperClass.name]) { helperSuperClass -> + helperSuperClass.flags.contains(Reference.Flag.ABSTRACT) + assertHelperSuperClassMethod(helperSuperClass, true) + assertMethod helperSuperClass, 'finalMethod', '()Ljava/lang/String;', + Reference.Flag.PUBLIC, + Reference.Flag.NON_STATIC, + Reference.Flag.FINAL + assertMethod helperSuperClass, 'bar', '()I', + Reference.Flag.PACKAGE_OR_HIGHER, + Reference.Flag.STATIC, + Reference.Flag.NON_FINAL + } + + with(references[TestHelperClasses.HelperInterface.name]) { helperInterface -> + helperInterface.flags.contains(Reference.Flag.ABSTRACT) + assertHelperInterfaceMethod helperInterface, true + } + + with(references[TestHelperClasses.Helper.name]) { helperClass -> + assertHelperSuperClassMethod helperClass, false + assertHelperInterfaceMethod helperClass, false + assertMethod helperClass, 'getStr', '()Ljava/lang/String;', + Reference.Flag.PRIVATE_OR_HIGHER, + Reference.Flag.NON_STATIC, + Reference.Flag.NON_FINAL + } } - private static Reference.Method findMethod(Set methods, String methodName, String methodDesc) { - for (Reference.Method method : methods) { + private static assertHelperSuperClassMethod(Reference reference, boolean isAbstract) { + assertMethod reference, 'abstractMethod', '()I', + Reference.Flag.PROTECTED_OR_HIGHER, + Reference.Flag.NON_STATIC, + isAbstract ? Reference.Flag.ABSTRACT : Reference.Flag.NON_FINAL + } + + private static assertHelperInterfaceMethod(Reference reference, boolean isAbstract) { + assertMethod reference, 'foo', '()V', + Reference.Flag.PUBLIC, + Reference.Flag.NON_STATIC, + isAbstract ? Reference.Flag.ABSTRACT : Reference.Flag.NON_FINAL + } + + private static assertMethod(Reference reference, String methodName, String methodDesc, Reference.Flag... flags) { + def method = findMethod reference, methodName, methodDesc + method != null && (method.flags == flags as Set) + } + + private static findMethod(Reference reference, String methodName, String methodDesc) { + for (def method : reference.methods) { if (method == new Reference.Method(methodName, methodDesc)) { return method } @@ -110,9 +175,14 @@ class ReferenceCreatorTest extends AgentTestRunner { return null } - private static Reference.Field findField(Set fields, String fieldName) { - for (Reference.Field field : fields) { - if (field.getName().equals(fieldName)) { + private static assertField(Reference reference, String fieldName, Reference.Flag... flags) { + def field = findField reference, fieldName + field != null && (field.flags == flags as Set) + } + + private static Reference.Field findField(Reference reference, String fieldName) { + for (def field : reference.fields) { + if (field.name == fieldName) { return field } } diff --git a/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy index 978647bd7723..fbd06f6f6962 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy @@ -16,6 +16,7 @@ package muzzle +import static io.opentelemetry.javaagent.tooling.muzzle.Reference.Flag.ABSTRACT import static io.opentelemetry.javaagent.tooling.muzzle.Reference.Flag.INTERFACE import static io.opentelemetry.javaagent.tooling.muzzle.Reference.Flag.NON_INTERFACE import static io.opentelemetry.javaagent.tooling.muzzle.Reference.Flag.NON_STATIC @@ -30,6 +31,7 @@ import static muzzle.TestClasses.MethodBodyAdvice import io.opentelemetry.auto.test.AgentTestRunner import io.opentelemetry.auto.test.utils.ClasspathUtils +import io.opentelemetry.instrumentation.TestHelperClasses import io.opentelemetry.javaagent.tooling.muzzle.Reference import io.opentelemetry.javaagent.tooling.muzzle.Reference.Source import io.opentelemetry.javaagent.tooling.muzzle.ReferenceCreator @@ -54,12 +56,14 @@ class ReferenceMatcherTest extends AgentTestRunner { def "match safe classpaths"() { setup: - Reference[] refs = ReferenceCreator.createReferencesFrom(MethodBodyAdvice.getName(), this.getClass().getClassLoader()).values().toArray(new Reference[0]) - ReferenceMatcher refMatcher = new ReferenceMatcher(refs) + Reference[] refs = ReferenceCreator.createReferencesFrom(MethodBodyAdvice.name, this.class.classLoader) + .values() + .toArray(new Reference[0]) + def refMatcher = new ReferenceMatcher(refs) expect: - getMismatchClassSet(refMatcher.getMismatchedReferenceSources(safeClasspath)) == new HashSet<>() - getMismatchClassSet(refMatcher.getMismatchedReferenceSources(unsafeClasspath)) == new HashSet<>([MissingClass]) + getMismatchClassSet(refMatcher.getMismatchedReferenceSources(safeClasspath)).empty + getMismatchClassSet(refMatcher.getMismatchedReferenceSources(unsafeClasspath)) == [MissingClass] as Set } def "matching does not hold a strong reference to classloaders"() { @@ -83,19 +87,21 @@ class ReferenceMatcherTest extends AgentTestRunner { def "muzzle type pool caches"() { setup: - ClassLoader cl = new CountingClassLoader( + def cl = new CountingClassLoader( [ClasspathUtils.createJarWithClasses(MethodBodyAdvice.A, MethodBodyAdvice.B, MethodBodyAdvice.SomeInterface, MethodBodyAdvice.SomeImplementation)] as URL[], (ClassLoader) null) - Reference[] refs = ReferenceCreator.createReferencesFrom(MethodBodyAdvice.getName(), this.getClass().getClassLoader()).values().toArray(new Reference[0]) - ReferenceMatcher refMatcher1 = new ReferenceMatcher(refs) - ReferenceMatcher refMatcher2 = new ReferenceMatcher(refs) - assert getMismatchClassSet(refMatcher1.getMismatchedReferenceSources(cl)) == new HashSet<>() + Reference[] refs = ReferenceCreator.createReferencesFrom(MethodBodyAdvice.name, this.class.classLoader) + .values() + .toArray(new Reference[0]) + def refMatcher1 = new ReferenceMatcher(refs) + def refMatcher2 = new ReferenceMatcher(refs) + assert getMismatchClassSet(refMatcher1.getMismatchedReferenceSources(cl)).empty int countAfterFirstMatch = cl.count // the second matcher should be able to used cached type descriptions from the first - assert getMismatchClassSet(refMatcher2.getMismatchedReferenceSources(cl)) == new HashSet<>() + assert getMismatchClassSet(refMatcher2.getMismatchedReferenceSources(cl)).empty expect: cl.count == countAfterFirstMatch @@ -103,30 +109,35 @@ class ReferenceMatcherTest extends AgentTestRunner { def "matching ref #referenceName #referenceFlags against #classToCheck produces #expectedMismatches"() { setup: - Reference.Builder builder = new Reference.Builder(referenceName) - for (Reference.Flag refFlag : referenceFlags) { - builder = builder.withFlag(refFlag) - } - Reference ref = builder.build() + def ref = new Reference.Builder(referenceName) + .withFlags(referenceFlags) + .build() - expect: - getMismatchClassSet(ReferenceMatcher.checkMatch(ref, this.getClass().getClassLoader())) == new HashSet(expectedMismatches) + when: + def mismatches = new ReferenceMatcher(ref).getMismatchedReferenceSources(this.class.classLoader) + + then: + getMismatchClassSet(mismatches) == expectedMismatches as Set where: - referenceName | referenceFlags | classToCheck | expectedMismatches - MethodBodyAdvice.B.getName() | [NON_INTERFACE] | MethodBodyAdvice.B | [] - MethodBodyAdvice.B.getName() | [INTERFACE] | MethodBodyAdvice.B | [MissingFlag] + referenceName | referenceFlags | classToCheck | expectedMismatches + MethodBodyAdvice.B.name | [NON_INTERFACE] | MethodBodyAdvice.B | [] + MethodBodyAdvice.B.name | [INTERFACE] | MethodBodyAdvice.B | [MissingFlag] } def "method match #methodTestDesc"() { setup: - Type methodType = Type.getMethodType(methodDesc) - Reference reference = new Reference.Builder(classToCheck.getName()) - .withMethod(new Source[0], methodFlags as Reference.Flag[], methodName, methodType.getReturnType(), methodType.getArgumentTypes()) + def methodType = Type.getMethodType(methodDesc) + def reference = new Reference.Builder(classToCheck.name) + .withMethod(new Source[0], methodFlags as Reference.Flag[], methodName, methodType.returnType, methodType.argumentTypes) .build() - expect: - getMismatchClassSet(ReferenceMatcher.checkMatch(reference, this.getClass().getClassLoader())) == new HashSet(expectedMismatches) + when: + def mismatches = new ReferenceMatcher(reference) + .getMismatchedReferenceSources(this.class.classLoader) + + then: + getMismatchClassSet(mismatches) == expectedMismatches as Set where: methodName | methodDesc | methodFlags | classToCheck | expectedMismatches | methodTestDesc @@ -141,12 +152,16 @@ class ReferenceMatcherTest extends AgentTestRunner { def "field match #fieldTestDesc"() { setup: - Reference reference = new Reference.Builder(classToCheck.getName()) + def reference = new Reference.Builder(classToCheck.name) .withField(new Source[0], fieldFlags as Reference.Flag[], fieldName, Type.getType(fieldType)) .build() - expect: - getMismatchClassSet(ReferenceMatcher.checkMatch(reference, this.getClass().getClassLoader())) == new HashSet(expectedMismatches) + when: + def mismatches = new ReferenceMatcher(reference) + .getMismatchedReferenceSources(this.class.classLoader) + + then: + getMismatchClassSet(mismatches) == expectedMismatches as Set where: fieldName | fieldType | fieldFlags | classToCheck | expectedMismatches | fieldTestDesc @@ -158,10 +173,91 @@ class ReferenceMatcherTest extends AgentTestRunner { "staticB" | Type.getType(MethodBodyAdvice.B).getDescriptor() | [STATIC, PROTECTED_OR_HIGHER] | MethodBodyAdvice.A | [] | "match static field" } + def "should ignore helper classes from third-party packages"() { + given: + def emptyClassLoader = new URLClassLoader(new URL[0], (ClassLoader) null) + def reference = new Reference.Builder("com.google.common.base.Strings") + .build() + + when: + def mismatches = new ReferenceMatcher([reference.className] as String[], [reference] as Reference[]) + .getMismatchedReferenceSources(emptyClassLoader) + + then: + mismatches.empty + } + + def "should not check abstract helper classes"() { + given: + def reference = new Reference.Builder("io.opentelemetry.instrumentation.Helper") + .withSuperName(TestHelperClasses.HelperSuperClass.name) + .withFlag(ABSTRACT) + .withMethod(new Source[0], [ABSTRACT] as Reference.Flag[], "unimplemented", Type.VOID_TYPE) + .build() + + when: + def mismatches = new ReferenceMatcher([reference.className] as String[], [reference] as Reference[]) + .getMismatchedReferenceSources(this.class.classLoader) + + then: + mismatches.empty + } + + def "should not check helper classes with no supertypes"() { + given: + def reference = new Reference.Builder("io.opentelemetry.instrumentation.Helper") + .withSuperName(Object.name) + .withMethod(new Source[0], [] as Reference.Flag[], "someMethod", Type.VOID_TYPE) + .build() + + when: + def mismatches = new ReferenceMatcher([reference.className] as String[], [reference] as Reference[]) + .getMismatchedReferenceSources(this.class.classLoader) + + then: + mismatches.empty + } + + def "should fail helper classes that does not implement all abstract methods"() { + given: + def reference = new Reference.Builder("io.opentelemetry.instrumentation.Helper") + .withSuperName(TestHelperClasses.HelperSuperClass.name) + .withMethod(new Source[0], [] as Reference.Flag[], "someMethod", Type.VOID_TYPE) + .build() + + when: + def mismatches = new ReferenceMatcher([reference.className] as String[], [reference] as Reference[]) + .getMismatchedReferenceSources(this.class.classLoader) + + then: + getMismatchClassSet(mismatches) == [MissingMethod] as Set + } + + def "should check whether interface methods are implemented in the super class"() { + given: + def baseHelper = new Reference.Builder("io.opentelemetry.instrumentation.BaseHelper") + .withInterface(TestHelperClasses.HelperInterface.name) + .withMethod(new Source[0], [] as Reference.Flag[], "foo", Type.VOID_TYPE) + .build() + // abstract HelperInterface#foo() is implemented by BaseHelper + def helper = new Reference.Builder("io.opentelemetry.instrumentation.Helper") + .withSuperName(baseHelper.className) + .withInterface(TestHelperClasses.AnotherHelperInterface.name) + .withMethod(new Source[0], [] as Reference.Flag[], "bar", Type.VOID_TYPE) + .build() + + when: + def mismatches = new ReferenceMatcher([helper.className, baseHelper] as String[], [helper, baseHelper] as Reference[]) + .getMismatchedReferenceSources(this.class.classLoader) + + then: + mismatches.empty + } + private static Set getMismatchClassSet(List mismatches) { Set mismatchClasses = new HashSet<>(mismatches.size()) for (Reference.Mismatch mismatch : mismatches) { - mismatchClasses.add(mismatch.getClass()) + mismatchClasses.add(mismatch.class) } return mismatchClasses } diff --git a/testing-common/src/test/java/io/opentelemetry/instrumentation/TestHelperClasses.java b/testing-common/src/test/java/io/opentelemetry/instrumentation/TestHelperClasses.java new file mode 100644 index 000000000000..718a8fe60378 --- /dev/null +++ b/testing-common/src/test/java/io/opentelemetry/instrumentation/TestHelperClasses.java @@ -0,0 +1,64 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.opentelemetry.instrumentation; + +import java.util.ArrayList; +import java.util.List; + +public class TestHelperClasses { + public static class Helper extends HelperSuperClass implements HelperInterface { + + @Override + public void foo() { + List list = new ArrayList<>(); + list.add(getStr()); + } + + @Override + protected int abstractMethod() { + return 54321; + } + + private String getStr() { + return "abc"; + } + } + + public interface HelperInterface { + void foo(); + } + + public interface AnotherHelperInterface extends HelperInterface { + void bar(); + + int hashCode(); + + boolean equals(Object other); + } + + public abstract static class HelperSuperClass { + protected abstract int abstractMethod(); + + public final String finalMethod() { + return "42"; + } + + static int bar() { + return 12345; + } + } +} diff --git a/testing-common/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java b/testing-common/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java new file mode 100644 index 000000000000..5aac74e4f628 --- /dev/null +++ b/testing-common/src/test/java/muzzle/HelperReferenceWrapperTestClasses.java @@ -0,0 +1,33 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package muzzle; + +public class HelperReferenceWrapperTestClasses { + interface Interface1 { + void foo(); + } + + interface Interface2 { + void bar(); + } + + abstract static class AbstractClasspathType implements Interface1 { + static void staticMethodsAreIgnored() {} + + private void privateMethodsToo() {} + } +} diff --git a/testing-common/src/test/java/muzzle/TestClasses.java b/testing-common/src/test/java/muzzle/TestClasses.java index 2ea8b5984773..6e3c750624c5 100644 --- a/testing-common/src/test/java/muzzle/TestClasses.java +++ b/testing-common/src/test/java/muzzle/TestClasses.java @@ -16,6 +16,7 @@ package muzzle; +import io.opentelemetry.instrumentation.TestHelperClasses.Helper; import net.bytebuddy.asm.Advice; public class TestClasses { @@ -105,4 +106,10 @@ public static class InDyAdvice { // return a::someMethod; // } } + + public static class HelperAdvice { + public static void adviceMethod() { + Helper h = new Helper(); + } + } } From ff1833ce1d1a7557b613e1e24aa20d4bdd665606 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Mon, 21 Sep 2020 16:44:56 +0200 Subject: [PATCH 2/6] Muzzle should fail on unimplemented abstract methods --- .../io/opentelemetry/javaagent/tooling/muzzle/Reference.java | 5 +++++ .../javaagent/tooling/muzzle/ReferenceMatcher.java | 5 ++++- .../io/opentelemetry/instrumentation/TestHelperClasses.java | 4 ++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java index dfca7f6a9b59..576bd3bc633b 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java @@ -447,6 +447,11 @@ public boolean supersedes(Flag anotherFlag) { return false; } + /** + * Predicate method that determines whether this flag is present in the passed bitmask. + * + * @see Opcodes + */ public abstract boolean matches(int asmFlags); } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java index cb57bac814ff..c8a019fb18ad 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java @@ -211,7 +211,10 @@ private static boolean shouldIgnore(HelperReferenceWrapper.Method superMethod) { || "equals".equals(superMethod.getName()) && "(Ljava/lang/Object;)Z".equals(superMethod.getDescriptor()) || "toString".equals(superMethod.getName()) - && "()Ljava/lang/String;".equals(superMethod.getDescriptor()); + && "()Ljava/lang/String;".equals(superMethod.getDescriptor()) + || "clone".equals(superMethod.getName()) + && "()Ljava/lang/Object;".equals(superMethod.getDescriptor()) + || "finalize".equals(superMethod.getName()) && "()V".equals(superMethod.getDescriptor()); } private static List checkMatch( diff --git a/testing-common/src/test/java/io/opentelemetry/instrumentation/TestHelperClasses.java b/testing-common/src/test/java/io/opentelemetry/instrumentation/TestHelperClasses.java index 718a8fe60378..e8baad5c7a1c 100644 --- a/testing-common/src/test/java/io/opentelemetry/instrumentation/TestHelperClasses.java +++ b/testing-common/src/test/java/io/opentelemetry/instrumentation/TestHelperClasses.java @@ -48,6 +48,10 @@ public interface AnotherHelperInterface extends HelperInterface { int hashCode(); boolean equals(Object other); + + Object clone(); + + void finalize(); } public abstract static class HelperSuperClass { From 1c95fdfb040b140e8ebc70313694c5305074516b Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Mon, 21 Sep 2020 17:44:29 +0200 Subject: [PATCH 3/6] Muzzle should fail on unimplemented abstract methods --- .../muzzle/HelperReferenceWrapper.java | 2 +- .../javaagent/tooling/muzzle/Reference.java | 98 ++++++------------- .../tooling/muzzle/ReferenceCreator.java | 10 +- .../groovy/muzzle/ReferenceCreatorTest.groovy | 6 +- 4 files changed, 40 insertions(+), 76 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java index f96825652fef..deb162b0b94c 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java @@ -177,7 +177,7 @@ private Predicate isOverrideable() { @Override public boolean apply(Reference.Method input) { return !(input.getFlags().contains(Flag.STATIC) - || input.getFlags().contains(Flag.PRIVATE_OR_HIGHER) + || input.getFlags().contains(Flag.PRIVATE) || CONSTRUCTOR_INTERNAL_NAME.equals(input.getName())); } }; diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java index 576bd3bc633b..5dcf7d0097e5 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java @@ -313,45 +313,46 @@ String getMismatchDetails() { /** Expected flag (or lack of flag) on a class, method, or field reference. */ public enum Flag { + // The following constants represent the exact visibility of a referenced class/method PUBLIC { @Override - public boolean supersedes(Flag anotherFlag) { - switch (anotherFlag) { - case PRIVATE_OR_HIGHER: - case PROTECTED_OR_HIGHER: - case PACKAGE_OR_HIGHER: - return true; - default: - return false; - } + public boolean matches(int asmFlags) { + return (Opcodes.ACC_PUBLIC & asmFlags) != 0; } - + }, + PROTECTED { @Override public boolean matches(int asmFlags) { - return (Opcodes.ACC_PUBLIC & asmFlags) != 0; + return (Opcodes.ACC_PROTECTED & asmFlags) != 0; } }, - PACKAGE_OR_HIGHER { + PACKAGE { @Override - public boolean supersedes(Flag anotherFlag) { - return anotherFlag == PRIVATE_OR_HIGHER; + public boolean matches(int asmFlags) { + return !(PUBLIC.matches(asmFlags) + || PROTECTED.matches(asmFlags) + || PRIVATE.matches(asmFlags)); } - + }, + PRIVATE { @Override public boolean matches(int asmFlags) { - return (Opcodes.ACC_PUBLIC & asmFlags) != 0 - || ((Opcodes.ACC_PRIVATE & asmFlags) == 0 && (Opcodes.ACC_PROTECTED & asmFlags) == 0); + return (Opcodes.ACC_PRIVATE & asmFlags) != 0; } }, + + // The following constants represent a minimum access level required by a method call or field + // access PROTECTED_OR_HIGHER { @Override - public boolean supersedes(Flag anotherFlag) { - return anotherFlag == PRIVATE_OR_HIGHER; + public boolean matches(int asmFlags) { + return PUBLIC.matches(asmFlags) || PROTECTED.matches(asmFlags); } - + }, + PACKAGE_OR_HIGHER { @Override public boolean matches(int asmFlags) { - return PUBLIC.matches(asmFlags) || (Opcodes.ACC_PROTECTED & asmFlags) != 0; + return PUBLIC.matches(asmFlags) || PROTECTED.matches(asmFlags) || PACKAGE.matches(asmFlags); } }, PRIVATE_OR_HIGHER { @@ -361,92 +362,55 @@ public boolean matches(int asmFlags) { return true; } }, - NON_FINAL { - @Override - public boolean contradicts(Flag anotherFlag) { - return anotherFlag == FINAL || anotherFlag == ABSTRACT; - } + // The following constants describe whether classes and methods are abstract or final + FINAL { @Override public boolean matches(int asmFlags) { - return ((Opcodes.ACC_ABSTRACT | Opcodes.ACC_FINAL) & asmFlags) == 0; + return (Opcodes.ACC_FINAL & asmFlags) != 0; } }, - FINAL { - @Override - public boolean contradicts(Flag anotherFlag) { - return anotherFlag == NON_FINAL || anotherFlag == ABSTRACT; - } - + NON_FINAL { @Override public boolean matches(int asmFlags) { - return (Opcodes.ACC_FINAL & asmFlags) != 0; + return ((Opcodes.ACC_ABSTRACT | Opcodes.ACC_FINAL) & asmFlags) == 0; } }, ABSTRACT { - @Override - public boolean contradicts(Flag anotherFlag) { - return anotherFlag == NON_FINAL || anotherFlag == FINAL; - } - @Override public boolean matches(int asmFlags) { return (Opcodes.ACC_ABSTRACT & asmFlags) != 0; } }, - STATIC { - @Override - public boolean contradicts(Flag anotherFlag) { - return anotherFlag == NON_STATIC; - } + // The following constants describe whether a method/field is static or not + STATIC { @Override public boolean matches(int asmFlags) { return (Opcodes.ACC_STATIC & asmFlags) != 0; } }, NON_STATIC { - @Override - public boolean contradicts(Flag anotherFlag) { - return anotherFlag == STATIC; - } - @Override public boolean matches(int asmFlags) { return (Opcodes.ACC_STATIC & asmFlags) == 0; } }, - INTERFACE { - @Override - public boolean contradicts(Flag anotherFlag) { - return anotherFlag == NON_INTERFACE; - } + // The following constants describe whether a class is an interface + INTERFACE { @Override public boolean matches(int asmFlags) { return (Opcodes.ACC_INTERFACE & asmFlags) != 0; } }, NON_INTERFACE { - @Override - public boolean contradicts(Flag anotherFlag) { - return anotherFlag == INTERFACE; - } - @Override public boolean matches(int asmFlags) { return (Opcodes.ACC_INTERFACE & asmFlags) == 0; } }; - public boolean contradicts(Flag anotherFlag) { - return false; - } - - public boolean supersedes(Flag anotherFlag) { - return false; - } - /** * Predicate method that determines whether this flag is present in the passed bitmask. * diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java index 8b1fb437b29c..53a7e44e1eb2 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java @@ -286,12 +286,12 @@ public MethodVisitor visitMethod( private static Flag computeVisibilityFlag(int access) { if (Flag.PUBLIC.matches(access)) { return Flag.PUBLIC; - } else if (Flag.PROTECTED_OR_HIGHER.matches(access)) { - return Flag.PROTECTED_OR_HIGHER; - } else if (Flag.PACKAGE_OR_HIGHER.matches(access)) { - return Flag.PACKAGE_OR_HIGHER; + } else if (Flag.PROTECTED.matches(access)) { + return Flag.PROTECTED; + } else if (Flag.PACKAGE.matches(access)) { + return Flag.PACKAGE; } else { - return Flag.PRIVATE_OR_HIGHER; + return Flag.PRIVATE; } } diff --git a/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy index d3f76e08f36b..d339d43e23fc 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy @@ -127,7 +127,7 @@ class ReferenceCreatorTest extends AgentTestRunner { Reference.Flag.NON_STATIC, Reference.Flag.FINAL assertMethod helperSuperClass, 'bar', '()I', - Reference.Flag.PACKAGE_OR_HIGHER, + Reference.Flag.PACKAGE, Reference.Flag.STATIC, Reference.Flag.NON_FINAL } @@ -141,7 +141,7 @@ class ReferenceCreatorTest extends AgentTestRunner { assertHelperSuperClassMethod helperClass, false assertHelperInterfaceMethod helperClass, false assertMethod helperClass, 'getStr', '()Ljava/lang/String;', - Reference.Flag.PRIVATE_OR_HIGHER, + Reference.Flag.PRIVATE, Reference.Flag.NON_STATIC, Reference.Flag.NON_FINAL } @@ -149,7 +149,7 @@ class ReferenceCreatorTest extends AgentTestRunner { private static assertHelperSuperClassMethod(Reference reference, boolean isAbstract) { assertMethod reference, 'abstractMethod', '()I', - Reference.Flag.PROTECTED_OR_HIGHER, + Reference.Flag.PROTECTED, Reference.Flag.NON_STATIC, isAbstract ? Reference.Flag.ABSTRACT : Reference.Flag.NON_FINAL } From dca390491bf5f52be3f3670b88622d95f0b48d78 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Mon, 21 Sep 2020 18:32:22 +0200 Subject: [PATCH 4/6] Muzzle should fail on unimplemented abstract methods --- .../src/test/groovy/muzzle/ReferenceCreatorTest.groovy | 2 ++ 1 file changed, 2 insertions(+) diff --git a/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy index d339d43e23fc..e9f5c34657ed 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy @@ -141,7 +141,9 @@ class ReferenceCreatorTest extends AgentTestRunner { assertHelperSuperClassMethod helperClass, false assertHelperInterfaceMethod helperClass, false assertMethod helperClass, 'getStr', '()Ljava/lang/String;', + // this method has both PRIVATE flags: first because it's defined in the helper class, second because it's called in foo() Reference.Flag.PRIVATE, + Reference.Flag.PRIVATE_OR_HIGHER, Reference.Flag.NON_STATIC, Reference.Flag.NON_FINAL } From 7df71f3e42dfe935c2dd98161f6ef52e7fc10650 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Tue, 22 Sep 2020 10:42:07 +0200 Subject: [PATCH 5/6] Update javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java Co-authored-by: Trask Stalnaker --- .../io/opentelemetry/javaagent/tooling/muzzle/Reference.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java index 5dcf7d0097e5..c4eed463d321 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java @@ -352,7 +352,7 @@ public boolean matches(int asmFlags) { PACKAGE_OR_HIGHER { @Override public boolean matches(int asmFlags) { - return PUBLIC.matches(asmFlags) || PROTECTED.matches(asmFlags) || PACKAGE.matches(asmFlags); + return !PRIVATE.matches(asmFlags); } }, PRIVATE_OR_HIGHER { From 91597cdd95baf922776dce1774887c9232399147 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Tue, 22 Sep 2020 12:00:51 +0200 Subject: [PATCH 6/6] Muzzle should fail on unimplemented abstract methods --- .../muzzle/HelperReferenceWrapper.java | 21 +++++------- .../javaagent/tooling/muzzle/Reference.java | 11 +++--- .../tooling/muzzle/ReferenceCreator.java | 34 +++++++++++-------- .../tooling/muzzle/ReferenceMatcher.java | 24 +++---------- .../muzzle/HelperReferenceWrapperTest.groovy | 7 ++-- .../groovy/muzzle/ReferenceCreatorTest.groovy | 11 +----- .../groovy/muzzle/ReferenceMatcherTest.groovy | 18 ++++++++++ 7 files changed, 64 insertions(+), 62 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java index deb162b0b94c..c98f38c271ea 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/HelperReferenceWrapper.java @@ -68,6 +68,10 @@ public boolean isAbstract() { return isAbstract; } + public String getDeclaringClass() { + return declaringClass; + } + public String getName() { return name; } @@ -88,11 +92,6 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hashCode(name, descriptor); } - - @Override - public String toString() { - return declaringClass + "#" + name + descriptor; - } } class Factory { @@ -109,13 +108,13 @@ public HelperReferenceWrapper create(Reference reference) { } private HelperReferenceWrapper create(String className) { - if (helperReferences.containsKey(className)) { - return new ReferenceType(helperReferences.get(className)); - } Resolution resolution = classpathPool.describe(className); if (resolution.isResolved()) { return new ClasspathType(resolution.resolve()); } + if (helperReferences.containsKey(className)) { + return new ReferenceType(helperReferences.get(className)); + } throw new IllegalStateException("Missing class " + className); } @@ -151,8 +150,7 @@ public Iterable getSuperTypes() { } private boolean hasActualSuperType() { - return reference.getSuperName() != null - && !reference.getSuperName().equals(Object.class.getName()); + return reference.getSuperName() != null; } private Function toWrapper() { @@ -215,8 +213,7 @@ public boolean hasSuperTypes() { } private boolean hasActualSuperType() { - return type.getSuperClass() != null - && !type.getSuperClass().asErasure().equals(TypeDescription.OBJECT); + return type.getSuperClass() != null; } // Uses guava iterables to avoid unnecessary collection copying diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java index c4eed463d321..9601255947a7 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java @@ -296,17 +296,20 @@ String getMismatchDetails() { public static class MissingMethod extends Mismatch { private final String className; - private final String methodSignature; + private final String methodName; + private final String methodDescriptor; - public MissingMethod(Source[] sources, String className, String methodSignature) { + public MissingMethod( + Source[] sources, String className, String methodName, String methodDescriptor) { super(sources); this.className = className; - this.methodSignature = methodSignature; + this.methodName = methodName; + this.methodDescriptor = methodDescriptor; } @Override String getMismatchDetails() { - return "Missing method " + className + "#" + methodSignature; + return "Missing method " + className + "#" + methodName + methodDescriptor; } } } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java index 53a7e44e1eb2..a088aec7c26a 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java @@ -32,6 +32,7 @@ import java.util.Map; import java.util.Queue; import java.util.Set; +import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.jar.asm.ClassReader; import net.bytebuddy.jar.asm.ClassVisitor; import net.bytebuddy.jar.asm.FieldVisitor; @@ -258,22 +259,25 @@ public MethodVisitor visitMethod( if (!skipClassReferenceGeneration) { Type methodType = Type.getMethodType(descriptor); - Reference.Flag[] methodFlags = - new Reference.Flag[] { - computeVisibilityFlag(access), - computeOwnershipFlag(access), - computeTypeManifestationFlag(access) - }; + Flag visibilityFlag = computeVisibilityFlag(access); + Flag ownershipFlag = computeOwnershipFlag(access); + Flag manifestationFlag = computeTypeManifestationFlag(access); - addReference( - new Reference.Builder(refSourceClassName) - .withMethod( - new Source[0], - methodFlags, - name, - methodType.getReturnType(), - methodType.getArgumentTypes()) - .build()); + // as an optimization skip constructors, private and static methods + if (!(visibilityFlag == Flag.PRIVATE + || ownershipFlag == Flag.STATIC + || MethodDescription.CONSTRUCTOR_INTERNAL_NAME.equals(name))) { + addReference( + new Reference.Builder(refSourceClassName) + .withSource(refSourceClassName) + .withMethod( + new Source[0], + new Flag[] {visibilityFlag, ownershipFlag, manifestationFlag}, + name, + methodType.getReturnType(), + methodType.getArgumentTypes()) + .build()); + } } // Additional references we could check diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java index c8a019fb18ad..0f73f1467659 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java @@ -182,8 +182,9 @@ private List checkHelperClassMatch(Reference helperClass, Ty mismatches, new Reference.Mismatch.MissingMethod( helperClass.getSources().toArray(new Reference.Source[0]), - helperClass.getClassName(), - unimplementedMethod.toString())); + unimplementedMethod.getDeclaringClass(), + unimplementedMethod.getName(), + unimplementedMethod.getDescriptor())); } return mismatches; @@ -193,9 +194,6 @@ private static void collectMethodsFromTypeHierarchy( HelperReferenceWrapper type, Set abstractMethods, Set plainMethods) { for (HelperReferenceWrapper.Method method : type.getMethods()) { - if (shouldIgnore(method)) { - continue; - } (method.isAbstract() ? abstractMethods : plainMethods).add(method); } @@ -204,19 +202,6 @@ private static void collectMethodsFromTypeHierarchy( } } - // ignore Object methods - sometimes defined by interfaces which causes them to be treated as - // abstract - private static boolean shouldIgnore(HelperReferenceWrapper.Method superMethod) { - return "hashCode".equals(superMethod.getName()) && "()I".equals(superMethod.getDescriptor()) - || "equals".equals(superMethod.getName()) - && "(Ljava/lang/Object;)Z".equals(superMethod.getDescriptor()) - || "toString".equals(superMethod.getName()) - && "()Ljava/lang/String;".equals(superMethod.getDescriptor()) - || "clone".equals(superMethod.getName()) - && "()Ljava/lang/Object;".equals(superMethod.getDescriptor()) - || "finalize".equals(superMethod.getName()) && "()V".equals(superMethod.getDescriptor()); - } - private static List checkMatch( Reference reference, TypeDescription typeOnClasspath) { List mismatches = Collections.emptyList(); @@ -276,7 +261,8 @@ private static List checkMatch( new Reference.Mismatch.MissingMethod( methodRef.getSources().toArray(new Reference.Source[0]), reference.getClassName(), - methodRef.getName() + methodRef.getDescriptor())); + methodRef.getName(), + methodRef.getDescriptor())); } else { for (Reference.Flag flag : methodRef.getFlags()) { if (!flag.matches(methodDescription.getModifiers())) { diff --git a/testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy b/testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy index 21d4dba464e8..0c8d098bc227 100644 --- a/testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy +++ b/testing-common/src/test/groovy/muzzle/HelperReferenceWrapperTest.groovy @@ -94,8 +94,11 @@ class HelperReferenceWrapperTest extends Specification { abstractClasspathType.hasSuperTypes() with(abstractClasspathType.superTypes.toList()) { - it.size() == 1 - with(it[0]) { interface1 -> + it.size() == 2 + with(it[0]) { object -> + !object.hasSuperTypes() + } + with(it[1]) { interface1 -> interface1.abstract with(interface1.methods.toList()) { diff --git a/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy index e9f5c34657ed..87dfee78c324 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceCreatorTest.groovy @@ -126,10 +126,6 @@ class ReferenceCreatorTest extends AgentTestRunner { Reference.Flag.PUBLIC, Reference.Flag.NON_STATIC, Reference.Flag.FINAL - assertMethod helperSuperClass, 'bar', '()I', - Reference.Flag.PACKAGE, - Reference.Flag.STATIC, - Reference.Flag.NON_FINAL } with(references[TestHelperClasses.HelperInterface.name]) { helperInterface -> @@ -138,14 +134,9 @@ class ReferenceCreatorTest extends AgentTestRunner { } with(references[TestHelperClasses.Helper.name]) { helperClass -> + helperClass.flags.contains(Reference.Flag.NON_FINAL) assertHelperSuperClassMethod helperClass, false assertHelperInterfaceMethod helperClass, false - assertMethod helperClass, 'getStr', '()Ljava/lang/String;', - // this method has both PRIVATE flags: first because it's defined in the helper class, second because it's called in foo() - Reference.Flag.PRIVATE, - Reference.Flag.PRIVATE_OR_HIGHER, - Reference.Flag.NON_STATIC, - Reference.Flag.NON_FINAL } } diff --git a/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy index fbd06f6f6962..b2aca78d05c0 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy @@ -233,9 +233,27 @@ class ReferenceMatcherTest extends AgentTestRunner { getMismatchClassSet(mismatches) == [MissingMethod] as Set } + def "should fail helper classes that does not implement all abstract methods - even if emtpy abstract class reference exists"() { + given: + def emptySuperClassRef = new Reference.Builder(TestHelperClasses.HelperSuperClass.name) + .build() + def reference = new Reference.Builder("io.opentelemetry.instrumentation.Helper") + .withSuperName(TestHelperClasses.HelperSuperClass.name) + .withMethod(new Source[0], [] as Reference.Flag[], "someMethod", Type.VOID_TYPE) + .build() + + when: + def mismatches = new ReferenceMatcher([reference.className] as String[], [reference, emptySuperClassRef] as Reference[]) + .getMismatchedReferenceSources(this.class.classLoader) + + then: + getMismatchClassSet(mismatches) == [MissingMethod] as Set + } + def "should check whether interface methods are implemented in the super class"() { given: def baseHelper = new Reference.Builder("io.opentelemetry.instrumentation.BaseHelper") + .withSuperName(Object.name) .withInterface(TestHelperClasses.HelperInterface.name) .withMethod(new Source[0], [] as Reference.Flag[], "foo", Type.VOID_TYPE) .build()