From 6e66dc25c827ffb39594ae46f3afce7defd824cc Mon Sep 17 00:00:00 2001 From: Skylot Date: Sat, 23 Mar 2019 22:36:49 +0300 Subject: [PATCH] fix: additional checks for loop exit edges and 'for' conversion (#483) --- .../dex/visitors/regions/IfRegionVisitor.java | 8 +- .../dex/visitors/regions/RegionMaker.java | 41 ++++++++-- .../visitors/regions/RegionMakerVisitor.java | 7 ++ .../java/jadx/core/utils/RegionUtils.java | 60 ++++++++++++++- .../integration/loops/TestIndexedLoop.java | 15 ++-- .../integration/loops/TestNotIndexedLoop.java | 77 +++++++++++++++++++ 6 files changed, 184 insertions(+), 24 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/loops/TestNotIndexedLoop.java diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java index 8429ad7f8..34cc99d5b 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java @@ -140,7 +140,7 @@ public class IfRegionVisitor extends AbstractVisitor { || ifRegion.getElseRegion().contains(AFlag.ELSE_IF_CHAIN)) { return false; } - if (!hasBranchTerminator(ifRegion.getThenRegion())) { + if (!RegionUtils.hasExitBlock(ifRegion.getThenRegion())) { return false; } // code style check: @@ -162,12 +162,6 @@ public class IfRegionVisitor extends AbstractVisitor { return false; } - private static boolean hasBranchTerminator(IContainer region) { - // TODO: check for exception throw - return RegionUtils.hasExitBlock(region) - || RegionUtils.hasBreakInsn(region); - } - private static void invertIfRegion(IfRegion ifRegion) { IContainer elseRegion = ifRegion.getElseRegion(); if (elseRegion != null) { 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 c5aa7498a..abd353695 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 @@ -7,6 +7,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Optional; import java.util.Set; import org.slf4j.Logger; @@ -192,7 +193,7 @@ public class RegionMaker { // add 'break' instruction before path cross between main loop exit and sub-exit for (Edge exitEdge : loop.getExitEdges()) { if (exitBlocks.contains(exitEdge.getSource())) { - insertBreak(stack, loopExit, exitEdge); + insertLoopBreak(stack, loop, loopExit, exitEdge); } } } @@ -288,6 +289,9 @@ public class RegionMaker { } } } + if (found && !checkLoopExits(loop, block)) { + found = false; + } if (found) { return loopRegion; } @@ -296,6 +300,32 @@ public class RegionMaker { return null; } + private boolean checkLoopExits(LoopInfo loop, BlockNode mainExitBlock) { + List exitEdges = loop.getExitEdges(); + if (exitEdges.size() < 2) { + return true; + } + Optional mainEdgeOpt = exitEdges.stream().filter(edge -> edge.getSource() == mainExitBlock).findFirst(); + if (!mainEdgeOpt.isPresent()) { + throw new JadxRuntimeException("Not found exit edge by exit block: " + mainExitBlock); + } + Edge mainExitEdge = mainEdgeOpt.get(); + BlockNode mainOutBlock = skipSyntheticSuccessor(mainExitEdge.getTarget()); + for (Edge exitEdge : exitEdges) { + if (exitEdge != mainExitEdge) { + BlockNode outBlock = skipSyntheticSuccessor(exitEdge.getTarget()); + // all exit paths must be same or don't cross (will be inside loop) + if (!isEqualPaths(mainOutBlock, outBlock)) { + BlockNode crossBlock = BlockUtils.getPathCross(mth, mainOutBlock, outBlock); + if (crossBlock != null) { + return false; + } + } + } + } + return true; + } + private BlockNode makeEndlessLoop(IRegion curRegion, RegionStack stack, LoopInfo loop, BlockNode loopStart) { LoopRegion loopRegion = new LoopRegion(curRegion, loop, null, false); curRegion.getSubBlocks().add(loopRegion); @@ -310,7 +340,7 @@ public class RegionMaker { if (exitEdges.size() == 1) { Edge exitEdge = exitEdges.get(0); BlockNode exit = exitEdge.getTarget(); - if (insertBreak(stack, exit, exitEdge)) { + if (insertLoopBreak(stack, loop, exit, exitEdge)) { BlockNode nextBlock = getNextBlock(exit); if (nextBlock != null) { stack.addExit(nextBlock); @@ -324,10 +354,10 @@ public class RegionMaker { for (BlockNode block : blocks) { if (BlockUtils.isPathExists(exit, block)) { stack.addExit(block); - insertBreak(stack, block, exitEdge); + insertLoopBreak(stack, loop, block, exitEdge); out = block; } else { - insertBreak(stack, exit, exitEdge); + insertLoopBreak(stack, loop, exit, exitEdge); } } } @@ -386,7 +416,7 @@ public class RegionMaker { return true; } - private boolean insertBreak(RegionStack stack, BlockNode loopExit, Edge exitEdge) { + private boolean insertLoopBreak(RegionStack stack, LoopInfo loop, BlockNode loopExit, Edge exitEdge) { BlockNode exit = exitEdge.getTarget(); BlockNode insertBlock = null; boolean confirm = false; @@ -425,6 +455,7 @@ public class RegionMaker { return false; } InsnNode breakInsn = new InsnNode(InsnType.BREAK, 0); + breakInsn.addAttr(AType.LOOP, loop); EdgeInsnAttr.addEdgeInsn(insertBlock, insertBlock.getSuccessors().get(0), breakInsn); stack.addExit(exit); // add label to 'break' if needed diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMakerVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMakerVisitor.java index 09f7987e9..ffea7cd43 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMakerVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMakerVisitor.java @@ -104,6 +104,13 @@ public class RegionMakerVisitor extends AbstractVisitor { if (!insnAttr.getStart().equals(last)) { return; } + if (last instanceof BlockNode) { + BlockNode block = (BlockNode) last; + if (block.getInstructions().isEmpty()) { + block.getInstructions().add(insnAttr.getInsn()); + return; + } + } List insns = Collections.singletonList(insnAttr.getInsn()); region.add(new InsnContainer(insns)); } diff --git a/jadx-core/src/main/java/jadx/core/utils/RegionUtils.java b/jadx-core/src/main/java/jadx/core/utils/RegionUtils.java index 5ded8fe65..014fbc11b 100644 --- a/jadx-core/src/main/java/jadx/core/utils/RegionUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/RegionUtils.java @@ -8,6 +8,9 @@ import java.util.Set; import org.jetbrains.annotations.Nullable; import jadx.core.dex.attributes.AType; +import jadx.core.dex.attributes.AttrList; +import jadx.core.dex.attributes.nodes.LoopInfo; +import jadx.core.dex.attributes.nodes.LoopLabelAttr; import jadx.core.dex.instructions.InsnType; import jadx.core.dex.nodes.BlockNode; import jadx.core.dex.nodes.IBlock; @@ -91,22 +94,71 @@ public class RegionUtils { } /** - * Return true if last block in region has no successors + * Return true if last block in region has no successors or jump out insn (return or break) */ public static boolean hasExitBlock(IContainer container) { + return hasExitBlock(container, container); + } + + private static boolean hasExitBlock(IContainer rootContainer, IContainer container) { if (container instanceof BlockNode) { - return ((BlockNode) container).getSuccessors().isEmpty(); + BlockNode blockNode = (BlockNode) container; + if (blockNode.getSuccessors().isEmpty()) { + return true; + } + return isInsnExitContainer(rootContainer, (IBlock) container); + } else if (container instanceof IBranchRegion) { + return false; } else if (container instanceof IBlock) { - return true; + return isInsnExitContainer(rootContainer, (IBlock) container); } else if (container instanceof IRegion) { List blocks = ((IRegion) container).getSubBlocks(); return !blocks.isEmpty() - && hasExitBlock(blocks.get(blocks.size() - 1)); + && hasExitBlock(rootContainer, blocks.get(blocks.size() - 1)); } else { throw new JadxRuntimeException(unknownContainerType(container)); } } + private static boolean isInsnExitContainer(IContainer rootContainer, IBlock block) { + InsnNode lastInsn = BlockUtils.getLastInsn(block); + if (lastInsn == null) { + return false; + } + InsnType insnType = lastInsn.getType(); + if (insnType == InsnType.RETURN) { + return true; + } + if (insnType == InsnType.THROW) { + // check if after throw execution can continue in current container + CatchAttr catchAttr = lastInsn.get(AType.CATCH_BLOCK); + if (catchAttr != null) { + for (ExceptionHandler handler : catchAttr.getTryBlock().getHandlers()) { + if (RegionUtils.isRegionContainsBlock(rootContainer, handler.getHandlerBlock())) { + return false; + } + } + } + return true; + } + if (insnType == InsnType.BREAK) { + AttrList loopInfoAttrList = lastInsn.get(AType.LOOP); + if (loopInfoAttrList != null) { + for (LoopInfo loopInfo : loopInfoAttrList.getList()) { + if (!RegionUtils.isRegionContainsBlock(rootContainer, loopInfo.getStart())) { + return true; + } + } + } + LoopLabelAttr loopLabelAttr = lastInsn.get(AType.LOOP_LABEL); + if (loopLabelAttr != null + && !RegionUtils.isRegionContainsBlock(rootContainer, loopLabelAttr.getLoop().getStart())) { + return true; + } + } + return false; + } + public static boolean hasBreakInsn(IContainer container) { if (container instanceof IBlock) { return BlockUtils.checkLastInsnType((IBlock) container, InsnType.BREAK); diff --git a/jadx-core/src/test/java/jadx/tests/integration/loops/TestIndexedLoop.java b/jadx-core/src/test/java/jadx/tests/integration/loops/TestIndexedLoop.java index 3b9181d34..a73bbaf2d 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/loops/TestIndexedLoop.java +++ b/jadx-core/src/test/java/jadx/tests/integration/loops/TestIndexedLoop.java @@ -1,5 +1,12 @@ package jadx.tests.integration.loops; +import java.io.File; + +import org.junit.jupiter.api.Test; + +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.IntegrationTest; + import static jadx.tests.api.utils.JadxMatchers.containsOne; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; @@ -7,14 +14,6 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; -import java.io.File; - -import org.junit.Test; - -import jadx.core.dex.nodes.ClassNode; -import jadx.tests.api.IntegrationTest; - - public class TestIndexedLoop extends IntegrationTest { public static class TestCls { diff --git a/jadx-core/src/test/java/jadx/tests/integration/loops/TestNotIndexedLoop.java b/jadx-core/src/test/java/jadx/tests/integration/loops/TestNotIndexedLoop.java new file mode 100644 index 000000000..7642a4661 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/loops/TestNotIndexedLoop.java @@ -0,0 +1,77 @@ +package jadx.tests.integration.loops; + +import java.io.File; + +import org.junit.jupiter.api.Test; + +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.IntegrationTest; + +import static jadx.tests.api.utils.JadxMatchers.containsOne; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; + +public class TestNotIndexedLoop extends IntegrationTest { + + public static class TestCls { + + public File test(File[] files) { + File file; + if (files != null) { + int length = files.length; + if (length == 0) { + file = null; + } else { + int i = 0; + while (true) { + if (i >= length) { + file = null; + break; + } + file = files[i]; + if (file.getName().equals("f")) { + break; + } + i++; + } + } + } else { + file = null; + } + if (file != null) { + file.deleteOnExit(); + } + return file; + } + + public void check() { + assertThat(test(null), nullValue()); + assertThat(test(new File[]{}), nullValue()); + + File file = new File("f"); + assertThat(test(new File[]{new File("a"), file}), is(file)); + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + + assertThat(code, not(containsString("for ("))); + assertThat(code, containsOne("while (true) {")); + } + + @Test + public void testNoDebug() { + noDebugInfo(); + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + + assertThat(code, not(containsString("for ("))); + assertThat(code, containsOne("while (true) {")); + } +}