fix: handle NOP instructions in unexpected places (#666)

jadx-core/src/main/java/jadx/core/dex/instructions/InsnDecoder.java

@@ -1,6 +1,7 @@
 package jadx.core.dex.instructions;
 
 import java.io.EOFException;
+import java.util.function.Predicate;
 
 import org.jetbrains.annotations.NotNull;
 import org.slf4j.Logger;
@@ -598,7 +599,7 @@ private ArgType tryResolveFieldType(FieldInfo igetFld) {
 
 	private InsnNode decodeSwitch(DecodedInstruction insn, int offset, boolean packed) {
 		int payloadOffset = insn.getTarget();
-		DecodedInstruction payload = insnArr[payloadOffset];
+		DecodedInstruction payload = getInsnByOffsetSkipNop(insnArr, payloadOffset);
 		Object[] keys;
 		int[] targets;
 		if (packed) {
@@ -626,7 +627,7 @@ private InsnNode decodeSwitch(DecodedInstruction insn, int offset, boolean packe
 	}
 
 	private InsnNode fillArray(DecodedInstruction insn) {
-		DecodedInstruction payload = insnArr[insn.getTarget()];
+		DecodedInstruction payload = getInsnByOffsetSkipNop(insnArr, insn.getTarget());
 		return new FillArrayNode(insn.getA(), (FillArrayDataPayloadDecodedInstruction) payload);
 	}
 
@@ -738,7 +739,7 @@ private InsnNode insn(InsnType type, RegisterArg res, InsnArg arg) {
 	}
 
 	private int getMoveResultRegister(DecodedInstruction[] insnArr, int offset) {
-		int nextOffset = getNextInsnOffset(insnArr, offset);
+		int nextOffset = getNextInsnOffsetSkipNop(insnArr, offset);
 		if (nextOffset >= 0) {
 			DecodedInstruction next = insnArr[nextOffset];
 			int opc = next.getOpcode();
@@ -751,21 +752,35 @@ private int getMoveResultRegister(DecodedInstruction[] insnArr, int offset) {
 		return -1;
 	}
 
-	public static int getPrevInsnOffset(Object[] insnArr, int offset) {
-		int i = offset - 1;
-		while (i >= 0 && insnArr[i] == null) {
-			i--;
-		}
-		if (i < 0) {
-			return -1;
+	private static DecodedInstruction getInsnByOffsetSkipNop(DecodedInstruction[] insnArr, int offset) {
+		DecodedInstruction payload = insnArr[offset];
+		if (payload.getOpcode() == Opcodes.NOP) {
+			return insnArr[getNextInsnOffsetSkipNop(insnArr, offset)];
 		}
-		return i;
+		return payload;
+	}
+
+	public static int getNextInsnOffset(DecodedInstruction[] insnArr, int offset) {
+		return getNextInsnOffset(insnArr, offset, null);
 	}
 
-	public static int getNextInsnOffset(Object[] insnArr, int offset) {
+	public static int getNextInsnOffsetSkipNop(DecodedInstruction[] insnArr, int offset) {
+		return getNextInsnOffset(insnArr, offset, i -> i.getOpcode() == Opcodes.NOP);
+	}
+
+	public static int getNextInsnOffset(InsnNode[] insnArr, int offset) {
+		return getNextInsnOffset(insnArr, offset, null);
+	}
+
+	public static <T> int getNextInsnOffset(T[] insnArr, int offset, Predicate<T> skip) {
 		int i = offset + 1;
-		while (i < insnArr.length && insnArr[i] == null) {
-			i++;
+		while (i < insnArr.length) {
+			T insn = insnArr[i];
+			if (insn == null || (skip != null && skip.test(insn))) {
+				i++;
+			} else {
+				break;
+			}
 		}
 		if (i >= insnArr.length) {
 			return -1;

jadx-core/src/main/java/jadx/core/dex/instructions/SwitchNode.java

@@ -64,6 +64,9 @@ public void initBlocks(BlockNode curBlock) {
 
 	@Override
 	public boolean replaceTargetBlock(BlockNode origin, BlockNode replace) {
+		if (targetBlocks == null) {
+			return false;
+		}
 		int count = 0;
 		int len = targetBlocks.length;
 		for (int i = 0; i < len; i++) {

jadx-core/src/main/java/jadx/core/dex/visitors/blocksmaker/BlockProcessor.java

@@ -87,38 +87,6 @@ private static void checkForUnreachableBlocks(MethodNode mth) {
 		});
 	}
 
-	private static boolean canRemoveBlock(BlockNode block) {
-		return block.getInstructions().isEmpty()
-				&& block.isAttrStorageEmpty()
-				&& block.getSuccessors().size() <= 1
-				&& !block.getPredecessors().isEmpty();
-	}
-
-	private static boolean removeEmptyBlock(BlockNode block) {
-		if (canRemoveBlock(block)) {
-			if (block.getSuccessors().size() == 1) {
-				BlockNode successor = block.getSuccessors().get(0);
-				block.getPredecessors().forEach(pred -> {
-					pred.getSuccessors().remove(block);
-					BlockSplitter.connect(pred, successor);
-					BlockSplitter.replaceTarget(pred, block, successor);
-					pred.updateCleanSuccessors();
-				});
-				BlockSplitter.removeConnection(block, successor);
-			} else {
-				block.getPredecessors().forEach(pred -> {
-					pred.getSuccessors().remove(block);
-					pred.updateCleanSuccessors();
-				});
-			}
-			block.add(AFlag.REMOVE);
-			block.getSuccessors().clear();
-			block.getPredecessors().clear();
-			return true;
-		}
-		return false;
-	}
-
 	private static boolean deduplicateBlockInsns(BlockNode block) {
 		if (block.contains(AFlag.LOOP_START) || block.contains(AFlag.LOOP_END)) {
 			// search for same instruction at end of all predecessors blocks
@@ -457,7 +425,7 @@ private static boolean independentBlockTreeMod(MethodNode mth) {
 			}
 		}
 		for (BlockNode basicBlock : basicBlocks) {
-			if (removeEmptyBlock(basicBlock)) {
+			if (BlockSplitter.removeEmptyBlock(basicBlock)) {
 				changed = true;
 			}
 		}
@@ -631,7 +599,7 @@ private static boolean mergeHandlers(MethodNode mth, List<BlockNode> blocksForMe
 			ExceptionHandler excHandler = otherExcHandlerAttr.getHandler();
 			excHandlerAttr.getHandler().addCatchTypes(excHandler.getCatchTypes());
 			tryBlock.removeHandler(mth, excHandler);
-			detachBlock(block);
+			BlockSplitter.detachBlock(block);
 		}
 		return true;
 	}
@@ -734,20 +702,6 @@ private static void removeMarkedBlocks(MethodNode mth) {
 		});
 	}
 
-	private static void detachBlock(BlockNode block) {
-		for (BlockNode pred : block.getPredecessors()) {
-			pred.getSuccessors().remove(block);
-			pred.updateCleanSuccessors();
-		}
-		for (BlockNode successor : block.getSuccessors()) {
-			successor.getPredecessors().remove(block);
-		}
-		block.add(AFlag.REMOVE);
-		block.getInstructions().clear();
-		block.getPredecessors().clear();
-		block.getSuccessors().clear();
-	}
-
 	private static void clearBlocksState(MethodNode mth) {
 		mth.getBasicBlocks().forEach(block -> {
 			block.remove(AType.LOOP);

jadx-core/src/main/java/jadx/core/dex/visitors/blocksmaker/BlockSplitter.java

@@ -50,13 +50,14 @@ public void visit(MethodNode mth) {
 
 		mth.initBasicBlocks();
 		splitBasicBlocks(mth);
+		initBlocksInTargetNodes(mth);
+
 		removeJumpAttr(mth);
 		removeInsns(mth);
 		removeEmptyDetachedBlocks(mth);
 		removeUnreachableBlocks(mth);
-		initBlocksInTargetNodes(mth);
+		mth.getBasicBlocks().removeIf(BlockSplitter::removeEmptyBlock);
 
-		removeJumpAttributes(mth.getInstructions());
 		mth.unloadInsnArr();
 	}
 
@@ -307,7 +308,7 @@ private static BlockNode getBlock(int offset, Map<Integer, BlockNode> blocksMap)
 		return block;
 	}
 
-	private void removeJumpAttr(MethodNode mth) {
+	private static void removeJumpAttr(MethodNode mth) {
 		for (BlockNode block : mth.getBasicBlocks()) {
 			for (InsnNode insn : block.getInstructions()) {
 				insn.remove(AType.JUMP);
@@ -333,44 +334,88 @@ static boolean removeEmptyDetachedBlocks(MethodNode mth) {
 				&& block.getSuccessors().isEmpty());
 	}
 
-	private void removeJumpAttributes(InsnNode[] insnArr) {
-		for (InsnNode insn : insnArr) {
-			if (insn != null && insn.contains(AType.JUMP)) {
-				insn.remove(AType.JUMP);
-			}
-		}
-	}
-
-	private void removeUnreachableBlocks(MethodNode mth) {
+	private static boolean removeUnreachableBlocks(MethodNode mth) {
 		Set<BlockNode> toRemove = new LinkedHashSet<>();
 		for (BlockNode block : mth.getBasicBlocks()) {
 			if (block.getPredecessors().isEmpty() && block != mth.getEnterBlock()) {
-				toRemove.add(block);
 				collectSuccessors(block, toRemove);
 			}
 		}
-		if (!toRemove.isEmpty()) {
-			mth.getBasicBlocks().removeIf(toRemove::contains);
+		if (toRemove.isEmpty()) {
+			return false;
+		}
 
+		toRemove.forEach(BlockSplitter::detachBlock);
+		mth.getBasicBlocks().removeAll(toRemove);
+		long notEmptyBlocks = toRemove.stream().filter(block -> !block.getInstructions().isEmpty()).count();
+		if (notEmptyBlocks != 0) {
 			int insnsCount = toRemove.stream().mapToInt(block -> block.getInstructions().size()).sum();
-			mth.addAttr(AType.COMMENTS, "JADX INFO: unreachable blocks removed: " + toRemove.size()
+			mth.addAttr(AType.COMMENTS, "JADX INFO: unreachable blocks removed: " + notEmptyBlocks
 					+ ", instructions: " + insnsCount);
 		}
+		return true;
 	}
 
-	private void collectSuccessors(BlockNode startBlock, Set<BlockNode> toRemove) {
+	static boolean removeEmptyBlock(BlockNode block) {
+		if (canRemoveBlock(block)) {
+			if (block.getSuccessors().size() == 1) {
+				BlockNode successor = block.getSuccessors().get(0);
+				block.getPredecessors().forEach(pred -> {
+					pred.getSuccessors().remove(block);
+					BlockSplitter.connect(pred, successor);
+					BlockSplitter.replaceTarget(pred, block, successor);
+					pred.updateCleanSuccessors();
+				});
+				BlockSplitter.removeConnection(block, successor);
+			} else {
+				block.getPredecessors().forEach(pred -> {
+					pred.getSuccessors().remove(block);
+					pred.updateCleanSuccessors();
+				});
+			}
+			block.add(AFlag.REMOVE);
+			block.getSuccessors().clear();
+			block.getPredecessors().clear();
+			return true;
+		}
+		return false;
+	}
+
+	private static boolean canRemoveBlock(BlockNode block) {
+		return block.getInstructions().isEmpty()
+				&& block.isAttrStorageEmpty()
+				&& block.getSuccessors().size() <= 1
+				&& !block.getPredecessors().isEmpty();
+	}
+
+	private static void collectSuccessors(BlockNode startBlock, Set<BlockNode> toRemove) {
 		Deque<BlockNode> stack = new ArrayDeque<>();
 		stack.add(startBlock);
 		while (!stack.isEmpty()) {
 			BlockNode block = stack.pop();
 			if (!toRemove.contains(block)) {
+				toRemove.add(block);
 				for (BlockNode successor : block.getSuccessors()) {
 					if (toRemove.containsAll(successor.getPredecessors())) {
 						stack.push(successor);
 					}
 				}
 			}
-			toRemove.add(block);
+
+		}
+	}
+
+	static void detachBlock(BlockNode block) {
+		for (BlockNode pred : block.getPredecessors()) {
+			pred.getSuccessors().remove(block);
+			pred.updateCleanSuccessors();
+		}
+		for (BlockNode successor : block.getSuccessors()) {
+			successor.getPredecessors().remove(block);
 		}
+		block.add(AFlag.REMOVE);
+		block.getInstructions().clear();
+		block.getPredecessors().clear();
+		block.getSuccessors().clear();
 	}
 }

jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessTryCatchRegions.java

@@ -7,6 +7,7 @@
 import java.util.Map;
 import java.util.Set;
 
+import jadx.core.dex.attributes.AFlag;
 import jadx.core.dex.attributes.AType;
 import jadx.core.dex.nodes.BlockNode;
 import jadx.core.dex.nodes.IBranchRegion;
@@ -167,6 +168,7 @@ private static boolean isHandlerPath(TryCatchBlock tb, IContainer cont) {
 		for (ExceptionHandler h : tb.getHandlers()) {
 			BlockNode handlerBlock = h.getHandlerBlock();
 			if (handlerBlock != null
+					&& !handlerBlock.contains(AFlag.REMOVE)
 					&& RegionUtils.hasPathThroughBlock(handlerBlock, cont)) {
 				return true;
 			}

jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMaker.java

@@ -1022,6 +1022,9 @@ private void processExcHandler(MethodNode mth, ExceptionHandler handler, Set<Blo
 			dom = start;
 			stack.addExits(exits);
 		}
+		if (dom.contains(AFlag.REMOVE)) {
+			return;
+		}
 		BitSet domFrontier = dom.getDomFrontier();
 		List<BlockNode> handlerExits = BlockUtils.bitSetToBlocks(this.mth, domFrontier);
 		boolean inLoop = this.mth.getLoopForBlock(start) != null;

jadx-core/src/main/java/jadx/core/utils/BlockUtils.java

@@ -270,6 +270,9 @@ public static BitSet blocksToBitSet(MethodNode mth, List<BlockNode> blocks) {
 	}
 
 	public static List<BlockNode> bitSetToBlocks(MethodNode mth, BitSet bs) {
+		if (bs == null) {
+			return Collections.emptyList();
+		}
 		int size = bs.cardinality();
 		if (size == 0) {
 			return Collections.emptyList();

jadx-core/src/test/java/jadx/tests/integration/others/TestMultipleNOPs.java

@@ -0,0 +1,16 @@
+package jadx.tests.integration.others;
+
+import org.junit.jupiter.api.Test;
+
+import jadx.tests.api.SmaliTest;
+
+public class TestMultipleNOPs extends SmaliTest {
+
+	@Test
+	public void test() {
+		disableCompilation();
+
+		// expected no errors
+		loadFromSmaliFiles();
+	}
+}