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 @@ public class InsnDecoder {
 
 	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 @@ public class InsnDecoder {
 	}
 
 	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 @@ public class InsnDecoder {
 	}
 
 	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 @@ public class InsnDecoder {
 		return -1;
 	}
 
-	public static int getPrevInsnOffset(Object[] insnArr, int offset) {
-		int i = offset - 1;
-		while (i >= 0 && insnArr[i] == null) {
-			i--;
+	private static DecodedInstruction getInsnByOffsetSkipNop(DecodedInstruction[] insnArr, int offset) {
+		DecodedInstruction payload = insnArr[offset];
+		if (payload.getOpcode() == Opcodes.NOP) {
+			return insnArr[getNextInsnOffsetSkipNop(insnArr, offset)];
 		}
-		if (i < 0) {
-			return -1;
-		}
-		return i;
+		return payload;
 	}
 
-	public static int getNextInsnOffset(Object[] insnArr, int offset) {
+	public static int getNextInsnOffset(DecodedInstruction[] insnArr, int offset) {
+		return getNextInsnOffset(insnArr, offset, null);
+	}
+
+	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 class SwitchNode extends TargetInsnNode {
 
 	@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 @@ public class BlockProcessor extends AbstractVisitor {
 		});
 	}
 
-	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 @@ public class BlockProcessor extends AbstractVisitor {
 			}
 		}
 		for (BlockNode basicBlock : basicBlocks) {
-			if (removeEmptyBlock(basicBlock)) {
+			if (BlockSplitter.removeEmptyBlock(basicBlock)) {
 				changed = true;
 			}
 		}
@@ -631,7 +599,7 @@ public class BlockProcessor extends AbstractVisitor {
 			ExceptionHandler excHandler = otherExcHandlerAttr.getHandler();
 			excHandlerAttr.getHandler().addCatchTypes(excHandler.getCatchTypes());
 			tryBlock.removeHandler(mth, excHandler);
-			detachBlock(block);
+			BlockSplitter.detachBlock(block);
 		}
 		return true;
 	}
@@ -734,20 +702,6 @@ public class BlockProcessor extends AbstractVisitor {
 		});
 	}
 
-	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 class BlockSplitter extends AbstractVisitor {
 
 		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 @@ public class BlockSplitter extends AbstractVisitor {
 		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 @@ public class BlockSplitter extends AbstractVisitor {
 				&& 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.List;
 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 @@ public class ProcessTryCatchRegions extends AbstractRegionVisitor {
 		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 @@ public class RegionMaker {
 			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 class BlockUtils {
 	}
 
 	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();
+	}
+}

jadx-core/src/test/smali/others/TestMultipleNOPs/test.smali

@@ -0,0 +1,226 @@
+.class final LseC/dujmehn/Cutyq/e;
+.super Ljava/lang/Object;
+
+# interfaces
+.implements Ljava/lang/Runnable;
+
+
+.method public static test()Ljava/lang/String;
+    .registers 9
+
+    nop
+
+    nop
+
+    nop
+
+    nop
+
+    .prologue
+    nop
+
+    const-string v5, "VA=="
+
+    nop
+
+    nop
+
+    .local v5, "x_gfxj":Ljava/lang/String;
+    nop
+
+    const-string v1, "54b6610e1af242f78e38a5866eaa7c41"
+
+    nop
+
+    nop
+
+    .local v1, "p":Ljava/lang/String;
+    nop
+
+    invoke-virtual {v5}, Ljava/lang/String;->getBytes()[B
+
+    nop
+
+    nop
+
+    move-result-object v7
+
+    nop
+
+    nop
+
+    const/4 v8, 0x0
+
+    nop
+
+    nop
+
+    invoke-static {v7, v8}, Landroid/util/Base64;->decode([BI)[B
+
+    nop
+
+    nop
+
+    move-result-object v3
+
+    nop
+
+    nop
+
+    .local v3, "wjxzqy":[B
+    nop
+
+    new-instance v4, Ljava/lang/String;
+
+    nop
+
+    nop
+
+    invoke-direct {v4, v3}, Ljava/lang/String;-><init>([B)V
+
+    nop
+
+    nop
+
+    .local v4, "x":Ljava/lang/String;
+    nop
+
+    new-instance v6, Ljava/lang/StringBuilder;
+
+    nop
+
+    nop
+
+    invoke-direct {v6}, Ljava/lang/StringBuilder;-><init>()V
+
+    nop
+
+    nop
+
+    .local v6, "xg":Ljava/lang/StringBuilder;
+    nop
+
+    const/4 v0, 0x0
+
+    nop
+
+    nop
+
+    .local v0, "n":I
+    nop
+
+    :goto_3b
+    nop
+
+    invoke-virtual {v4}, Ljava/lang/String;->length()I
+
+    nop
+
+    nop
+
+    move-result v7
+
+    nop
+
+    nop
+
+    if-ge v0, v7, :cond_76
+
+    nop
+
+    nop
+
+    invoke-virtual {v4, v0}, Ljava/lang/String;->charAt(I)C
+
+    nop
+
+    nop
+
+    move-result v7
+
+    nop
+
+    nop
+
+    invoke-virtual {v1}, Ljava/lang/String;->length()I
+
+    nop
+
+    nop
+
+    move-result v8
+
+    nop
+
+    nop
+
+    rem-int v8, v0, v8
+
+    nop
+
+    nop
+
+    invoke-virtual {v1, v8}, Ljava/lang/String;->charAt(I)C
+
+    nop
+
+    nop
+
+    move-result v8
+
+    nop
+
+    nop
+
+    xor-int/2addr v7, v8
+
+    nop
+
+    nop
+
+    int-to-char v7, v7
+
+    nop
+
+    nop
+
+    invoke-virtual {v6, v7}, Ljava/lang/StringBuilder;->append(C)Ljava/lang/StringBuilder;
+
+    nop
+
+    nop
+
+    add-int/lit8 v0, v0, 0x1
+
+    nop
+
+    nop
+
+    goto :goto_3b
+
+    nop
+
+    nop
+
+    :cond_76
+    nop
+
+    invoke-virtual {v6}, Ljava/lang/StringBuilder;->toString()Ljava/lang/String;
+
+    nop
+
+    nop
+
+    move-result-object v2
+
+    nop
+
+    nop
+
+    .local v2, "w":Ljava/lang/String;
+    nop
+
+    return-object v2
+
+    nop
+.end method