Skip to content

Commit

Permalink
Muzzle should fail on unimplemented abstract methods
Browse files Browse the repository at this point in the history
  • Loading branch information
mateuszrzeszutek committed Sep 11, 2020
1 parent f0eaa5f commit f6e4705
Show file tree
Hide file tree
Showing 3 changed files with 162 additions and 26 deletions.
23 changes: 20 additions & 3 deletions javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/Reference.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -363,25 +364,36 @@ 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
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) {
Expand Down Expand Up @@ -615,6 +627,11 @@ public Builder withSuperName(String superName) {
return this;
}

public Builder withInterfaces(Collection<String> interfaceNames) {
interfaces.addAll(interfaceNames);
return this;
}

public Builder withInterface(String interfaceName) {
interfaces.add(interfaceName);
return this;
Expand Down
59 changes: 58 additions & 1 deletion ...ent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCreator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package io.opentelemetry.javaagent.tooling.muzzle;

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;
Expand Down Expand Up @@ -110,7 +112,8 @@ public static Map<String, Reference> createReferencesFrom(
return references;
}

private static boolean isInReferenceCreationPackage(String className) {
// TODO: this is also used in ReferenceMatcher, move it to some better (common) place
static boolean isInReferenceCreationPackage(String className) {
if (!className.startsWith(REFERENCE_CREATION_PACKAGE)) {
return false;
}
Expand Down Expand Up @@ -224,9 +227,24 @@ 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

List<String> fixedInterfaceNames = new ArrayList<>(interfaces.length);
for (String interfaceName : interfaces) {
fixedInterfaceNames.add(Utils.getClassName(interfaceName));
}

Reference.Builder builder = new Reference.Builder(refSourceClassName)
.withSuperName(Utils.getClassName(superName))
.withInterfaces(fixedInterfaceNames);
if (Flag.ABSTRACT.matches(access)) {
builder.withFlag(Flag.ABSTRACT);
}
addReference(builder.build());

super.visit(version, access, name, signature, superName, interfaces);
}

Expand All @@ -244,6 +262,45 @@ public FieldVisitor visitField(
@Override
public MethodVisitor visitMethod(
int access, String name, String descriptor, String signature, String[] exceptions) {

// add all helper class methods to check if they're implemented
// this only executes for helper classes

Type methodType = Type.getMethodType(descriptor);

List<Reference.Flag> methodFlags = new ArrayList<>();
if (Flag.PUBLIC.matches(access)) {
methodFlags.add(Flag.PUBLIC);
} else if (Flag.PROTECTED_OR_HIGHER.matches(access)) {
methodFlags.add(Flag.PROTECTED_OR_HIGHER);
} else if (Flag.PACKAGE_OR_HIGHER.matches(access)) {
methodFlags.add(Flag.PACKAGE_OR_HIGHER);
} else {
methodFlags.add(Flag.PRIVATE_OR_HIGHER);
}
if (Flag.STATIC.matches(access)) {
methodFlags.add(Flag.STATIC);
} else {
methodFlags.add(Flag.NON_STATIC);
}
if (Flag.ABSTRACT.matches(access)) {
methodFlags.add(Flag.ABSTRACT);
} else if (Flag.FINAL.matches(access)) {
methodFlags.add(Flag.FINAL);
} else {
methodFlags.add(Flag.NON_FINAL);
}

addReference(
new Reference.Builder(refSourceClassName)
.withMethod(
new Source[0],
methodFlags.toArray(new Flag[0]),
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(
Expand Down
106 changes: 84 additions & 22 deletions ...ent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceMatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,34 +21,34 @@
import io.opentelemetry.javaagent.bootstrap.WeakCache;
import io.opentelemetry.javaagent.tooling.AgentTooling;
import io.opentelemetry.javaagent.tooling.Utils;
import io.opentelemetry.javaagent.tooling.muzzle.Reference.Flag;
import io.opentelemetry.javaagent.tooling.muzzle.Reference.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.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.Callable;
import net.bytebuddy.description.field.FieldDescription;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.method.MethodDescription.InDefinedShape;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.pool.TypePool;
import net.bytebuddy.pool.TypePool.Resolution;

/** Matches a set of references against a classloader. */
public final class ReferenceMatcher {

private final WeakCache<ClassLoader, Boolean> mismatchCache = AgentTooling.newWeakCache();
private final Reference[] references;
private final Set<String> helperClassNames;

public ReferenceMatcher(Reference... references) {
this(new String[0], references);
}

// TODO: changing this constructor requires changes in MuzzleVisitor's code generation
public ReferenceMatcher(String[] helperClassNames, Reference[] references) {
this.references = references;
this.helperClassNames = new HashSet<>(Arrays.asList(helperClassNames));
}

public Reference[] getReferences() {
Expand Down Expand Up @@ -78,12 +78,8 @@ 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;
}
if (!checkMatch(reference, loader).isEmpty()) {
return false;
}
}

Expand All @@ -104,11 +100,7 @@ public List<Reference.Mismatch> getMismatchedReferenceSources(ClassLoader loader
List<Mismatch> 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));
}
mismatches = lazyAddAll(mismatches, checkMatch(reference, loader));
}

return mismatches;
Expand All @@ -125,13 +117,18 @@ private static List<Reference.Mismatch> checkMatch(Reference reference, ClassLoa
AgentTooling.poolStrategy()
.typePool(AgentTooling.locationStrategy().classFileLocator(loader), loader);
try {
TypePool.Resolution resolution = typePool.describe(reference.getClassName());
if (!resolution.isResolved()) {
return Collections.<Mismatch>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 (ReferenceCreator.isInReferenceCreationPackage(reference.getClassName())) {
return checkHelperClass(reference, typePool);
} else {
TypePool.Resolution resolution = typePool.describe(reference.getClassName());
if (!resolution.isResolved()) {
return Collections.<Mismatch>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
Expand All @@ -147,6 +144,71 @@ private static List<Reference.Mismatch> checkMatch(Reference reference, ClassLoa
}
}

private static List<Reference.Mismatch> checkHelperClass(
Reference helperClass, TypePool typePool) {
List<Mismatch> mismatches = Collections.emptyList();

if (helperClass.getFlags().contains(Flag.ABSTRACT)) {
return mismatches;
}

if (helperClass.getSuperName() == null) {
return mismatches;
}

Resolution resolution = typePool.describe(helperClass.getSuperName());
if (!resolution.isResolved()) {
// TODO: helper super class is a helper itself
return mismatches;
}

TypeDescription superType = resolution.resolve();

while (superType.isAbstract()) {
for (MethodDescription.InDefinedShape superMethod : superType.getDeclaredMethods()) {
if (!superMethod.isAbstract()) {
continue;
}

if (!findMethodWithSignature(helperClass, superMethod)) {
String desc =
superType.getName()
+ "#"
+ superMethod.getInternalName()
+ superMethod.getDescriptor();
mismatches =
lazyAdd(
mismatches,
new Reference.Mismatch.MissingMethod(
helperClass.getSources().toArray(new Reference.Source[0]),
helperClass.getClassName(),
desc));
}
// additional checks: method flags
}

if (superType.getSuperClass() == null) {
break;
}
superType = superType.getSuperClass().asErasure();
}

// additional checks: interface methods

return mismatches;
}

private static boolean findMethodWithSignature(
Reference helperClass, InDefinedShape superMethod) {
for (Method method : helperClass.getMethods()) {
if (method.getName().equals(superMethod.getInternalName())
&& method.getDescriptor().equals(superMethod.getDescriptor())) {
return true;
}
}
return false;
}

public static List<Reference.Mismatch> checkMatch(
Reference reference, TypeDescription typeOnClasspath) {
List<Mismatch> mismatches = Collections.emptyList();
Expand Down

0 comments on commit f6e4705

Please sign in to comment.