From 1a85fa8e3cfe31b1a98e61484026b33dce3e30b0 Mon Sep 17 00:00:00 2001 From: Skylot Date: Sat, 13 Dec 2014 18:24:36 +0300 Subject: [PATCH] core: fix complex conditions with mode alternation (fix #31) --- .../core/dex/regions/conditions/IfInfo.java | 6 +- .../core/dex/regions/loops/LoopRegion.java | 2 +- .../dex/visitors/regions/CheckRegions.java | 7 ++- .../dex/visitors/regions/IfMakerHelper.java | 60 +++++++++++++++---- .../dex/visitors/regions/RegionMaker.java | 16 +++-- .../main/java/jadx/core/utils/BlockUtils.java | 12 +++- .../conditions/TestConditions16.java | 37 ++++++++++++ .../integration/loops/TestIfInLoop3.java | 57 ++++++++++++++++++ 8 files changed, 170 insertions(+), 27 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions16.java create mode 100644 jadx-core/src/test/java/jadx/tests/integration/loops/TestIfInLoop3.java diff --git a/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfInfo.java b/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfInfo.java index d21a18fba..1d53b2bd3 100644 --- a/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfInfo.java +++ b/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfInfo.java @@ -19,10 +19,6 @@ public final class IfInfo { this(condition, thenBlock, elseBlock, new HashSet(), new HashSet()); } - public IfInfo(IfCondition condition, IfInfo info) { - this(condition, info.getThenBlock(), info.getElseBlock(), info.getMergedBlocks(), info.getSkipBlocks()); - } - public IfInfo(IfInfo info, BlockNode thenBlock, BlockNode elseBlock) { this(info.getCondition(), thenBlock, elseBlock, info.getMergedBlocks(), info.getSkipBlocks()); } @@ -90,6 +86,6 @@ public final class IfInfo { @Override public String toString() { - return "IfInfo: " + condition + ", then: " + thenBlock + ", else: " + elseBlock; + return "IfInfo: then: " + thenBlock + ", else: " + elseBlock; } } diff --git a/jadx-core/src/main/java/jadx/core/dex/regions/loops/LoopRegion.java b/jadx-core/src/main/java/jadx/core/dex/regions/loops/LoopRegion.java index 3cd3fed14..eaa0117e4 100644 --- a/jadx-core/src/main/java/jadx/core/dex/regions/loops/LoopRegion.java +++ b/jadx-core/src/main/java/jadx/core/dex/regions/loops/LoopRegion.java @@ -156,7 +156,7 @@ public final class LoopRegion extends AbstractRegion { @Override public String baseString() { - return body.baseString(); + return body == null ? "-" : body.baseString(); } @Override diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CheckRegions.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CheckRegions.java index cc6bccea9..421b285c4 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CheckRegions.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CheckRegions.java @@ -30,7 +30,7 @@ public class CheckRegions extends AbstractVisitor { return; } - // printRegion(mth, mth.getRegion(), "|"); + // printRegion(mth); // check if all blocks included in regions final Set blocksInRegions = new HashSet(); @@ -93,6 +93,11 @@ public class CheckRegions extends AbstractVisitor { LOG.debug(" Found block: {} in regions: {}", block, regions); } + private void printRegion(MethodNode mth) { + LOG.debug("|" + mth.toString()); + printRegion(mth, mth.getRegion(), "| "); + } + private void printRegion(MethodNode mth, IRegion region, String indent) { LOG.debug(indent + region); for (IContainer container : region.getSubBlocks()) { diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfMakerHelper.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfMakerHelper.java index 097864a70..3d44007d0 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfMakerHelper.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfMakerHelper.java @@ -14,6 +14,7 @@ import jadx.core.dex.regions.conditions.IfCondition; import jadx.core.dex.regions.conditions.IfCondition.Mode; import jadx.core.dex.regions.conditions.IfInfo; import jadx.core.utils.BlockUtils; +import jadx.core.utils.exceptions.JadxRuntimeException; import java.util.Collection; import java.util.List; @@ -22,6 +23,8 @@ import java.util.Set; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import static jadx.core.dex.visitors.regions.RegionMaker.isEqualPaths; +import static jadx.core.dex.visitors.regions.RegionMaker.isReturnBlocks; import static jadx.core.utils.BlockUtils.getNextBlock; import static jadx.core.utils.BlockUtils.isPathExists; @@ -117,6 +120,7 @@ public class IfMakerHelper { List preds = block.getPredecessors(); Set ifBlocks = info.getMergedBlocks(); for (BlockNode pred : preds) { + pred = BlockUtils.skipSyntheticPredecessor(pred); if (!ifBlocks.contains(pred) && !pred.contains(AFlag.LOOP_END)) { return false; } @@ -150,22 +154,18 @@ public class IfMakerHelper { // invert current node for match pattern nextIf = IfInfo.invert(nextIf); } - if (!RegionMaker.isEqualPaths(curElse, nextIf.getElseBlock()) - && !RegionMaker.isEqualPaths(curThen, nextIf.getThenBlock())) { + if (!isEqualPaths(curThen, nextIf.getThenBlock()) + && !isEqualPaths(curElse, nextIf.getElseBlock())) { // complex condition, run additional checks if (checkConditionBranches(curThen, curElse) || checkConditionBranches(curElse, curThen)) { return null; } BlockNode otherBranchBlock = followThenBranch ? curElse : curThen; + otherBranchBlock = BlockUtils.skipSyntheticSuccessor(otherBranchBlock); if (!isPathExists(nextIf.getIfBlock(), otherBranchBlock)) { return checkForTernaryInCondition(currentIf); } - if (isPathExists(nextIf.getThenBlock(), otherBranchBlock) - && isPathExists(nextIf.getElseBlock(), otherBranchBlock)) { - // both branches paths points to one block - return null; - } // this is nested conditions with different mode (i.e (a && b) || c), // search next condition for merge, get null if failed @@ -175,6 +175,9 @@ public class IfMakerHelper { if (isInversionNeeded(currentIf, nextIf)) { nextIf = IfInfo.invert(nextIf); } + if (!canMerge(currentIf, nextIf, followThenBranch)) { + return currentIf; + } } else { return currentIf; } @@ -219,8 +222,16 @@ public class IfMakerHelper { } private static boolean isInversionNeeded(IfInfo currentIf, IfInfo nextIf) { - return RegionMaker.isEqualPaths(currentIf.getElseBlock(), nextIf.getThenBlock()) - || RegionMaker.isEqualPaths(currentIf.getThenBlock(), nextIf.getElseBlock()); + return isEqualPaths(currentIf.getElseBlock(), nextIf.getThenBlock()) + || isEqualPaths(currentIf.getThenBlock(), nextIf.getElseBlock()); + } + + private static boolean canMerge(IfInfo a, IfInfo b, boolean followThenBranch) { + if (followThenBranch) { + return isEqualPaths(a.getElseBlock(), b.getElseBlock()); + } else { + return isEqualPaths(a.getThenBlock(), b.getThenBlock()); + } } private static boolean checkConditionBranches(BlockNode from, BlockNode to) { @@ -231,7 +242,17 @@ public class IfMakerHelper { Mode mergeOperation = followThenBranch ? Mode.AND : Mode.OR; IfCondition condition = IfCondition.merge(mergeOperation, first.getCondition(), second.getCondition()); - IfInfo result = new IfInfo(condition, second); + // skip synthetic successor if both parts leads to same block + BlockNode thenBlock; + BlockNode elseBlock; + if (followThenBranch) { + thenBlock = second.getThenBlock(); + elseBlock = getCrossBlock(first.getElseBlock(), second.getElseBlock()); + } else { + thenBlock = getCrossBlock(first.getThenBlock(), second.getThenBlock()); + elseBlock = second.getElseBlock(); + } + IfInfo result = new IfInfo(condition, thenBlock, elseBlock); result.setIfBlock(first.getIfBlock()); result.merge(first, second); @@ -240,6 +261,25 @@ public class IfMakerHelper { return result; } + private static BlockNode getCrossBlock(BlockNode first, BlockNode second) { + if (isSameBlocks(first, second)) { + return second; + } + BlockNode firstSkip = BlockUtils.skipSyntheticSuccessor(first); + if (isSameBlocks(firstSkip, second)) { + return second; + } + BlockNode secondSkip = BlockUtils.skipSyntheticSuccessor(second); + if (isSameBlocks(firstSkip, secondSkip) || isSameBlocks(first, secondSkip)) { + return secondSkip; + } + throw new JadxRuntimeException("Unexpected merge pattern"); + } + + private static boolean isSameBlocks(BlockNode first, BlockNode second) { + return first == second || isReturnBlocks(first, second); + } + static void confirmMerge(IfInfo info) { if (info.getMergedBlocks().size() > 1) { for (BlockNode block : info.getMergedBlocks()) { 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 ee13530ed..c1e920130 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 @@ -49,6 +49,7 @@ import static jadx.core.dex.visitors.regions.IfMakerHelper.searchNestedIf; import static jadx.core.utils.BlockUtils.getBlockByOffset; import static jadx.core.utils.BlockUtils.getNextBlock; import static jadx.core.utils.BlockUtils.isPathExists; +import static jadx.core.utils.BlockUtils.skipSyntheticSuccessor; public class RegionMaker { private static final Logger LOG = LoggerFactory.getLogger(RegionMaker.class); @@ -811,15 +812,12 @@ public class RegionMaker { } private static boolean isSyntheticPath(BlockNode b1, BlockNode b2) { - if (!b1.isSynthetic() || !b2.isSynthetic()) { - return false; - } - BlockNode n1 = getNextBlock(b1); - BlockNode n2 = getNextBlock(b2); - return isEqualPaths(n1, n2); + BlockNode n1 = skipSyntheticSuccessor(b1); + BlockNode n2 = skipSyntheticSuccessor(b2); + return (n1 != b1 || n2 != b2) && isEqualPaths(n1, n2); } - private static boolean isReturnBlocks(BlockNode b1, BlockNode b2) { + public static boolean isReturnBlocks(BlockNode b1, BlockNode b2) { if (!b1.isReturnBlock() || !b2.isReturnBlock()) { return false; } @@ -830,9 +828,9 @@ public class RegionMaker { } InsnNode i1 = b1Insns.get(0); InsnNode i2 = b2Insns.get(0); - if (i1.getArgsCount() == 0 || i2.getArgsCount() == 0) { + if (i1.getArgsCount() != i2.getArgsCount()) { return false; } - return i1.getArg(0).equals(i2.getArg(0)); + return i1.getArgsCount() == 0 || i1.getArg(0).equals(i2.getArg(0)); } } diff --git a/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java b/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java index b48f033f4..bbcb1170c 100644 --- a/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java @@ -452,9 +452,19 @@ public class BlockUtils { * Return successor of synthetic block or same block otherwise. */ public static BlockNode skipSyntheticSuccessor(BlockNode block) { - if (block.isSynthetic() && !block.getSuccessors().isEmpty()) { + if (block.isSynthetic() && block.getSuccessors().size() == 1) { return block.getSuccessors().get(0); } return block; } + + /** + * Return predecessor of synthetic block or same block otherwise. + */ + public static BlockNode skipSyntheticPredecessor(BlockNode block) { + if (block.isSynthetic() && block.getPredecessors().size() == 1) { + return block.getPredecessors().get(0); + } + return block; + } } diff --git a/jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions16.java b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions16.java new file mode 100644 index 000000000..3d47787a3 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions16.java @@ -0,0 +1,37 @@ +package jadx.tests.integration.conditions; + +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.IntegrationTest; + +import org.junit.Test; + +import static jadx.tests.api.utils.JadxMatchers.containsOne; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; + +public class TestConditions16 extends IntegrationTest { + + public static class TestCls { + private static boolean test(int a, int b) { + return a < 0 || b % 2 != 0 && a > 28 || b < 0; + } + + public void check() { + assertTrue(test(-1, 1)); + assertTrue(test(1, -1)); + assertTrue(test(29, 3)); + assertFalse(test(2, 2)); + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + +// assertThat(code, containsOne("return a < 0 || (b % 2 != 0 && a > 28) || b < 0;")); + assertThat(code, containsOne("return a < 0 || ((b % 2 != 0 && a > 28) || b < 0);")); + + } +} diff --git a/jadx-core/src/test/java/jadx/tests/integration/loops/TestIfInLoop3.java b/jadx-core/src/test/java/jadx/tests/integration/loops/TestIfInLoop3.java new file mode 100644 index 000000000..2dc50d237 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/loops/TestIfInLoop3.java @@ -0,0 +1,57 @@ +package jadx.tests.integration.loops; + +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.IntegrationTest; + +import org.junit.Test; + +import static jadx.tests.api.utils.JadxMatchers.containsOne; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; + +public class TestIfInLoop3 extends IntegrationTest { + + public static class TestCls { + static boolean[][] occupied = new boolean[70][70]; + static boolean placingStone = true; + + private static boolean test(int xx, int yy) { + int[] extraArray = new int[]{10, 45, 50, 50, 20, 20}; + if (extraArray != null && placingStone) { + for (int i = 0; i < extraArray.length; i += 2) { + int tX; + int tY; + if (yy % 2 == 0) { + if (extraArray[i + 1] % 2 == 0) { + tX = xx + extraArray[i]; + } else { + tX = extraArray[i] + xx - 1; + } + tY = yy + extraArray[i + 1]; + } else { + tX = xx + extraArray[i]; + tY = yy + extraArray[i + 1]; + } + if (tX < 0 || tY < 0 || tY % 2 != 0 && tX > 28 || tY > 70 + || occupied[tX][tY]) { + return false; + } + } + } + return true; + } + + public void check() { + assertTrue(test(14, 2)); + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + + assertThat(code, containsOne("for (int i = 0; i < extraArray.length; i += 2) {")); + assertThat(code, containsOne("if (extraArray != null && placingStone) {")); + } +}