Replace bogus instructions that jump beyond the method code bounds

This kind of trick only works with -noverify so the replacement being NOP + RETURN is still 'wrong' but less wrong. ASM will not crash when reading the class, and the code where this occurs cannot be visited via normal control flow anyways. A dead code pass should remove this.

Author: Col-E

core/src/main/java/software/coley/cafedude/classfile/attribute/CodeAttribute.java

@@ -1,5 +1,7 @@
 package software.coley.cafedude.classfile.attribute;
 
+import jakarta.annotation.Nonnull;
+import jakarta.annotation.Nullable;
 import software.coley.cafedude.classfile.behavior.AttributeHolder;
 import software.coley.cafedude.classfile.behavior.CpAccessor;
 import software.coley.cafedude.classfile.constant.CpClass;
@@ -8,8 +10,6 @@
 import software.coley.cafedude.classfile.instruction.Instruction;
 import software.coley.cafedude.io.AttributeContext;
 
-import jakarta.annotation.Nonnull;
-import jakarta.annotation.Nullable;
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
@@ -50,6 +50,38 @@ public CodeAttribute(@Nonnull CpUtf8 name, int maxStack, int maxLocals, @Nonnull
 		this.attributes = attributes;
 	}
 
+	/**
+	 * A instance matching index-of since the standard {@link List#indexOf(Object)} uses object equality.
+	 *
+	 * @param instruction
+	 * 		A specific instruction in the code.
+	 *
+	 * @return Index in the instructions list where the code appears.
+	 */
+	public int indexOf(@Nonnull Instruction instruction) {
+		// We have our own index-of because 'list.indexOf' uses 'item.equals(other)' which is not what we want.
+		for (int i = 0; i < instructions.size(); i++)
+			if (instructions.get(i) == instruction)
+				return i;
+		return -1;
+	}
+
+	/**
+	 * @param instruction
+	 * 		A specific instruction in the code.
+	 *
+	 * @return Method bytecode offset of the instruction.
+	 */
+	public int computeOffsetOf(@Nonnull Instruction instruction) {
+		int index = indexOf(instruction);
+		if (index == 0) return 0;
+		if (index < 0) return -1;
+		int offset = 0;
+		for (int i = 0; i < index; i++)
+			offset += instructions.get(i).computeSize();
+		return offset;
+	}
+
 	/**
 	 * @return Instruction code data.
 	 */

core/src/main/java/software/coley/cafedude/transform/IllegalStrippingTransformer.java

@@ -53,6 +53,11 @@
 import software.coley.cafedude.classfile.constant.CpClass;
 import software.coley.cafedude.classfile.constant.CpEntry;
 import software.coley.cafedude.classfile.constant.CpUtf8;
+import software.coley.cafedude.classfile.instruction.BasicInstruction;
+import software.coley.cafedude.classfile.instruction.Instruction;
+import software.coley.cafedude.classfile.instruction.IntOperandInstruction;
+import software.coley.cafedude.classfile.instruction.LookupSwitchInstruction;
+import software.coley.cafedude.classfile.instruction.TableSwitchInstruction;
 import software.coley.cafedude.io.AttributeContext;
 
 import java.util.Collection;
