From 3474f0da04f241e4dbc26a46799b1dc3ecce06e8 Mon Sep 17 00:00:00 2001 From: Skylot Date: Sat, 13 May 2023 21:29:20 +0100 Subject: [PATCH] fix: split loop exit at loop end (#1866) Loop exit (like condition) in loop end block can confuse loop restructure. To resolve this try to split back edge and make loop end a simple path. --- .../java/jadx/core/codegen/MethodGen.java | 2 +- .../dex/visitors/blocks/BlockProcessor.java | 18 +++++++++++- .../dex/visitors/regions/RegionMaker.java | 15 ++++++++-- .../integration/loops/TestDoWhileBreak2.java | 2 +- .../loops/TestIterableForEach4.java | 29 +++++++++++++++++++ .../integration/loops/TestLoopCondition5.java | 5 +++- .../integration/loops/TestNestedLoops5.java | 14 +++------ .../trycatch/TestLoopInTryCatch.java | 16 ++++++++++ 8 files changed, 84 insertions(+), 17 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/loops/TestIterableForEach4.java diff --git a/jadx-core/src/main/java/jadx/core/codegen/MethodGen.java b/jadx-core/src/main/java/jadx/core/codegen/MethodGen.java index 4b6810c6a..2c5ae3399 100644 --- a/jadx-core/src/main/java/jadx/core/codegen/MethodGen.java +++ b/jadx-core/src/main/java/jadx/core/codegen/MethodGen.java @@ -553,7 +553,7 @@ public class MethodGen { } public static String getLabelName(BlockNode block) { - return String.format("L%d", block.getId()); + return String.format("L%d", block.getCId()); } public static String getLabelName(IfNode insn) { diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockProcessor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockProcessor.java index b50a25df8..75f1d27c5 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockProcessor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockProcessor.java @@ -329,6 +329,21 @@ public class BlockProcessor extends AbstractVisitor { return changed; } + private static boolean simplifyLoopEnd(MethodNode mth, LoopInfo loop) { + BlockNode loopEnd = loop.getEnd(); + if (loopEnd.getSuccessors().size() > 1) { + // make loop end a simple path block + BlockNode newLoopEnd = BlockSplitter.startNewBlock(mth, -1); + newLoopEnd.add(AFlag.SYNTHETIC); + newLoopEnd.add(AFlag.LOOP_END); + BlockNode loopStart = loop.getStart(); + BlockSplitter.replaceConnection(loopEnd, loopStart, newLoopEnd); + BlockSplitter.connect(newLoopEnd, loopStart); + return true; + } + return false; + } + private static boolean checkLoops(MethodNode mth, BlockNode block) { if (!block.contains(AFlag.LOOP_START)) { return false; @@ -350,7 +365,8 @@ public class BlockProcessor extends AbstractVisitor { LoopInfo loop = loops.get(0); return insertBlocksForContinue(mth, loop) || insertBlockForPredecessors(mth, loop) - || insertPreHeader(mth, loop); + || insertPreHeader(mth, loop) + || simplifyLoopEnd(mth, loop); } return false; } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMaker.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMaker.java index 819b733b8..0ef9d2169 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMaker.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMaker.java @@ -44,6 +44,7 @@ import jadx.core.dex.trycatch.ExcHandlerAttr; import jadx.core.dex.trycatch.ExceptionHandler; import jadx.core.dex.trycatch.TryCatchBlockAttr; import jadx.core.utils.BlockUtils; +import jadx.core.utils.ListUtils; import jadx.core.utils.RegionUtils; import jadx.core.utils.Utils; import jadx.core.utils.exceptions.JadxOverflowException; @@ -217,6 +218,8 @@ public class RegionMaker { condInfo = IfInfo.invert(condInfo); } loopRegion.updateCondition(condInfo); + // prevent if's merge with loop condition + condInfo.getMergedBlocks().forEach(b -> b.add(AFlag.ADDED_TO_REGION)); exitBlocks.removeAll(condInfo.getMergedBlocks()); if (!exitBlocks.isEmpty()) { @@ -234,7 +237,8 @@ public class RegionMaker { BlockNode out; if (loopRegion.isConditionAtEnd()) { BlockNode thenBlock = condInfo.getThenBlock(); - out = thenBlock == loopStart ? condInfo.getElseBlock() : thenBlock; + out = (thenBlock == loop.getEnd() || thenBlock == loopStart) ? condInfo.getElseBlock() : thenBlock; + out = BlockUtils.followEmptyPath(out); loopStart.remove(AType.LOOP); loop.getEnd().add(AFlag.ADDED_TO_REGION); stack.addExit(loop.getEnd()); @@ -246,6 +250,7 @@ public class RegionMaker { } else { out = condInfo.getElseBlock(); if (outerRegion != null + && out != null && out.contains(AFlag.LOOP_START) && !out.getAll(AType.LOOP).contains(loop) && RegionUtils.isRegionContainsBlock(outerRegion, out)) { @@ -298,9 +303,13 @@ public class RegionMaker { // skip nested loop condition continue; } - LoopRegion loopRegion = new LoopRegion(curRegion, loop, block, block == loop.getEnd()); + BlockNode loopEnd = loop.getEnd(); + boolean exitAtLoopEnd = block == loopEnd + || (loopEnd.getInstructions().isEmpty() && ListUtils.isSingleElement(loopEnd.getPredecessors(), block)); + + LoopRegion loopRegion = new LoopRegion(curRegion, loop, block, exitAtLoopEnd); boolean found; - if (block == loop.getStart() || block == loop.getEnd() + if (block == loop.getStart() || exitAtLoopEnd || BlockUtils.isEmptySimplePath(loop.getStart(), block)) { found = true; } else if (block.getPredecessors().contains(loop.getStart())) { diff --git a/jadx-core/src/test/java/jadx/tests/integration/loops/TestDoWhileBreak2.java b/jadx-core/src/test/java/jadx/tests/integration/loops/TestDoWhileBreak2.java index f1d302630..25645cb76 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/loops/TestDoWhileBreak2.java +++ b/jadx-core/src/test/java/jadx/tests/integration/loops/TestDoWhileBreak2.java @@ -20,7 +20,7 @@ public class TestDoWhileBreak2 extends IntegrationTest { do { obj = this.it.next(); if (obj == null) { - return obj; // 'return null' works + return obj; // 'return null' or 'break' also fine } } while (this.it.hasNext()); return obj; diff --git a/jadx-core/src/test/java/jadx/tests/integration/loops/TestIterableForEach4.java b/jadx-core/src/test/java/jadx/tests/integration/loops/TestIterableForEach4.java new file mode 100644 index 000000000..0f10e3eaa --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/loops/TestIterableForEach4.java @@ -0,0 +1,29 @@ +package jadx.tests.integration.loops; + +import java.util.List; + +import org.junit.jupiter.api.Test; + +import jadx.tests.api.IntegrationTest; + +import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat; + +public class TestIterableForEach4 extends IntegrationTest { + + public static class TestCls { + public void test(List objects) { + for (Object o : objects) { + if (o.hashCode() != 42 || o.hashCode() != 1) { + break; + } + } + } + } + + @Test + public void test() { + assertThat(getClassNode(TestCls.class)) + .code() + .doesNotContain("while ("); + } +} 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 index 3f5608556..b63661a0f 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/loops/TestLoopCondition5.java +++ b/jadx-core/src/test/java/jadx/tests/integration/loops/TestLoopCondition5.java @@ -38,7 +38,10 @@ public class TestLoopCondition5 extends SmaliTest { ClassNode cls = getClassNodeFromSmaliWithPath("loops", "TestLoopCondition5"); String code = cls.getCode().toString(); - assertThat(code, anyOf(containsOne("for ("), containsOne("while (true) {"))); + assertThat(code, anyOf( + containsOne("for ("), + containsOne("while (true) {"), + containsOne("} while (iArr[i3] != i);"))); assertThat(code, containsOne("return -1;")); assertThat(code, countString(2, "return ")); } diff --git a/jadx-core/src/test/java/jadx/tests/integration/loops/TestNestedLoops5.java b/jadx-core/src/test/java/jadx/tests/integration/loops/TestNestedLoops5.java index ae8cb62b1..18162671b 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/loops/TestNestedLoops5.java +++ b/jadx-core/src/test/java/jadx/tests/integration/loops/TestNestedLoops5.java @@ -2,13 +2,9 @@ package jadx.tests.integration.loops; import org.junit.jupiter.api.Test; -import jadx.NotYetImplemented; -import jadx.core.dex.nodes.ClassNode; import jadx.tests.api.IntegrationTest; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.not; +import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; public class TestNestedLoops5 extends IntegrationTest { @@ -36,11 +32,9 @@ public class TestNestedLoops5 extends IntegrationTest { } @Test - @NotYetImplemented public void test() { - ClassNode cls = getClassNode(TestCls.class); - String code = cls.getCode().toString(); - - assertThat(code, not(containsString("continue;"))); + assertThat(getClassNode(TestCls.class)) + .code() + .doesNotContain("continue;"); } } diff --git a/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestLoopInTryCatch.java b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestLoopInTryCatch.java index 1568b62f2..c112f9472 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestLoopInTryCatch.java +++ b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestLoopInTryCatch.java @@ -40,6 +40,22 @@ public class TestLoopInTryCatch extends SmaliTest { "}", "if (i != 1) {", " getI();", + "}"), + // TODO: weird result but correct, better to not use do-while if not really needed + c -> c.containsLines(2, + "int i;", + "do {", + " try {", + " i = getI();", + " if (i == 1) {", + " break;", + " }", + " } catch (RuntimeException unused) {", + " return;", + " }", + "} while (i != 2);", + "if (i != 1) {", + " getI();", "}")); } }