From d20cd43a99e535107acb6ca024f70bfcfb2e9516 Mon Sep 17 00:00:00 2001
From: Skylot <skylot@gmail.com>
Date: Sat, 2 Jun 2018 21:16:58 +0300
Subject: [PATCH] core: fix loop handling

---
 .../visitors/blocksmaker/BlockProcessor.java  | 24 +++++++---
 .../integration/loops/TestLoopCondition5.java | 45 +++++++++++++++++++
 .../test/smali/loops/TestLoopCondition5.smali | 31 +++++++++++++
 3 files changed, 94 insertions(+), 6 deletions(-)
 create mode 100644 jadx-core/src/test/java/jadx/tests/integration/loops/TestLoopCondition5.java
 create mode 100644 jadx-core/src/test/smali/loops/TestLoopCondition5.smali

diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/blocksmaker/BlockProcessor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/blocksmaker/BlockProcessor.java
index 121455232..053d02047 100644
--- a/jadx-core/src/main/java/jadx/core/dex/visitors/blocksmaker/BlockProcessor.java
+++ b/jadx-core/src/main/java/jadx/core/dex/visitors/blocksmaker/BlockProcessor.java
@@ -21,6 +21,7 @@ import jadx.core.dex.nodes.MethodNode;
 import jadx.core.dex.trycatch.CatchAttr;
 import jadx.core.dex.visitors.AbstractVisitor;
 import jadx.core.utils.BlockUtils;
+import jadx.core.utils.exceptions.JadxOverflowException;
 import jadx.core.utils.exceptions.JadxRuntimeException;
 
 import static jadx.core.dex.visitors.blocksmaker.BlockSplitter.connect;
@@ -68,11 +69,16 @@ public class BlockProcessor extends AbstractVisitor {
 		processNestedLoops(mth);
 	}
 
-	private static boolean removeEmptyBlock(MethodNode mth, BlockNode block) {
-		if (block.getInstructions().isEmpty()
+	private static boolean canRemoveBlock(BlockNode block) {
+		return block.getInstructions().isEmpty()
 				&& !block.isSynthetic()
 				&& block.isAttrStorageEmpty()
-				&& block.getSuccessors().size() <= 1) {
+				&& block.getSuccessors().size() <= 1
+				&& !block.getPredecessors().isEmpty();
+	}
+
+	private static boolean removeEmptyBlock(BlockNode block) {
+		if (canRemoveBlock(block)) {
 			LOG.debug("Removing empty block: {}", block);
 			if (block.getSuccessors().size() == 1) {
 				BlockNode successor = block.getSuccessors().get(0);
@@ -245,7 +251,13 @@ public class BlockProcessor extends AbstractVisitor {
 			exit.setDomFrontier(EMPTY);
 		}
 		for (BlockNode block : mth.getBasicBlocks()) {
-			computeBlockDF(mth, block);
+			try {
+				computeBlockDF(mth, block);
+			} catch (StackOverflowError e) {
+				throw new JadxOverflowException("Failed compute block dominance frontier");
+			} catch (Exception e) {
+				throw new JadxRuntimeException("Failed compute block dominance frontier", e);
+			}
 		}
 	}
 
@@ -253,7 +265,7 @@ public class BlockProcessor extends AbstractVisitor {
 		if (block.getDomFrontier() != null) {
 			return;
 		}
-		block.getDominatesOn().forEach(c -> computeBlockDF(mth, c));
+		block.getDominatesOn().forEach(domBlock -> computeBlockDF(mth, domBlock));
 		List<BlockNode> blocks = mth.getBasicBlocks();
 		BitSet domFrontier = null;
 		for (BlockNode s : block.getSuccessors()) {
@@ -367,7 +379,7 @@ public class BlockProcessor extends AbstractVisitor {
 			}
 		}
 		for (BlockNode basicBlock : basicBlocks) {
-			if (removeEmptyBlock(mth, basicBlock)) {
+			if (removeEmptyBlock(basicBlock)) {
 				changed = true;
 			}
 		}
diff --git a/jadx-core/src/test/java/jadx/tests/integration/loops/TestLoopCondition5.java b/jadx-core/src/test/java/jadx/tests/integration/loops/TestLoopCondition5.java
new file mode 100644
index 000000000..5f3a97354
--- /dev/null
+++ b/jadx-core/src/test/java/jadx/tests/integration/loops/TestLoopCondition5.java
@@ -0,0 +1,45 @@
+package jadx.tests.integration.loops;
+
+import org.junit.Test;
+
+import jadx.core.dex.nodes.ClassNode;
+import jadx.tests.api.SmaliTest;
+
+import static jadx.tests.api.utils.JadxMatchers.containsOne;
+import static jadx.tests.api.utils.JadxMatchers.countString;
+import static org.hamcrest.Matchers.anyOf;
+import static org.junit.Assert.assertThat;
+
+public class TestLoopCondition5 extends SmaliTest {
+
+	public static class TestCls {
+		private static int lastIndexOf(int[] array, int target, int start, int end) {
+			for (int i = end - 1; i >= start; i--) {
+				if (array[i] == target) {
+					return i;
+				}
+			}
+			return -1;
+		}
+	}
+
+	@Test
+	public void test0() {
+		ClassNode cls = getClassNode(TestCls.class);
+		String code = cls.getCode().toString();
+
+		assertThat(code, containsOne("for ("));
+		assertThat(code, containsOne("return -1;"));
+		assertThat(code, countString(2, "return "));
+	}
+
+	@Test
+	public void test1() {
+		ClassNode cls = getClassNodeFromSmaliWithPath("loops", "TestLoopCondition5");
+		String code = cls.getCode().toString();
+
+		assertThat(code, anyOf(containsOne("for ("), containsOne("while (true) {")));
+		assertThat(code, containsOne("return -1;"));
+		assertThat(code, countString(2, "return "));
+	}
+}
diff --git a/jadx-core/src/test/smali/loops/TestLoopCondition5.smali b/jadx-core/src/test/smali/loops/TestLoopCondition5.smali
new file mode 100644
index 000000000..954d4dc7b
--- /dev/null
+++ b/jadx-core/src/test/smali/loops/TestLoopCondition5.smali
@@ -0,0 +1,31 @@
+.class public LTestLoopCondition5;
+.super Ljava/lang/Object;
+.source "TestLoopCondition5.java"
+
+.method private static lastIndexOf([IIII)I
+    .locals 1
+
+    add-int/lit8 p3, p3, -0x1
+
+    :goto_0
+    const/4 v0, -0x1
+
+    if-lt p3, p2, :cond_1
+
+    .line 219
+    aget v0, p0, p3
+
+    if-ne v0, p1, :cond_0
+
+    return p3
+
+    :cond_0
+    add-int/lit8 p3, p3, -0x1
+
+    goto :goto_0
+
+    :cond_1
+    move p3, v0
+
+    return p3
+.end method