@@ -64,6 +69,8 @@
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
+import static software.coley.cafedude.classfile.instruction.Opcodes.*;
+
 /**
  * A transformer to remove illegal attributes and data from a class.
  *
@@ -103,15 +110,70 @@ public void transform() {
 		clazz.getAttributes().removeIf(attribute -> !isValidWrapped(clazz, attribute));
 		for (Field field : clazz.getFields())
 			field.getAttributes().removeIf(attribute -> !isValidWrapped(field, attribute));
-		for (Method method : clazz.getMethods())
+		for (Method method : clazz.getMethods()) {
 			method.getAttributes().removeIf(attribute -> !isValidWrapped(method, attribute));
+			CodeAttribute code = method.getAttribute(CodeAttribute.class);
+			if (code != null)
+				removeInvalidInstructions(code);
+		}
 
 		// Record filtered CP refs, the difference of the sets are the indices that were referenced
 		// by removed attributes/data.
 		Set<CpEntry> filteredCpAccesses = clazz.cpAccesses();
 		cpAccesses.removeAll(filteredCpAccesses);
 	}
 
+	protected void removeInvalidInstructions(@Nonnull CodeAttribute code) {
+		List<Instruction> instructions = code.getInstructions();
+		if (instructions.size() <= 1)
+			return;
+		int maxPc = code.computeOffsetOf(instructions.get(instructions.size() - 1));
+
+		// Remove junk try-catch entries with bogus offsets
+		List<ExceptionTableEntry> exceptions = code.getExceptionTable();
+		for (int i = exceptions.size() - 1; i >= 0; i--) {
+			ExceptionTableEntry entry = exceptions.get(i);
+			if (entry.getStartPc() > maxPc || entry.getEndPc() > maxPc || entry.getHandlerPc() > maxPc)
+				exceptions.remove(i);
+		}
+
+		// Remove any jump instructions that point to bogus offsets
+		// - The code is already unverifiable, so when we stub out the instruction with
+		//   a 'return' the code is still unverifiable but fixes the ASM crash.
+		// - The number of bytes if kept the same, so valid jump instructions elsewhere
+		//   in the method are not broken.
+		// - This de-syncs the stack-frame entries for some reason I haven't looked into but
+		//   since the code that relies on these tricks already requires use of -noverify
+		//   I don't really feel like looking into it. Just use SKIP_FRAMES with ASM on these classes.
+		for (int i = instructions.size() - 1; i >= 0; i--) {
+			Instruction instruction = instructions.get(i);
+			int op = instruction.getOpcode();
+			if (((op >= IFEQ && op <= JSR) || (op == GOTO_W || op == JSR_W)) && instruction instanceof IntOperandInstruction jump) {
+				int jumpOffset = jump.getOperand();
+				if (jumpOffset > maxPc) {
+					int size = instruction.computeSize();
+					instructions.set(i, new BasicInstruction(RETURN));
+					for (int j = 0; j < size - 1; j++)
+						instructions.add(i, new BasicInstruction(NOP));
+				}
+			} else if (instruction instanceof TableSwitchInstruction tswitch) {
+				if (tswitch.getDefault() > maxPc || tswitch.getOffsets().stream().anyMatch(o -> o > maxPc)) {
+					int size = instruction.computeSize();
+					instructions.set(i, new BasicInstruction(RETURN));
+					for (int j = 0; j < size - 1; j++)
+						instructions.add(i, new BasicInstruction(NOP));
+				}
+			} else if (instruction instanceof LookupSwitchInstruction lswitch) {
+				if (lswitch.getDefault() > maxPc || lswitch.getOffsets().stream().anyMatch(o -> o > maxPc)) {
+					int size = instruction.computeSize();
+					instructions.set(i, new BasicInstruction(RETURN));
+					for (int j = 0; j < size - 1; j++)
+						instructions.add(i, new BasicInstruction(NOP));
+				}
+			}
+		}
+	}
+
 	private void removeInvalidBootstrapMethodAttribute() {
 		// ASM will try to read this attribute if any CP entry exists for DYNAMIC or INVOKE_DYNAMIC.
 		// If no methods actually refer to those CP entries, this attribute can be filled with garbage,

core/src/test/java/software/coley/cafedude/CrasherPatchingTest.java

@@ -29,7 +29,7 @@
  * Test that asserts ASM-crashing obfuscated classes are written back without the offending attributes.
  */
 public class CrasherPatchingTest {
-	private static final Set<String> noverify_code = Set.of("sample35.class");
+	private static final Set<String> noverify_code = Set.of("sample35.class", "sample36.class");
 
 	@ParameterizedTest
 	@MethodSource("supply")