From eee0b036ac7ecb3145e9093bc6a1bcb733df6c0e Mon Sep 17 00:00:00 2001 From: Uwe Schindler Date: Fri, 16 Apr 2021 18:37:08 +0200 Subject: [PATCH 1/3] Refactor class hierarchy climbing with visitor (fixes also some smaller bugs) --- .../de/thetaphi/forbiddenapis/Checker.java | 14 +- .../thetaphi/forbiddenapis/ClassScanner.java | 239 +++++++++++------- .../forbiddenapis/ClassSignature.java | 4 +- .../de/thetaphi/forbiddenapis/Signatures.java | 16 +- 4 files changed, 173 insertions(+), 100 deletions(-) diff --git a/src/main/java/de/thetaphi/forbiddenapis/Checker.java b/src/main/java/de/thetaphi/forbiddenapis/Checker.java index a2633d1..5abd901 100644 --- a/src/main/java/de/thetaphi/forbiddenapis/Checker.java +++ b/src/main/java/de/thetaphi/forbiddenapis/Checker.java @@ -361,8 +361,8 @@ public void addClassToCheck(final InputStream in, String name) throws IOExceptio throw new IllegalArgumentException(String.format(Locale.ENGLISH, "The class file format of '%s' is too recent to be parsed by ASM.", name)); } - final String binaryName = Type.getObjectType(reader.getClassName()).getClassName(); - classesToCheck.put(binaryName, new ClassSignature(reader, false, true)); + final ClassSignature metadata = new ClassSignature(reader, false, true); + classesToCheck.put(metadata.getBinaryClassName(), metadata); } /** Parses and adds a class from the given file to the list of classes to check. Does not log anything. */ @@ -407,11 +407,11 @@ public void addSuppressAnnotation(String annoName) { } /** Parses a class and checks for valid method invocations */ - private int checkClass(final ClassReader reader, Pattern suppressAnnotationsPattern) throws ForbiddenApiException { - final String className = Type.getObjectType(reader.getClassName()).getClassName(); - final ClassScanner scanner = new ClassScanner(this, forbiddenSignatures, suppressAnnotationsPattern); + private int checkClass(ClassSignature c, Pattern suppressAnnotationsPattern) throws ForbiddenApiException { + final String className = c.getBinaryClassName(); + final ClassScanner scanner = new ClassScanner(c, this, forbiddenSignatures, suppressAnnotationsPattern); try { - reader.accept(scanner, ClassReader.SKIP_FRAMES); + c.getReader().accept(scanner, ClassReader.SKIP_FRAMES); } catch (RelatedClassLoadingException rcle) { final Exception cause = rcle.getException(); final StringBuilder msg = new StringBuilder() @@ -457,7 +457,7 @@ public void run() throws ForbiddenApiException { int errors = 0; final Pattern suppressAnnotationsPattern = AsmUtils.glob2Pattern(suppressAnnotations.toArray(new String[suppressAnnotations.size()])); for (final ClassSignature c : classesToCheck.values()) { - errors += checkClass(c.getReader(), suppressAnnotationsPattern); + errors += checkClass(c, suppressAnnotationsPattern); } final String message = String.format(Locale.ENGLISH, diff --git a/src/main/java/de/thetaphi/forbiddenapis/ClassScanner.java b/src/main/java/de/thetaphi/forbiddenapis/ClassScanner.java index 9cc5b78..9e09b5f 100644 --- a/src/main/java/de/thetaphi/forbiddenapis/ClassScanner.java +++ b/src/main/java/de/thetaphi/forbiddenapis/ClassScanner.java @@ -26,6 +26,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.regex.Pattern; import org.objectweb.asm.AnnotationVisitor; @@ -42,6 +43,7 @@ public final class ClassScanner extends ClassVisitor implements Constants { private final boolean forbidNonPortableRuntime; + final ClassSignature metadata; final RelatedClassLookup lookup; final List violations = new ArrayList<>(); @@ -53,7 +55,6 @@ public final class ClassScanner extends ClassVisitor implements Constants { private String source = null; private boolean isDeprecated = false; private boolean done = false; - String internalMainClassName = null; int currentGroupId = 0; // Mapping from a (possible) lambda Method to groupId of declaring method @@ -63,8 +64,9 @@ public final class ClassScanner extends ClassVisitor implements Constants { final BitSet suppressedGroups = new BitSet(); boolean classSuppressed = false; - public ClassScanner(RelatedClassLookup lookup, Signatures forbiddenSignatures, final Pattern suppressAnnotations) { + public ClassScanner(ClassSignature metadata, RelatedClassLookup lookup, Signatures forbiddenSignatures, final Pattern suppressAnnotations) { super(Opcodes.ASM9); + this.metadata = metadata; this.lookup = lookup; this.forbiddenSignatures = forbiddenSignatures; this.suppressAnnotations = suppressAnnotations; @@ -92,9 +94,9 @@ String checkClassUse(Type type, String what, boolean deep, String origInternalNa if (type.getSort() != Type.OBJECT) { return null; // we don't know this type, just pass! } - final String printout = forbiddenSignatures.checkType(type); - if (printout != null) { - return String.format(Locale.ENGLISH, "Forbidden %s use: %s", what, printout); + final String violation = forbiddenSignatures.checkType(type, what); + if (violation != null) { + return violation; } if (deep && forbidNonPortableRuntime) { final String binaryClassName = type.getClassName(); @@ -113,32 +115,91 @@ String checkClassUse(String internalName, String what, String origInternalName) return checkClassUse(Type.getObjectType(internalName), what, true, origInternalName); } - private String checkClassDefinition(String origName, String superName, String[] interfaces) { - if (superName != null) { - String violation = checkClassUse(superName, "class", origName); - if (violation != null) { - return violation; - } - final ClassSignature c = lookup.lookupRelatedClass(superName, origName); - if (c != null && (violation = checkClassDefinition(origName, c.superName, c.interfaces)) != null) { - return violation; + // TODO: @FunctionalInterface from Java 8 on + static interface AncestorVisitor { + final String STOP = new String("STOP"); + + String visit(ClassSignature c, String origName, boolean isInterfaceOfAncestor, boolean previousInRuntime); + } + + String visitAncestors(ClassSignature cls, AncestorVisitor visitor, boolean visitSelf, boolean visitInterfacesFirst) { + if (visitSelf) { + final String result = visitor.visit(cls, cls.className, cls.isInterface, cls.isRuntimeClass); + if (result != null && result != AncestorVisitor.STOP) { + return result; } } - if (interfaces != null) { - for (String intf : interfaces) { - String violation = checkClassUse(intf, "interface", origName); - if (violation != null) { - return violation; + return visitAncestorsRecursive(cls, cls.className, visitor, cls.isRuntimeClass, visitInterfacesFirst); + } + + private String visitSuperclassRecursive(ClassSignature cls, String origName, AncestorVisitor visitor, boolean previousInRuntime, boolean visitInterfacesFirst) { + if (cls.superName != null) { + final ClassSignature c = lookup.lookupRelatedClass(cls.superName, origName); + if (c != null) { + String result = visitor.visit(c, origName, false, previousInRuntime); + if (result != AncestorVisitor.STOP) { + if (result != null) { + return result; + } + result = visitAncestorsRecursive(c, origName, visitor, cls.isRuntimeClass, visitInterfacesFirst); + if (result != null) { + return result; + } } + } + } + return null; + } + + private String visitInterfacesRecursive(ClassSignature cls, String origName, AncestorVisitor visitor, boolean previousInRuntime, boolean visitInterfacesFirst) { + if (cls.interfaces != null) { + for (String intf : cls.interfaces) { final ClassSignature c = lookup.lookupRelatedClass(intf, origName); - if (c != null && (violation = checkClassDefinition(origName, c.superName, c.interfaces)) != null) { - return violation; + if (c == null) continue; + String result = visitor.visit(c, origName, true, previousInRuntime); + if (result != AncestorVisitor.STOP) { + if (result != null) { + return result; + } + result = visitAncestorsRecursive(c, origName, visitor, cls.isRuntimeClass, visitInterfacesFirst); + if (result != null) { + return result; + } } } } return null; } + private String visitAncestorsRecursive(ClassSignature cls, String origName, AncestorVisitor visitor, boolean previousInRuntime, boolean visitInterfacesFirst) { + String result; + if (visitInterfacesFirst) { + result = visitInterfacesRecursive(cls, origName, visitor, previousInRuntime, visitInterfacesFirst); + if (result != null) { + return result; + } + } + result = visitSuperclassRecursive(cls, origName, visitor, previousInRuntime, visitInterfacesFirst); + if (result != null) { + return result; + } + if (!visitInterfacesFirst) { + result = visitInterfacesRecursive(cls, origName, visitor, previousInRuntime, visitInterfacesFirst); + if (result != null) { + return result; + } + } + return null; + } + + // TODO: convert to lambda method with method reference + private final AncestorVisitor classRelationAncestorVisitor = new AncestorVisitor() { + @Override + public String visit(ClassSignature c, String origName, boolean isInterfaceOfAncestor, boolean previousInRuntime) { + return checkClassUse(c.className, isInterfaceOfAncestor ? "interface" : "class", origName); + } + }; + String checkType(Type type) { while (type != null) { String violation; @@ -150,8 +211,7 @@ String checkType(Type type) { return violation; } final ClassSignature c = lookup.lookupRelatedClass(internalName, internalName); - if (c == null) return null; - return checkClassDefinition(internalName, c.superName, c.interfaces); + return (c == null) ? null : visitAncestors(c, classRelationAncestorVisitor, false, false); case Type.ARRAY: type = type.getElementType(); break; @@ -212,9 +272,11 @@ private void reportClassViolation(String violation, String where) { @Override public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) { - this.internalMainClassName = name; + if (!Objects.equals(name, metadata.className)) { + throw new AssertionError("Wrong class parsed: " + name); + } this.isDeprecated = (access & Opcodes.ACC_DEPRECATED) != 0; - reportClassViolation(checkClassDefinition(name, superName, interfaces), "class declaration"); + reportClassViolation(visitAncestors(metadata, classRelationAncestorVisitor, false, false), "class declaration"); if (this.isDeprecated) { classSuppressed |= suppressAnnotations.matcher(DEPRECATED_TYPE.getClassName()).matches(); reportClassViolation(checkType(DEPRECATED_TYPE), "deprecation on class declaration"); @@ -310,91 +372,94 @@ public MethodVisitor visitMethod(final int access, final String name, final Stri reportMethodViolation(checkType(DEPRECATED_TYPE), "deprecation on method declaration"); } } - - private String checkMethodAccess(String owner, Method method) { + + private String checkMethodAccess(String owner, final Method method) { + if (CLASS_CONSTRUCTOR_METHOD_NAME.equals(method.getName())) { + // we don't check for violations on class constructors + return null; + } String violation = checkClassUse(owner, "class/interface", owner); if (violation != null) { return violation; } - if (CLASS_CONSTRUCTOR_METHOD_NAME.equals(method.getName())) { - // we don't check for violations on class constructors - return null; + // do a quick check that works without a ClassSignature (a more thorough check is done later): + violation = forbiddenSignatures.checkMethod(owner, method); + if (violation != null) { + return violation; } - return checkMethodAccessRecursion(owner, method, true, owner); - } - - private String checkMethodAccessRecursion(String owner, Method method, boolean checkClassUse, String origOwner) { - String printout = forbiddenSignatures.checkMethod(owner, method); - if (printout != null) { - return "Forbidden method invocation: " + printout; + if (CONSTRUCTOR_METHOD_NAME.equals(method.getName())) { + return null; // don't look into superclasses or interfaces to find constructors! } - final ClassSignature c = lookup.lookupRelatedClass(owner, origOwner); - if (c != null) { - if (c.signaturePolymorphicMethods.contains(method.getName())) { - // convert the invoked descriptor to a signature polymorphic one for the lookup - final Method lookupMethod = new Method(method.getName(), SIGNATURE_POLYMORPHIC_DESCRIPTOR); - printout = forbiddenSignatures.checkMethod(owner, lookupMethod); - if (printout != null) { - return "Forbidden method invocation: " + printout; + final ClassSignature c = lookup.lookupRelatedClass(owner, owner); + if (c == null) { + return null; + } + return visitAncestors(c, new AncestorVisitor() { + @Override + public String visit(ClassSignature c, String origName, boolean isInterfaceOfAncestor, boolean previousInRuntime) { + final Method lookupMethod; + if (c.signaturePolymorphicMethods.contains(method.getName())) { + // convert the invoked descriptor to a signature polymorphic one for the lookup + lookupMethod = new Method(method.getName(), SIGNATURE_POLYMORPHIC_DESCRIPTOR); + } else { + lookupMethod = method; } - } - String violation; - if (checkClassUse && c.methods.contains(method)) { - violation = checkClassUse(owner, "class/interface", origOwner); + if (!c.methods.contains(lookupMethod)) { + return null; + } + String violation = forbiddenSignatures.checkMethod(c.className, lookupMethod); if (violation != null) { return violation; } - } - if (CONSTRUCTOR_METHOD_NAME.equals(method.getName())) { - return null; // don't look into superclasses or interfaces to find constructors! - } - if (c.superName != null && (violation = checkMethodAccessRecursion(c.superName, method, true, origOwner)) != null) { - return violation; - } - // JVM spec says: interfaces after superclasses - if (c.interfaces != null) { - for (String intf : c.interfaces) { - // for interfaces we don't check the class use (it is too strict, if just the interface is implemented, but nothing more!): - if (intf != null && (violation = checkMethodAccessRecursion(intf, method, false, origOwner)) != null) { + // for interfaces we don't check the class use (it is too strict, if just the interface is implemented, but nothing more!): + if (!isInterfaceOfAncestor) { + violation = checkClassUse(c.className, "class", origName); + if (violation != null) { return violation; } } + return null; } - } - return null; - } - - private String checkFieldAccess(String owner, String field) { - return checkFieldAccessRecursion(owner, field, owner); + }, true, false /* JVM spec says: interfaces after superclasses */); } - - private String checkFieldAccessRecursion(String owner, String field, String origOwner) { - String violation = checkClassUse(owner, "class/interface", origOwner); + + private String checkFieldAccess(String owner, final String field) { + String violation = checkClassUse(owner, "class/interface", owner); if (violation != null) { return violation; } - final String printout = forbiddenSignatures.checkField(owner, field); - if (printout != null) { - return "Forbidden field access: " + printout; + // do a quick check that works without a ClassSignature (a more thorough check is done later): + violation = forbiddenSignatures.checkField(owner, field); + if (violation != null) { + return violation; } - final ClassSignature c = lookup.lookupRelatedClass(owner, origOwner); - // if we have seen the field already, no need to look into superclasses (fields cannot override) - if (c != null && !c.fields.contains(field)) { - if (c.interfaces != null) { - for (String intf : c.interfaces) { - if (intf != null && (violation = checkFieldAccessRecursion(intf, field, origOwner)) != null) { + final ClassSignature c = lookup.lookupRelatedClass(owner, owner); + if (c == null) { + return null; + } + return visitAncestors(c, new AncestorVisitor() { + @Override + public String visit(ClassSignature c, String origName, boolean isInterfaceOfAncestor, boolean previousInRuntime) { + if (!c.fields.contains(field)) { + return null; + } + String violation = forbiddenSignatures.checkField(c.className, field); + if (violation != null) { + return violation; + } + // for interfaces we don't check the class use (it is too strict, if just the interface is implemented, but nothing more!): + if (!isInterfaceOfAncestor) { + violation = checkClassUse(c.className, "class", origName); + if (violation != null) { return violation; } } + // we found the field and as those are not virtual, there is no need to go up in class hierarchy: + return STOP; } - // JVM spec says: superclasses after interfaces - if (c.superName != null && (violation = checkFieldAccessRecursion(c.superName, field, origOwner)) != null) { - return violation; - } - } - return null; + }, true, true /* JVM spec says: superclasses after interfaces */); } - + private String checkHandle(Handle handle, boolean checkLambdaHandle) { switch (handle.getTag()) { case Opcodes.H_GETFIELD: @@ -408,7 +473,7 @@ private String checkHandle(Handle handle, boolean checkLambdaHandle) { case Opcodes.H_NEWINVOKESPECIAL: case Opcodes.H_INVOKEINTERFACE: final Method m = new Method(handle.getName(), handle.getDesc()); - if (checkLambdaHandle && handle.getOwner().equals(internalMainClassName) && handle.getName().startsWith(LAMBDA_METHOD_NAME_PREFIX)) { + if (checkLambdaHandle && handle.getOwner().equals(metadata.className) && handle.getName().startsWith(LAMBDA_METHOD_NAME_PREFIX)) { // as described in , // we will record this metafactory call as "lambda" invokedynamic, // so we can assign the called lambda with the same groupId like *this* method: diff --git a/src/main/java/de/thetaphi/forbiddenapis/ClassSignature.java b/src/main/java/de/thetaphi/forbiddenapis/ClassSignature.java index 7fabcde..3a4c09a 100644 --- a/src/main/java/de/thetaphi/forbiddenapis/ClassSignature.java +++ b/src/main/java/de/thetaphi/forbiddenapis/ClassSignature.java @@ -36,7 +36,7 @@ final class ClassSignature implements Constants { private ClassReader reader; - public final boolean isRuntimeClass, isNonPortableRuntime; + public final boolean isRuntimeClass, isNonPortableRuntime, isInterface; public final Set methods; public final Set fields, signaturePolymorphicMethods; public final String className, superName; @@ -49,6 +49,7 @@ public ClassSignature(final ClassReader classReader, boolean isRuntimeClass, boo this.className = classReader.getClassName(); this.superName = classReader.getSuperName(); this.interfaces = classReader.getInterfaces(); + this.isInterface = (classReader.getAccess() & Opcodes.ACC_INTERFACE) != 0; final Set methods = new HashSet<>(); final Set fields = new HashSet<>(); final Set signaturePolymorphicMethods = new HashSet<>(); @@ -91,6 +92,7 @@ public ClassSignature(final Class clazz, boolean isRuntimeClass) { for (int i = 0; i < interfClasses.length; i++) { this.interfaces[i] = Type.getType(interfClasses[i]).getInternalName(); } + this.isInterface = clazz.isInterface(); final Set methods = new HashSet<>(); final Set fields = new HashSet<>(); final Set signaturePolymorphicMethods = new HashSet<>(); diff --git a/src/main/java/de/thetaphi/forbiddenapis/Signatures.java b/src/main/java/de/thetaphi/forbiddenapis/Signatures.java index 5c5e7e2..b2e4fec 100644 --- a/src/main/java/de/thetaphi/forbiddenapis/Signatures.java +++ b/src/main/java/de/thetaphi/forbiddenapis/Signatures.java @@ -345,29 +345,35 @@ public boolean isNonPortableRuntimeForbidden() { return this.forbidNonPortableRuntime; } - public String checkType(Type type) { + private static String formatTypePrintout(String printout, String what) { + return String.format(Locale.ENGLISH, "Forbidden %s use: %s", what, printout); + } + + public String checkType(Type type, String what) { if (type.getSort() != Type.OBJECT) { return null; // we don't know this type, just pass! } final String printout = signatures.get(getKey(type.getInternalName())); if (printout != null) { - return printout; + return formatTypePrintout(printout, what); } final String binaryClassName = type.getClassName(); for (final ClassPatternRule r : classPatterns) { if (r.matches(binaryClassName)) { - return r.getPrintout(binaryClassName); + return formatTypePrintout(r.getPrintout(binaryClassName), what); } } return null; } public String checkMethod(String internalClassName, Method method) { - return signatures.get(getKey(internalClassName, method)); + final String printout = signatures.get(getKey(internalClassName, method)); + return (printout == null) ? null : "Forbidden method invocation: ".concat(printout); } public String checkField(String internalClassName, String field) { - return signatures.get(getKey(internalClassName, field)); + final String printout = signatures.get(getKey(internalClassName, field)); + return (printout == null) ? null : "Forbidden field access: ".concat(printout); } public static String fixTargetVersion(String name) throws ParseException { From accce12fea9d24aa09ee4be54106870a58191587 Mon Sep 17 00:00:00 2001 From: Uwe Schindler Date: Fri, 16 Apr 2021 19:21:48 +0200 Subject: [PATCH 2/3] Fix JDK-17 bug with RandomSupport extending internal/nonportable classes --- .../java/de/thetaphi/forbiddenapis/ClassScanner.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/main/java/de/thetaphi/forbiddenapis/ClassScanner.java b/src/main/java/de/thetaphi/forbiddenapis/ClassScanner.java index 9e09b5f..0210dbe 100644 --- a/src/main/java/de/thetaphi/forbiddenapis/ClassScanner.java +++ b/src/main/java/de/thetaphi/forbiddenapis/ClassScanner.java @@ -196,6 +196,9 @@ private String visitAncestorsRecursive(ClassSignature cls, String origName, Ance private final AncestorVisitor classRelationAncestorVisitor = new AncestorVisitor() { @Override public String visit(ClassSignature c, String origName, boolean isInterfaceOfAncestor, boolean previousInRuntime) { + if (previousInRuntime && c.isNonPortableRuntime) { + return null; // something inside the JVM is extending internal class/interface + } return checkClassUse(c.className, isInterfaceOfAncestor ? "interface" : "class", origName); } }; @@ -407,6 +410,9 @@ public String visit(ClassSignature c, String origName, boolean isInterfaceOfAnce if (!c.methods.contains(lookupMethod)) { return null; } + if (previousInRuntime && c.isNonPortableRuntime) { + return null; // something inside the JVM is extending internal class/interface + } String violation = forbiddenSignatures.checkMethod(c.className, lookupMethod); if (violation != null) { return violation; @@ -443,6 +449,10 @@ public String visit(ClassSignature c, String origName, boolean isInterfaceOfAnce if (!c.fields.contains(field)) { return null; } + // we found the field: from now on we use STOP to exit, because fields are not virtual! + if (previousInRuntime && c.isNonPortableRuntime) { + return STOP; // something inside the JVM is extending internal class/interface + } String violation = forbiddenSignatures.checkField(c.className, field); if (violation != null) { return violation; From fe7737dea9c95e68b314a5d29590b7152ab5f7e3 Mon Sep 17 00:00:00 2001 From: Uwe Schindler Date: Fri, 16 Apr 2021 20:52:47 +0200 Subject: [PATCH 3/3] Rename ClassSignature to ClassMetadata --- .../de/thetaphi/forbiddenapis/Checker.java | 28 ++++++++-------- ...ClassSignature.java => ClassMetadata.java} | 6 ++-- .../thetaphi/forbiddenapis/ClassScanner.java | 32 +++++++++---------- .../forbiddenapis/RelatedClassLookup.java | 4 +-- .../de/thetaphi/forbiddenapis/Signatures.java | 2 +- .../forbiddenapis/CheckerSetupTest.java | 4 +-- 6 files changed, 38 insertions(+), 38 deletions(-) rename src/main/java/de/thetaphi/forbiddenapis/{ClassSignature.java => ClassMetadata.java} (96%) diff --git a/src/main/java/de/thetaphi/forbiddenapis/Checker.java b/src/main/java/de/thetaphi/forbiddenapis/Checker.java index 5abd901..3992c3b 100644 --- a/src/main/java/de/thetaphi/forbiddenapis/Checker.java +++ b/src/main/java/de/thetaphi/forbiddenapis/Checker.java @@ -70,9 +70,9 @@ public static enum Option { final EnumSet