Add back ClassWriter to validation checks

recaf-core/src/main/java/software/coley/recaf/info/builder/JvmClassInfoBuilder.java

@@ -24,9 +24,9 @@
 public class JvmClassInfoBuilder extends AbstractClassInfoBuilder<JvmClassInfoBuilder> {
 	private byte[] bytecode;
 	private int version = JvmClassInfo.BASE_VERSION + 8; // Java 8
-	private boolean skipCustomAttributeChecks = true;
+	private boolean skipValidationChecks = true;
 	@Nullable
-	private ClassBuilderAppender classVisitor;
+	private ClassBuilderAdapter adapter;
 
 	/**
 	 * Create empty builder.
@@ -70,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}.
+	 * <p/>
+	 * <b>IMPORTANT:</b> 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.
+	 * <br>
+	 * Validation is disabled by default.
+	 * <br>
+	 * If you wish to validate the input, you must use the one of the given constructors:
+	 * <ul>
+	 *     <li>{@link #JvmClassInfoBuilder()}</li>
+	 *     <li>{@link #JvmClassInfoBuilder(JvmClassInfo)}</li>
+	 * </ul>
 	 *
 	 * @param reader
 	 * 		ASM class reader to pull data from.
@@ -83,6 +95,18 @@ public JvmClassInfoBuilder adaptFrom(@Nonnull ClassReader reader) {
 
 	/**
 	 * Copies over values by reading the contents of the class file in the reader.
+	 * <p/>
+	 * <b>IMPORTANT:</b> 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.
+	 * <br>
+	 * Validation is disabled by default.
+	 * <br>
+	 * If you wish to validate the input, you must use the one of the given constructors:
+	 * <ul>
+	 *     <li>{@link #JvmClassInfoBuilder()}</li>
+	 *     <li>{@link #JvmClassInfoBuilder(JvmClassInfo)}</li>
+	 * </ul>
 	 *
 	 * @param reader
 	 * 		ASM class reader to pull data from.
@@ -94,8 +118,11 @@ public JvmClassInfoBuilder adaptFrom(@Nonnull ClassReader reader) {
 	@Nonnull
 	@SuppressWarnings(value = "deprecation")
 	public JvmClassInfoBuilder adaptFrom(@Nonnull ClassReader reader, int flags) {
-		this.classVisitor = new ClassBuilderAppender();
-		reader.accept(classVisitor, 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);
 	}
 
@@ -112,14 +139,17 @@ public JvmClassInfoBuilder withVersion(int version) {
 	}
 
 	/**
-	 * The default value is true. When {@code verify} is run it will check if there are any custom attributes.
+	 * 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
 	 *
-	 * @param skipCustomAttributeChecks {@code false} if we want to verify the classes custom attribute
 	 * @return {@code JvmClassInfoBuilder}
 	 */
 	@Nonnull
-	public JvmClassInfoBuilder skipCustomAttributeChecks(boolean skipCustomAttributeChecks) {
-		this.skipCustomAttributeChecks = skipCustomAttributeChecks;
+	public JvmClassInfoBuilder skipValidationChecks(boolean skipValidationChecks) {
+		this.skipValidationChecks = skipValidationChecks;
 		return this;
 	}
 
@@ -144,32 +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 (!this.skipCustomAttributeChecks && classVisitor != null && classVisitor.hasCustomAttributes()) {
+		if (!skipValidationChecks && adapter != null && adapter.hasCustomAttributes()) {
 			throw new IllegalStateException("Unknown attributes found in class: " + this.getName() + "[" +
-				String.join(", ", classVisitor.getCustomAttributeNames()) + "]");
+					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<AnnotationInfo> annotations = new ArrayList<>();
 		private final List<TypeAnnotationInfo> typeAnnotations = new ArrayList<>();
 		private final List<InnerClassInfo> innerClasses = new ArrayList<>();
 		private final List<FieldMember> fields = new ArrayList<>();
 		private final List<MethodMember> methods = new ArrayList<>();
-		/**
-		 * Collects custom attributes in a class.
-		 */
 		private final List<Attribute> classCustomAttributes = new ArrayList<>();
 		private final MultiMap<String, Attribute, List<Attribute>> fieldCustomAttributes;
 		private final MultiMap<String, Attribute, List<Attribute>> methodCustomAttributes;
 
-
-		protected ClassBuilderAppender() {
-			super(getAsmVersion());
-			fieldCustomAttributes
-				= MultiMap.from(new HashMap<>(), ArrayList::new);
-			methodCustomAttributes
-				= MultiMap.from(new HashMap<>(), ArrayList::new);
+		protected ClassBuilderAdapter(@Nullable ClassVisitor cv) {
+			super(getAsmVersion(), cv);
+			fieldCustomAttributes = MultiMap.from(new HashMap<>(), ArrayList::new);
+			methodCustomAttributes = MultiMap.from(new HashMap<>(), ArrayList::new);
 		}
 
 		@Override
@@ -199,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)));
 		}
 
@@ -230,7 +261,6 @@ public void visitInnerClass(String name, String outerName, String innerName, int
 					withOuterClassName(outerName);
 				}
 			}
