diff --git a/codecov.yml b/codecov.yml new file mode 100644 index 000000000..1b215550e --- /dev/null +++ b/codecov.yml @@ -0,0 +1,10 @@ +coverage: + precision: 2 + round: down + status: + project: + default: + informational: true + patch: + default: + informational: true \ No newline at end of file diff --git a/recaf-core/src/main/java/software/coley/recaf/info/builder/JvmClassInfoBuilder.java b/recaf-core/src/main/java/software/coley/recaf/info/builder/JvmClassInfoBuilder.java index 0d5e97f2e..6bc8d6acf 100644 --- a/recaf-core/src/main/java/software/coley/recaf/info/builder/JvmClassInfoBuilder.java +++ b/recaf-core/src/main/java/software/coley/recaf/info/builder/JvmClassInfoBuilder.java @@ -1,6 +1,7 @@ package software.coley.recaf.info.builder; import jakarta.annotation.Nonnull; +import jakarta.annotation.Nullable; import org.objectweb.asm.*; import software.coley.recaf.info.BasicInnerClassInfo; import software.coley.recaf.info.BasicJvmClassInfo; @@ -8,6 +9,7 @@ import software.coley.recaf.info.JvmClassInfo; import software.coley.recaf.info.annotation.*; import software.coley.recaf.info.member.*; +import software.coley.recaf.util.MultiMap; import java.util.*; import java.util.function.Consumer; @@ -22,6 +24,9 @@ public class JvmClassInfoBuilder extends AbstractClassInfoBuilder { private byte[] bytecode; private int version = JvmClassInfo.BASE_VERSION + 8; // Java 8 + private boolean skipValidationChecks = true; + @Nullable + private ClassBuilderAdapter adapter; /** * Create empty builder. @@ -65,6 +70,18 @@ public JvmClassInfoBuilder(@Nonnull byte[] bytecode) { /** * Copies over values by reading the contents of the class file in the reader. * Calls {@link #adaptFrom(ClassReader, int)} with {@code flags=0}. + *

+ * IMPORTANT: If {@link #skipValidationChecks(boolean)} is {@code false} and validation checks are active + * extra steps are taken to ensure the class is fully ASM compliant. You will want to wrap this call in a try-catch + * block handling {@link Throwable} to cover any potential ASM failure. + *
+ * Validation is disabled by default. + *
+ * If you wish to validate the input, you must use the one of the given constructors: + *

* * @param reader * ASM class reader to pull data from. @@ -78,6 +95,18 @@ public JvmClassInfoBuilder adaptFrom(@Nonnull ClassReader reader) { /** * Copies over values by reading the contents of the class file in the reader. + *

+ * IMPORTANT: If {@link #skipValidationChecks(boolean)} is {@code false} and validation checks are active + * extra steps are taken to ensure the class is fully ASM compliant. You will want to wrap this call in a try-catch + * block handling {@link Throwable} to cover any potential ASM failure. + *
+ * Validation is disabled by default. + *
+ * If you wish to validate the input, you must use the one of the given constructors: + *

* * @param reader * ASM class reader to pull data from. @@ -89,7 +118,11 @@ public JvmClassInfoBuilder adaptFrom(@Nonnull ClassReader reader) { @Nonnull @SuppressWarnings(value = "deprecation") public JvmClassInfoBuilder adaptFrom(@Nonnull ClassReader reader, int flags) { - reader.accept(new ClassBuilderAppender(), flags); + // If we are doing validation checks, delegating the reader to a writer should catch most issues + // that would normally crash ASM. It is the caller's responsibility to error handle ASM failing + // if such failures occur. + adapter = new ClassBuilderAdapter(skipValidationChecks ? null : new ClassWriter(reader, 0)); + reader.accept(adapter, flags); return withBytecode(reader.b); } @@ -105,6 +138,21 @@ public JvmClassInfoBuilder withVersion(int version) { return this; } + /** + * The default value is {@code true}. Setting to {@code false} enables class validation steps. + * When {@link #verify()} is run it will check if there are any custom attributes. + * + * @param skipValidationChecks + * {@code false} if we want to verify the classes custom attribute + * + * @return {@code JvmClassInfoBuilder} + */ + @Nonnull + public JvmClassInfoBuilder skipValidationChecks(boolean skipValidationChecks) { + this.skipValidationChecks = skipValidationChecks; + return this; + } + public byte[] getBytecode() { return bytecode; } @@ -126,17 +174,33 @@ protected void verify() { throw new IllegalStateException("Bytecode required"); if (version < JvmClassInfo.BASE_VERSION) throw new IllegalStateException("Version cannot be lower than 44 (v1)"); + if (!skipValidationChecks && adapter != null && adapter.hasCustomAttributes()) { + throw new IllegalStateException("Unknown attributes found in class: " + this.getName() + "[" + + String.join(", ", adapter.getCustomAttributeNames()) + "]"); + } } - private class ClassBuilderAppender extends ClassVisitor { + /** + * Converts ASM visitor actions to 'with' actions in the class builder. + * Results in a fully reconstructed class model. + * + * @see FieldBuilderAdapter + * @see MethodBuilderAdapter + */ + private class ClassBuilderAdapter extends ClassVisitor { private final List annotations = new ArrayList<>(); private final List typeAnnotations = new ArrayList<>(); private final List innerClasses = new ArrayList<>(); private final List fields = new ArrayList<>(); private final List methods = new ArrayList<>(); - - protected ClassBuilderAppender() { - super(getAsmVersion()); + private final List classCustomAttributes = new ArrayList<>(); + private final MultiMap> fieldCustomAttributes; + private final MultiMap> methodCustomAttributes; + + protected ClassBuilderAdapter(@Nullable ClassVisitor cv) { + super(getAsmVersion(), cv); + fieldCustomAttributes = MultiMap.from(new HashMap<>(), ArrayList::new); + methodCustomAttributes = MultiMap.from(new HashMap<>(), ArrayList::new); } @Override @@ -166,12 +230,12 @@ public void visitOuterClass(String owner, String name, String descriptor) { @Override public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) { - return new AnnotationBuilderAppender(visible, descriptor, annotations::add); + return new AnnotationBuilderAdapter(visible, descriptor, annotations::add); } @Override public AnnotationVisitor visitTypeAnnotation(int typeRef, TypePath typePath, String descriptor, boolean visible) { - return new AnnotationBuilderAppender(visible, descriptor, + return new AnnotationBuilderAdapter(visible, descriptor, anno -> typeAnnotations.add(anno.withTypeInfo(typeRef, typePath))); } @@ -197,12 +261,24 @@ public void visitInnerClass(String name, String outerName, String innerName, int withOuterClassName(outerName); } } + } + @Override + public void visitAttribute(Attribute attribute) { + classCustomAttributes.add(attribute); + super.visitAttribute(attribute); } @Override public FieldVisitor visitField(int access, String name, String descriptor, String signature, Object value) { - return new FieldBuilderAppender(access, name, descriptor, signature, value) { + return new FieldBuilderAdapter(access, name, descriptor, signature, value) { + + @Override + public void visitAttribute(Attribute attribute) { + fieldCustomAttributes.get(name).add(attribute); + super.visitAttribute(attribute); + } + @Override public void visitEnd() { fields.add(getFieldMember()); @@ -212,7 +288,14 @@ public void visitEnd() { @Override public MethodVisitor visitMethod(int access, String name, String descriptor, String signature, String[] exceptions) { - return new MethodBuilderAppender(access, name, descriptor, signature, exceptions) { + return new MethodBuilderAdapter(access, name, descriptor, signature, exceptions) { + + @Override + public void visitAttribute(Attribute attribute) { + methodCustomAttributes.get(name).add(attribute); + super.visitAttribute(attribute); + } + @Override public void visitEnd() { methods.add(getMethodMember()); @@ -232,25 +315,51 @@ public void visitEnd() { withAnnotations(annotations); withTypeAnnotations(typeAnnotations); } + + /** + * @return {@code true} when any custom attributes were found. + */ + public boolean hasCustomAttributes() { + return !classCustomAttributes.isEmpty() || + !fieldCustomAttributes.isEmpty() || + !methodCustomAttributes.isEmpty(); + } + + /** + * @return Unique names of attributes found. + */ + public Collection getCustomAttributeNames() { + Set names = new TreeSet<>(); + classCustomAttributes.stream() + .map(a -> a.type) + .forEach(names::add); + fieldCustomAttributes.values() + .map(a -> a.type) + .forEach(names::add); + methodCustomAttributes.values() + .map(a -> a.type) + .forEach(names::add); + return names; + } } - private static class FieldBuilderAppender extends FieldVisitor { + private static class FieldBuilderAdapter extends FieldVisitor { private final BasicFieldMember fieldMember; - public FieldBuilderAppender(int access, String name, String descriptor, - String signature, Object value) { + public FieldBuilderAdapter(int access, String name, String descriptor, + String signature, Object value) { super(getAsmVersion()); fieldMember = new BasicFieldMember(name, descriptor, signature, access, value); } @Override public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) { - return new AnnotationBuilderAppender(visible, descriptor, fieldMember::addAnnotation); + return new AnnotationBuilderAdapter(visible, descriptor, fieldMember::addAnnotation); } @Override public AnnotationVisitor visitTypeAnnotation(int typeRef, TypePath typePath, String descriptor, boolean visible) { - return new AnnotationBuilderAppender(visible, descriptor, + return new AnnotationBuilderAdapter(visible, descriptor, anno -> fieldMember.addTypeAnnotation(anno.withTypeInfo(typeRef, typePath))); } @@ -260,11 +369,11 @@ public BasicFieldMember getFieldMember() { } } - private static class MethodBuilderAppender extends MethodVisitor { + private static class MethodBuilderAdapter extends MethodVisitor { private final BasicMethodMember methodMember; - public MethodBuilderAppender(int access, String name, String descriptor, - String signature, String[] exceptions) { + public MethodBuilderAdapter(int access, String name, String descriptor, + String signature, String[] exceptions) { super(getAsmVersion()); List exceptionList = exceptions == null ? Collections.emptyList() : Arrays.asList(exceptions); methodMember = new BasicMethodMember(name, descriptor, signature, access, exceptionList, new ArrayList<>()); @@ -272,12 +381,12 @@ public MethodBuilderAppender(int access, String name, String descriptor, @Override public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) { - return new AnnotationBuilderAppender(visible, descriptor, methodMember::addAnnotation); + return new AnnotationBuilderAdapter(visible, descriptor, methodMember::addAnnotation); } @Override public AnnotationVisitor visitTypeAnnotation(int typeRef, TypePath typePath, String descriptor, boolean visible) { - return new AnnotationBuilderAppender(visible, descriptor, + return new AnnotationBuilderAdapter(visible, descriptor, anno -> methodMember.addTypeAnnotation(anno.withTypeInfo(typeRef, typePath))); } @@ -293,15 +402,15 @@ public BasicMethodMember getMethodMember() { } } - private static class AnnotationBuilderAppender extends AnnotationVisitor { + private static class AnnotationBuilderAdapter extends AnnotationVisitor { private final Consumer annotationConsumer; private final Map elements = new HashMap<>(); private final List arrayValues = new ArrayList<>(); private final boolean visible; private final String descriptor; - protected AnnotationBuilderAppender(boolean visible, String descriptor, - Consumer annotationConsumer) { + protected AnnotationBuilderAdapter(boolean visible, String descriptor, + Consumer annotationConsumer) { super(getAsmVersion()); this.visible = visible; this.descriptor = descriptor; @@ -333,7 +442,7 @@ public void visitEnum(String name, String descriptor, String value) { @Override public AnnotationVisitor visitAnnotation(String name, String descriptor) { - return new AnnotationBuilderAppender(true, descriptor, anno -> { + return new AnnotationBuilderAdapter(true, descriptor, anno -> { if (name == null) { arrayValues.add(anno); } else { @@ -344,11 +453,11 @@ public AnnotationVisitor visitAnnotation(String name, String descriptor) { @Override public AnnotationVisitor visitArray(String name) { - AnnotationBuilderAppender outer = this; - return new AnnotationBuilderAppender(true, "", null) { + AnnotationBuilderAdapter outer = this; + return new AnnotationBuilderAdapter(true, "", null) { @Override public void visitEnd() { - AnnotationBuilderAppender inner = this; + AnnotationBuilderAdapter inner = this; outer.elements.put(name, new BasicAnnotationElement(name, inner.arrayValues)); } }; diff --git a/recaf-core/src/main/java/software/coley/recaf/util/visitors/CustomAttributeCollectingVisitor.java b/recaf-core/src/main/java/software/coley/recaf/util/visitors/CustomAttributeCollectingVisitor.java deleted file mode 100644 index 08bae89c6..000000000 --- a/recaf-core/src/main/java/software/coley/recaf/util/visitors/CustomAttributeCollectingVisitor.java +++ /dev/null @@ -1,111 +0,0 @@ -package software.coley.recaf.util.visitors; - -import jakarta.annotation.Nullable; -import org.objectweb.asm.Attribute; -import org.objectweb.asm.ClassVisitor; -import org.objectweb.asm.FieldVisitor; -import org.objectweb.asm.MethodVisitor; -import software.coley.recaf.RecafConstants; -import software.coley.recaf.util.MultiMap; - -import java.util.*; - -/** - * Collects custom attributes in a class. - * - * @author Matt Coley - */ -public class CustomAttributeCollectingVisitor extends ClassVisitor { - private final List classCustomAttributes = new ArrayList<>(); - private final MultiMap> fieldCustomAttributes; - private final MultiMap> methodCustomAttributes; - - /** - * @param cv - * Parent visitor. - */ - public CustomAttributeCollectingVisitor(@Nullable ClassVisitor cv) { - super(RecafConstants.getAsmVersion(), cv); - fieldCustomAttributes - = MultiMap.from(new HashMap<>(), ArrayList::new); - methodCustomAttributes - = MultiMap.from(new HashMap<>(), ArrayList::new); - } - - /** - * @return {@code true} when any custom attributes were found. - */ - public boolean hasCustomAttributes() { - return classCustomAttributes.size() > 0 || - fieldCustomAttributes.size() > 0 || - methodCustomAttributes.size() > 0; - } - - /** - * @return Unique names of attributes found. - */ - public Collection getCustomAttributeNames() { - Set names = new TreeSet<>(); - classCustomAttributes.stream() - .map(a -> a.type) - .forEach(names::add); - fieldCustomAttributes.values() - .map(a -> a.type) - .forEach(names::add); - methodCustomAttributes.values() - .map(a -> a.type) - .forEach(names::add); - return names; - } - - /** - * @return Class level custom attributes. - */ - public List getClassCustomAttributes() { - return classCustomAttributes; - } - - /** - * @return Field level custom attributes. Keys are field names. - */ - public MultiMap> getFieldCustomAttributes() { - return fieldCustomAttributes; - } - - /** - * @return Method level custom attributes. Keys are method names. - */ - public MultiMap> getMethodCustomAttributes() { - return methodCustomAttributes; - } - - @Override - public void visitAttribute(Attribute attribute) { - classCustomAttributes.add(attribute); - super.visitAttribute(attribute); - } - - @Override - public FieldVisitor visitField(int access, String name, String descriptor, String signature, Object value) { - FieldVisitor fv = super.visitField(access, name, descriptor, signature, value); - return new FieldVisitor(RecafConstants.getAsmVersion(), fv) { - @Override - public void visitAttribute(Attribute attribute) { - fieldCustomAttributes.get(name).add(attribute); - super.visitAttribute(attribute); - } - }; - } - - @Override - public MethodVisitor visitMethod(int access, String name, String descriptor, String signature, String[] exceptions) { - MethodVisitor mv = super.visitMethod(access, name, descriptor, signature, exceptions); - return new MethodVisitor(RecafConstants.getAsmVersion(), mv) { - @Override - public void visitAttribute(Attribute attribute) { - methodCustomAttributes.get(name).add(attribute); - super.visitAttribute(attribute); - } - }; - } -} diff --git a/recaf-core/src/main/java/software/coley/recaf/workspace/io/BasicInfoImporter.java b/recaf-core/src/main/java/software/coley/recaf/workspace/io/BasicInfoImporter.java index 90cdddcdf..1ce946524 100644 --- a/recaf-core/src/main/java/software/coley/recaf/workspace/io/BasicInfoImporter.java +++ b/recaf-core/src/main/java/software/coley/recaf/workspace/io/BasicInfoImporter.java @@ -4,7 +4,6 @@ import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; import org.objectweb.asm.ClassReader; -import org.objectweb.asm.ClassWriter; import org.slf4j.Logger; import software.coley.cafedude.classfile.VersionConstants; import software.coley.recaf.analytics.logging.Logging; @@ -15,7 +14,6 @@ import software.coley.recaf.util.IOUtil; import software.coley.recaf.util.android.AndroidXmlUtil; import software.coley.recaf.util.io.ByteSource; -import software.coley.recaf.util.visitors.CustomAttributeCollectingVisitor; import java.io.IOException; @@ -44,26 +42,33 @@ public Info readInfo(@Nonnull String name, @Nonnull ByteSource source) throws IO // Check for Java classes if (matchesClass(data)) { try { - // Patch if not compatible with ASM - if (!isAsmCompliantClass(data)) { + // If we're skipping validation, any ASM parse failures will result in the class + // being treated as a file instead (see catch block) + if (config.doSkipAsmValidation()) + return new JvmClassInfoBuilder(data).build(); + + // If we are doing validation, disable skipping ASM checks. + try { + return new JvmClassInfoBuilder() + .skipValidationChecks(false) + .adaptFrom(new ClassReader(data)) + .build(); + } catch (Throwable t) { + // Patch if not compatible with ASM byte[] patched = classPatcher.patch(name, data); - - // Ensure the patch was successful - if (!isAsmCompliantClass(patched)) { + logger.debug("CafeDude patched class: {}", name); + try { + return new JvmClassInfoBuilder(patched) + .skipValidationChecks(false) + .build(); + } catch (Throwable t1) { logger.error("CafeDude patching output is still non-compliant with ASM for file: {}", name); return new FileInfoBuilder<>() - .withRawContent(data) - .withName(name) - .build(); - } else { - logger.debug("CafeDude patched class: {}", name); - return new JvmClassInfoBuilder(patched) - .build(); + .withRawContent(data) + .withName(name) + .build(); } } - - // Yield class - return new JvmClassInfoBuilder(data).build(); } catch (Throwable t) { // Invalid class, either some new edge case we need to add to CafeDude, or the file // isn't a class, but the structure models it close enough to look like one at a glance. @@ -181,34 +186,6 @@ private static boolean matchesClass(byte[] content) { return cpSize >= 4; } - /** - * Check if the class can be parsed by ASM. - * - * @param content - * The class file content. - * - * @return {@code true} if ASM can parse the class. - */ - private boolean isAsmCompliantClass(byte[] content) { - // Skip validation if configured. - if (config.doSkipAsmValidation()) - return true; - - // ASM should be able to write back the read class. - try { - CustomAttributeCollectingVisitor visitor = new CustomAttributeCollectingVisitor(new ClassWriter(0)); - ClassReader reader = new ClassReader(content); - reader.accept(visitor, 0); - if (visitor.hasCustomAttributes()) { - throw new IllegalStateException("Unknown attributes found in class: " + reader.getClassName() + "[" + - String.join(", ", visitor.getCustomAttributeNames()) + "]"); - } - return true; - } catch (Throwable t) { - return false; - } - } - @Nonnull @Override public String getServiceId() {