-
 		}
 
 		@Override
@@ -241,7 +271,7 @@ public void visitAttribute(Attribute 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) {
@@ -258,7 +288,7 @@ 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) {
@@ -291,8 +321,8 @@ public void visitEnd() {
 		 */
 		public boolean hasCustomAttributes() {
 			return !classCustomAttributes.isEmpty() ||
-				!fieldCustomAttributes.isEmpty() ||
-				!methodCustomAttributes.isEmpty();
+					!fieldCustomAttributes.isEmpty() ||
+					!methodCustomAttributes.isEmpty();
 		}
 
 		/**
@@ -301,35 +331,35 @@ public boolean hasCustomAttributes() {
 		public Collection<String> getCustomAttributeNames() {
 			Set<String> names = new TreeSet<>();
 			classCustomAttributes.stream()
-				.map(a -> a.type)
-				.forEach(names::add);
+					.map(a -> a.type)
+					.forEach(names::add);
 			fieldCustomAttributes.values()
-				.map(a -> a.type)
-				.forEach(names::add);
+					.map(a -> a.type)
+					.forEach(names::add);
 			methodCustomAttributes.values()
-				.map(a -> a.type)
-				.forEach(names::add);
+					.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)));
 		}
 
@@ -339,24 +369,24 @@ 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<String> exceptionList = exceptions == null ? Collections.emptyList() : Arrays.asList(exceptions);
 			methodMember = new BasicMethodMember(name, descriptor, signature, access, exceptionList, new ArrayList<>());
 		}
 
 		@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)));
 		}
 
@@ -372,15 +402,15 @@ public BasicMethodMember getMethodMember() {
 		}
 	}
 
-	private static class AnnotationBuilderAppender extends AnnotationVisitor {
+	private static class AnnotationBuilderAdapter extends AnnotationVisitor {
 		private final Consumer<BasicAnnotationInfo> annotationConsumer;
 		private final Map<String, AnnotationElement> elements = new HashMap<>();
 		private final List<Object> arrayValues = new ArrayList<>();
 		private final boolean visible;
 		private final String descriptor;
 
-		protected AnnotationBuilderAppender(boolean visible, String descriptor,
-											Consumer<BasicAnnotationInfo> annotationConsumer) {
+		protected AnnotationBuilderAdapter(boolean visible, String descriptor,
+										   Consumer<BasicAnnotationInfo> annotationConsumer) {
 			super(getAsmVersion());
 			this.visible = visible;
 			this.descriptor = descriptor;
@@ -412,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 {
@@ -423,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));
 				}
 			};

recaf-core/src/main/java/software/coley/recaf/workspace/io/BasicInfoImporter.java

@@ -3,6 +3,7 @@
 import jakarta.annotation.Nonnull;
 import jakarta.enterprise.context.ApplicationScoped;
 import jakarta.inject.Inject;
+import org.objectweb.asm.ClassReader;
 import org.slf4j.Logger;
 import software.coley.cafedude.classfile.VersionConstants;
 import software.coley.recaf.analytics.logging.Logging;
@@ -41,21 +42,24 @@ public Info readInfo(@Nonnull String name, @Nonnull ByteSource source) throws IO
 		// Check for Java classes
 		if (matchesClass(data)) {
 			try {
-				if (config.doSkipAsmValidation()) {
+				// 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(data)
-						.skipCustomAttributeChecks(false)
-						.build();
+					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);
 					logger.debug("CafeDude patched class: {}", name);
 					try {
 						return new JvmClassInfoBuilder(patched)
-							.skipCustomAttributeChecks(false)
+							.skipValidationChecks(false)
 							.build();
 					} catch (Throwable t1) {
 						logger.error("CafeDude patching output is still non-compliant with ASM for file: {}", name);