From ebbe6db378305e6d6c1ac014f0cc2cbfbcaa66fc Mon Sep 17 00:00:00 2001 From: Skylot Date: Sat, 12 Jul 2014 21:09:51 +0400 Subject: [PATCH] core: fix complex 'if' processing (issues #9 and #12) --- jadx-core/src/main/java/jadx/core/Jadx.java | 12 +- .../java/jadx/core/codegen/ConditionGen.java | 27 ++- .../java/jadx/core/dex/attributes/AType.java | 3 + .../jadx/core/dex/regions/IfCondition.java | 7 +- .../java/jadx/core/dex/regions/IfInfo.java | 57 +++-- .../dex/visitors/regions/CheckRegions.java | 14 +- .../dex/visitors/regions/IfMakerHelper.java | 190 +++++++++++++++ .../dex/visitors/regions/IfRegionVisitor.java | 35 ++- .../regions/ProcessTryCatchRegions.java | 2 +- .../dex/visitors/regions/RegionMaker.java | 222 +++--------------- .../dex/visitors/regions/ReturnVisitor.java | 6 +- .../main/java/jadx/core/utils/BlockUtils.java | 13 +- .../java/jadx/core/utils/RegionUtils.java | 2 +- .../internal/conditions/TestConditions10.java | 45 ++++ .../internal/conditions/TestConditions11.java | 39 +++ .../internal/conditions/TestConditions12.java | 71 ++++++ .../internal/conditions/TestConditions13.java | 43 ++++ .../internal/conditions/TestConditions2.java | 1 - .../internal/conditions/TestConditions5.java | 12 + .../internal/conditions/TestConditions9.java | 37 +++ .../conditions/TestSimpleConditions.java | 32 +++ .../internal/loops/TestLoopCondition2.java | 7 +- 22 files changed, 632 insertions(+), 245 deletions(-) create mode 100644 jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfMakerHelper.java create mode 100644 jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions10.java create mode 100644 jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions11.java create mode 100644 jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions12.java create mode 100644 jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions13.java create mode 100644 jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions9.java create mode 100644 jadx-core/src/test/java/jadx/tests/internal/conditions/TestSimpleConditions.java diff --git a/jadx-core/src/main/java/jadx/core/Jadx.java b/jadx-core/src/main/java/jadx/core/Jadx.java index b896d613..4a628468 100644 --- a/jadx-core/src/main/java/jadx/core/Jadx.java +++ b/jadx-core/src/main/java/jadx/core/Jadx.java @@ -60,17 +60,19 @@ public class Jadx { passes.add(new ConstInlinerVisitor()); passes.add(new FinishTypeInference()); - + passes.add(new EliminatePhiNodes()); if (args.isRawCFGOutput()) { passes.add(new DotGraphVisitor(outDir, false, true)); } - passes.add(new EliminatePhiNodes()); - passes.add(new ModVisitor()); passes.add(new EnumVisitor()); passes.add(new CodeShrinker()); + if (args.isCFGOutput()) { + passes.add(new DotGraphVisitor(outDir, false)); + } + passes.add(new RegionMakerVisitor()); passes.add(new IfRegionVisitor()); passes.add(new ReturnVisitor()); @@ -87,10 +89,6 @@ public class Jadx { passes.add(new MethodInlineVisitor()); passes.add(new ClassModifier()); passes.add(new PrepareForCodeGen()); - - if (args.isCFGOutput()) { - passes.add(new DotGraphVisitor(outDir, false)); - } } passes.add(new CodeGen(args)); return passes; diff --git a/jadx-core/src/main/java/jadx/core/codegen/ConditionGen.java b/jadx-core/src/main/java/jadx/core/codegen/ConditionGen.java index 13b95a9f..a838addf 100644 --- a/jadx-core/src/main/java/jadx/core/codegen/ConditionGen.java +++ b/jadx-core/src/main/java/jadx/core/codegen/ConditionGen.java @@ -75,7 +75,7 @@ public class ConditionGen extends InsnGen { } else if (op == IfOp.NE) { // != true code.add('!'); - boolean wrap = isWrapNeeded(firstArg); + boolean wrap = isArgWrapNeeded(firstArg); if (wrap) { code.add('('); } @@ -88,9 +88,9 @@ public class ConditionGen extends InsnGen { LOG.warn(ErrorsCounter.formatErrorMsg(mth, "Unsupported boolean condition " + op.getSymbol())); } - addArg(code, firstArg, isWrapNeeded(firstArg)); + addArg(code, firstArg, isArgWrapNeeded(firstArg)); code.add(' ').add(op.getSymbol()).add(' '); - addArg(code, secondArg, isWrapNeeded(secondArg)); + addArg(code, secondArg, isArgWrapNeeded(secondArg)); } private void addNot(CodeWriter code, IfCondition condition) throws CodegenException { @@ -110,15 +110,16 @@ public class ConditionGen extends InsnGen { } private boolean isWrapNeeded(IfCondition condition) { - return !condition.isCompare(); + return !condition.isCompare() && condition.getMode() != IfCondition.Mode.NOT; } - private static boolean isWrapNeeded(InsnArg arg) { + private static boolean isArgWrapNeeded(InsnArg arg) { if (!arg.isInsnWrap()) { return false; } InsnNode insn = ((InsnWrapArg) arg).getWrapInsn(); - if (insn.getType() == InsnType.ARITH) { + InsnType insnType = insn.getType(); + if (insnType == InsnType.ARITH) { switch (((ArithNode) insn).getOp()) { case ADD: case SUB: @@ -127,8 +128,18 @@ public class ConditionGen extends InsnGen { case REM: return false; } - } else if (insn.getType() == InsnType.INVOKE) { - return false; + } else { + switch (insnType) { + case INVOKE: + case SGET: + case IGET: + case AGET: + case CONST: + case ARRAY_LENGTH: + return false; + default: + return true; + } } return true; } diff --git a/jadx-core/src/main/java/jadx/core/dex/attributes/AType.java b/jadx-core/src/main/java/jadx/core/dex/attributes/AType.java index f982a8a0..24895646 100644 --- a/jadx-core/src/main/java/jadx/core/dex/attributes/AType.java +++ b/jadx-core/src/main/java/jadx/core/dex/attributes/AType.java @@ -25,6 +25,9 @@ import jadx.core.dex.trycatch.SplitterBlockAttr; */ public class AType { + private AType() { + } + public static final AType> JUMP = new AType>(); public static final AType> LOOP = new AType>(); diff --git a/jadx-core/src/main/java/jadx/core/dex/regions/IfCondition.java b/jadx-core/src/main/java/jadx/core/dex/regions/IfCondition.java index c0452c14..6da6e09c 100644 --- a/jadx-core/src/main/java/jadx/core/dex/regions/IfCondition.java +++ b/jadx-core/src/main/java/jadx/core/dex/regions/IfCondition.java @@ -69,13 +69,8 @@ public final class IfCondition { IfCondition n = new IfCondition(a); n.addArg(b); return n; - } else if (b.getMode() == mode) { - IfCondition n = new IfCondition(b); - n.addArg(a); - return n; - } else { - return new IfCondition(mode, Arrays.asList(a, b)); } + return new IfCondition(mode, Arrays.asList(a, b)); } public Mode getMode() { diff --git a/jadx-core/src/main/java/jadx/core/dex/regions/IfInfo.java b/jadx-core/src/main/java/jadx/core/dex/regions/IfInfo.java index eb708171..9f3fe28d 100644 --- a/jadx-core/src/main/java/jadx/core/dex/regions/IfInfo.java +++ b/jadx-core/src/main/java/jadx/core/dex/regions/IfInfo.java @@ -2,41 +2,64 @@ package jadx.core.dex.regions; import jadx.core.dex.nodes.BlockNode; +import java.util.HashSet; +import java.util.Set; + public final class IfInfo { - IfCondition condition; - BlockNode ifnode; - BlockNode thenBlock; - BlockNode elseBlock; + private final IfCondition condition; + private final Set mergedBlocks = new HashSet(); + private final BlockNode thenBlock; + private final BlockNode elseBlock; + @Deprecated + private BlockNode ifBlock; - public IfCondition getCondition() { - return condition; + public IfInfo(IfCondition condition, BlockNode thenBlock, BlockNode elseBlock) { + this.condition = condition; + this.thenBlock = thenBlock; + this.elseBlock = elseBlock; } - public void setCondition(IfCondition condition) { + public IfInfo(IfCondition condition, IfInfo info) { this.condition = condition; + this.thenBlock = info.getThenBlock(); + this.elseBlock = info.getElseBlock(); + this.mergedBlocks.addAll(info.getMergedBlocks()); } - public BlockNode getIfnode() { - return ifnode; + public static IfInfo invert(IfInfo info) { + IfInfo tmpIf = new IfInfo(IfCondition.invert(info.getCondition()), + info.getElseBlock(), info.getThenBlock()); + tmpIf.setIfBlock(info.getIfBlock()); + tmpIf.getMergedBlocks().addAll(info.getMergedBlocks()); + return tmpIf; } - public void setIfnode(BlockNode ifnode) { - this.ifnode = ifnode; + public IfCondition getCondition() { + return condition; } - public BlockNode getThenBlock() { - return thenBlock; + public Set getMergedBlocks() { + return mergedBlocks; } - public void setThenBlock(BlockNode thenBlock) { - this.thenBlock = thenBlock; + public BlockNode getThenBlock() { + return thenBlock; } public BlockNode getElseBlock() { return elseBlock; } - public void setElseBlock(BlockNode elseBlock) { - this.elseBlock = elseBlock; + public BlockNode getIfBlock() { + return ifBlock; + } + + public void setIfBlock(BlockNode ifBlock) { + this.ifBlock = ifBlock; + } + + @Override + public String toString() { + return "IfInfo: " + condition + ", then: " + thenBlock + ", else: " + elseBlock; } } 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 6d0fa4f2..b4a4ddbe 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,16 +30,14 @@ public class CheckRegions extends AbstractVisitor { // check if all blocks included in regions final Set blocksInRegions = new HashSet(); - IRegionVisitor collectBlocks = new AbstractRegionVisitor() { + DepthRegionTraversal.traverseAll(mth, new AbstractRegionVisitor() { @Override public void processBlock(MethodNode mth, IBlock container) { if (container instanceof BlockNode) { blocksInRegions.add((BlockNode) container); } } - }; - DepthRegionTraversal.traverseAll(mth, collectBlocks); - + }); if (mth.getBasicBlocks().size() != blocksInRegions.size()) { for (BlockNode block : mth.getBasicBlocks()) { if (!blocksInRegions.contains(block) @@ -52,19 +50,17 @@ public class CheckRegions extends AbstractVisitor { } // check loop conditions - IRegionVisitor checkLoops = new AbstractRegionVisitor() { + DepthRegionTraversal.traverseAll(mth, new AbstractRegionVisitor() { @Override public void enterRegion(MethodNode mth, IRegion region) { if (region instanceof LoopRegion) { - LoopRegion loop = (LoopRegion) region; - BlockNode loopHeader = loop.getHeader(); + BlockNode loopHeader = ((LoopRegion) region).getHeader(); if (loopHeader != null && loopHeader.getInstructions().size() != 1) { ErrorsCounter.methodError(mth, "Incorrect condition in loop: " + loopHeader); mth.add(AFlag.INCONSISTENT_CODE); } } } - }; - DepthRegionTraversal.traverseAll(mth, checkLoops); + }); } } 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 new file mode 100644 index 00000000..c99e297b --- /dev/null +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfMakerHelper.java @@ -0,0 +1,190 @@ +package jadx.core.dex.visitors.regions; + +import jadx.core.dex.attributes.AFlag; +import jadx.core.dex.attributes.AType; +import jadx.core.dex.instructions.IfNode; +import jadx.core.dex.instructions.InsnType; +import jadx.core.dex.instructions.args.InsnArg; +import jadx.core.dex.instructions.args.RegisterArg; +import jadx.core.dex.nodes.BlockNode; +import jadx.core.dex.nodes.InsnNode; +import jadx.core.dex.regions.IfCondition; +import jadx.core.dex.regions.IfCondition.Mode; +import jadx.core.dex.regions.IfInfo; +import jadx.core.utils.BlockUtils; + +import java.util.List; + +import static jadx.core.utils.BlockUtils.isPathExists; + +public class IfMakerHelper { + private IfMakerHelper() { + } + + static IfInfo makeIfInfo(BlockNode ifBlock) { + return makeIfInfo(ifBlock, IfCondition.fromIfBlock(ifBlock)); + } + + static IfInfo mergeNestedIfNodes(BlockNode block) { + IfInfo info = makeIfInfo(block); + return mergeNestedIfNodes(info); + } + + private static IfInfo mergeNestedIfNodes(IfInfo currentIf) { + BlockNode curThen = currentIf.getThenBlock(); + BlockNode curElse = currentIf.getElseBlock(); + if (curThen == curElse) { + return null; + } + boolean followThenBranch; + IfInfo nextIf = getNextIf(currentIf, curThen); + if (nextIf != null) { + followThenBranch = true; + } else { + nextIf = getNextIf(currentIf, curElse); + if (nextIf != null) { + followThenBranch = false; + } else { + return null; + } + } + if (isInversionNeeded(currentIf, nextIf)) { + // invert current node for match pattern + nextIf = IfInfo.invert(nextIf); + } + if (!RegionMaker.isEqualPaths(curElse, nextIf.getElseBlock()) + && !RegionMaker.isEqualPaths(curThen, nextIf.getThenBlock())) { + // complex condition, run additional checks + if (checkConditionBranches(curThen, curElse) || checkConditionBranches(curElse, curThen)) { + return null; + } + BlockNode otherBranchBlock = followThenBranch ? curElse : curThen; + if (!isPathExists(nextIf.getIfBlock(), otherBranchBlock)) { + return null; + } + 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 + IfInfo tmpIf = mergeNestedIfNodes(nextIf); + if (tmpIf != null) { + nextIf = tmpIf; + if (isInversionNeeded(currentIf, nextIf)) { + nextIf = IfInfo.invert(nextIf); + } + } + } + + IfInfo result = mergeIfInfo(currentIf, nextIf, followThenBranch); + // search next nested if block + IfInfo next = mergeNestedIfNodes(result); + if (next != null) { + return next; + } + return result; + } + + private static boolean isInversionNeeded(IfInfo currentIf, IfInfo nextIf) { + return RegionMaker.isEqualPaths(currentIf.getElseBlock(), nextIf.getThenBlock()) + || RegionMaker.isEqualPaths(currentIf.getThenBlock(), nextIf.getElseBlock()); + } + + private static IfInfo makeIfInfo(BlockNode ifBlock, IfCondition condition) { + IfNode ifnode = (IfNode) ifBlock.getInstructions().get(0); + IfInfo info = new IfInfo(condition, ifnode.getThenBlock(), ifnode.getElseBlock()); + info.setIfBlock(ifBlock); + info.getMergedBlocks().add(ifBlock); + return info; + } + + private static boolean checkConditionBranches(BlockNode from, BlockNode to) { + return from.getCleanSuccessors().size() == 1 && from.getCleanSuccessors().contains(to); + } + + private static IfInfo mergeIfInfo(IfInfo first, IfInfo second, boolean followThenBranch) { + Mode mergeOperation = followThenBranch ? Mode.AND : Mode.OR; + BlockNode otherPathBlock = followThenBranch ? first.getElseBlock() : first.getThenBlock(); + RegionMaker.skipSimplePath(otherPathBlock); + first.getIfBlock().add(AFlag.SKIP); + second.getIfBlock().add(AFlag.SKIP); + + IfCondition condition = IfCondition.merge(mergeOperation, first.getCondition(), second.getCondition()); + IfInfo result = new IfInfo(condition, second); + result.setIfBlock(first.getIfBlock()); + result.getMergedBlocks().addAll(first.getMergedBlocks()); + result.getMergedBlocks().addAll(second.getMergedBlocks()); + return result; + } + + private static IfInfo getNextIf(IfInfo info, BlockNode block) { + if (!canSelectNext(info, block)) { + return null; + } + BlockNode nestedIfBlock = getNextIfNode(block); + if (nestedIfBlock != null) { + return makeIfInfo(nestedIfBlock); + } + return null; + } + + private static boolean canSelectNext(IfInfo info, BlockNode block) { + if (block.getPredecessors().size() == 1) { + return true; + } + if (info.getMergedBlocks().containsAll(block.getPredecessors())) { + return true; + } + return false; + } + + private static BlockNode getNextIfNode(BlockNode block) { + if (block == null || block.contains(AType.LOOP) || block.contains(AFlag.SKIP)) { + return null; + } + List insns = block.getInstructions(); + if (insns.size() == 1 && insns.get(0).getType() == InsnType.IF) { + return block; + } + // skip this block and search in successors chain + List successors = block.getSuccessors(); + if (successors.size() != 1) { + return null; + } + + BlockNode next = successors.get(0); + if (next.getPredecessors().size() != 1) { + return null; + } + boolean pass = true; + if (!insns.isEmpty()) { + // check that all instructions can be inlined + for (InsnNode insn : insns) { + RegisterArg res = insn.getResult(); + if (res == null) { + pass = false; + break; + } + List useList = res.getSVar().getUseList(); + if (useList.size() != 1) { + pass = false; + break; + } + InsnArg arg = useList.get(0); + InsnNode usePlace = arg.getParentInsn(); + if (!BlockUtils.blockContains(block, usePlace) + && !BlockUtils.blockContains(next, usePlace)) { + pass = false; + break; + } + } + } + if (pass) { + return getNextIfNode(next); + } + return null; + } +} 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 e50132ea..94bf408f 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 @@ -14,6 +14,8 @@ import jadx.core.utils.RegionUtils; import java.util.List; +import static jadx.core.utils.RegionUtils.insnsCount; + public class IfRegionVisitor extends AbstractVisitor implements IRegionVisitor, IRegionIterativeVisitor { @Override @@ -60,10 +62,36 @@ public class IfRegionVisitor extends AbstractVisitor implements IRegionVisitor, invertIfRegion(ifRegion); } } - if (RegionUtils.isEmpty(ifRegion.getThenRegion()) - || hasSimpleReturnBlock(ifRegion.getThenRegion())) { + IContainer elseRegion = ifRegion.getElseRegion(); + if (elseRegion == null || RegionUtils.isEmpty(elseRegion)) { + return; + } + boolean thenIsEmpty = RegionUtils.isEmpty(ifRegion.getThenRegion()); + if (thenIsEmpty || hasSimpleReturnBlock(ifRegion.getThenRegion())) { invertIfRegion(ifRegion); } + + if (!thenIsEmpty) { + // move 'if' from then to make 'else if' chain + if (isIfRegion(ifRegion.getThenRegion()) + && !isIfRegion(elseRegion)) { + invertIfRegion(ifRegion); + } + + } + } + + private static boolean isIfRegion(IContainer container) { + if (container instanceof IfRegion) { + return true; + } + if (container instanceof IRegion) { + List subBlocks = ((IRegion) container).getSubBlocks(); + if (subBlocks.size() == 1 && subBlocks.get(0) instanceof IfRegion) { + return true; + } + } + return false; } private static void moveReturnToThenBlock(MethodNode mth, IfRegion ifRegion) { @@ -95,7 +123,8 @@ public class IfRegionVisitor extends AbstractVisitor implements IRegionVisitor, if (ifRegion.getElseRegion() != null && !ifRegion.contains(AFlag.ELSE_IF_CHAIN) && !ifRegion.getElseRegion().contains(AFlag.ELSE_IF_CHAIN) - && RegionUtils.hasExitBlock(ifRegion.getThenRegion())) { + && RegionUtils.hasExitBlock(ifRegion.getThenRegion()) + && insnsCount(ifRegion.getThenRegion()) < 2) { IRegion parent = ifRegion.getParent(); Region newRegion = new Region(parent); if (parent.replaceSubBlock(ifRegion, newRegion)) { diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessTryCatchRegions.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessTryCatchRegions.java index c15da1c2..8ca9baf3 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessTryCatchRegions.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessTryCatchRegions.java @@ -151,7 +151,7 @@ public class ProcessTryCatchRegions extends AbstractRegionVisitor { aReg.setParent(newRegion); } } - + return newRegion; } 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 067dd94f..d278e7e9 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 @@ -8,13 +8,11 @@ import jadx.core.dex.instructions.IfNode; import jadx.core.dex.instructions.InsnType; import jadx.core.dex.instructions.SwitchNode; import jadx.core.dex.instructions.args.InsnArg; -import jadx.core.dex.instructions.args.RegisterArg; import jadx.core.dex.nodes.BlockNode; import jadx.core.dex.nodes.Edge; import jadx.core.dex.nodes.IRegion; import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.nodes.MethodNode; -import jadx.core.dex.regions.IfCondition; import jadx.core.dex.regions.IfInfo; import jadx.core.dex.regions.IfRegion; import jadx.core.dex.regions.LoopRegion; @@ -42,10 +40,9 @@ import java.util.Set; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static jadx.core.dex.regions.IfCondition.Mode; +import static jadx.core.dex.visitors.regions.IfMakerHelper.mergeNestedIfNodes; import static jadx.core.utils.BlockUtils.getBlockByOffset; import static jadx.core.utils.BlockUtils.isPathExists; -import static jadx.core.utils.BlockUtils.selectOther; public class RegionMaker { private static final Logger LOG = LoggerFactory.getLogger(RegionMaker.class); @@ -110,29 +107,26 @@ public class RegionMaker { } } - if (!processed) { - int size = block.getInstructions().size(); - if (size == 1) { - InsnNode insn = block.getInstructions().get(0); - switch (insn.getType()) { - case IF: - next = processIf(r, block, (IfNode) insn, stack); - processed = true; - break; + if (!processed && block.getInstructions().size() == 1) { + InsnNode insn = block.getInstructions().get(0); + switch (insn.getType()) { + case IF: + next = processIf(r, block, (IfNode) insn, stack); + processed = true; + break; - case SWITCH: - next = processSwitch(r, block, (SwitchNode) insn, stack); - processed = true; - break; + case SWITCH: + next = processSwitch(r, block, (SwitchNode) insn, stack); + processed = true; + break; - case MONITOR_ENTER: - next = processMonitorEnter(r, block, insn, stack); - processed = true; - break; + case MONITOR_ENTER: + next = processMonitorEnter(r, block, insn, stack); + processed = true; + break; - default: - break; - } + default: + break; } } if (!processed) { @@ -170,10 +164,7 @@ public class RegionMaker { LoopRegion loopRegion = null; // exit block with loop condition - BlockNode condBlock = null; - - // first block in loop - BlockNode bThen = null; + IfInfo condInfo = null; for (BlockNode exit : exitBlocks) { if (exit.contains(AType.EXC_HANDLER) @@ -185,7 +176,7 @@ public class RegionMaker { continue; } ifnode = (IfNode) insn; - condBlock = exit; + BlockNode condBlock = exit; loopRegion = new LoopRegion(curRegion, condBlock, condBlock == loop.getEnd()); boolean found = true; @@ -205,46 +196,31 @@ public class RegionMaker { // try another exit continue; } - - List merged = new ArrayList(2); - IfInfo mergedIf = mergeNestedIfNodes(condBlock, ifnode, merged); - if (mergedIf != null) { - condBlock = mergedIf.getIfnode(); - if (!loop.getLoopBlocks().contains(mergedIf.getThenBlock())) { - // invert loop condition if it points to exit - loopRegion.setCondition(IfCondition.invert(mergedIf.getCondition())); - bThen = mergedIf.getElseBlock(); - } else { - loopRegion.setCondition(mergedIf.getCondition()); - bThen = mergedIf.getThenBlock(); - } - exitBlocks.removeAll(merged); + condInfo = mergeNestedIfNodes(condBlock); + if (condInfo == null) { + condInfo = IfMakerHelper.makeIfInfo(condBlock); } break; } - // endless loop if (loopRegion == null) { return makeEndlessLoop(curRegion, stack, loop, loopStart); } - if (bThen == null) { - bThen = ifnode.getThenBlock(); - } - BlockNode loopBody = null; - for (BlockNode s : condBlock.getSuccessors()) { - if (loop.getLoopBlocks().contains(s)) { - loopBody = s; - break; - } + if (!loop.getLoopBlocks().contains(condInfo.getThenBlock())) { + // invert loop condition if 'then' points to exit + condInfo = IfInfo.invert(condInfo); } + loopRegion.setCondition(condInfo.getCondition()); + exitBlocks.removeAll(condInfo.getMergedBlocks()); + + BlockNode bThen = condInfo.getThenBlock(); curRegion.getSubBlocks().add(loopRegion); stack.push(loopRegion); - exitBlocks.remove(condBlock); if (exitBlocks.size() > 0) { - BlockNode loopExit = BlockUtils.selectOtherSafe(loopBody, condBlock.getCleanSuccessors()); + BlockNode loopExit = condInfo.getElseBlock(); if (loopExit != null) { // add 'break' instruction before path cross between main loop exit and subexit for (Edge exitEdge : loop.getExitEdges()) { @@ -267,10 +243,7 @@ public class RegionMaker { loopRegion.setBody(makeRegion(loopStart, stack)); loopStart.addAttr(AType.LOOP, loop); } else { - if (bThen != loopBody) { - loopRegion.setCondition(IfCondition.invert(loopRegion.getCondition())); - } - out = selectOther(loopBody, condBlock.getSuccessors()); + out = condInfo.getElseBlock(); if (out.contains(AFlag.LOOP_START) && !out.getAll(AType.LOOP).contains(loop) && stack.peekRegion() instanceof LoopRegion) { @@ -282,6 +255,7 @@ public class RegionMaker { } } stack.addExit(out); + BlockNode loopBody = condInfo.getThenBlock(); loopRegion.setBody(makeRegion(loopBody, stack)); } stack.pop(); @@ -415,7 +389,7 @@ public class RegionMaker { IfRegion ifRegion = new IfRegion(currentRegion, block); currentRegion.getSubBlocks().add(ifRegion); - IfInfo mergedIf = mergeNestedIfNodes(block, ifnode, null); + IfInfo mergedIf = mergeNestedIfNodes(block); if (mergedIf != null) { ifRegion.setCondition(mergedIf.getCondition()); thenBlock = mergedIf.getThenBlock(); @@ -476,130 +450,6 @@ public class RegionMaker { return out; } - private IfInfo mergeNestedIfNodes(BlockNode block, IfNode ifnode, List merged) { - IfInfo info = new IfInfo(); - info.setIfnode(block); - info.setCondition(IfCondition.fromIfBlock(block)); - info.setThenBlock(ifnode.getThenBlock()); - info.setElseBlock(ifnode.getElseBlock()); - return mergeNestedIfNodes(info, merged); - } - - private IfInfo mergeNestedIfNodes(IfInfo info, List merged) { - BlockNode bThen = info.getThenBlock(); - BlockNode bElse = info.getElseBlock(); - if (bThen == bElse) { - return null; - } - - BlockNode ifBlock = info.getIfnode(); - BlockNode nestedIfBlock = getNextIfBlock(ifBlock); - if (nestedIfBlock == null) { - return null; - } - - IfNode nestedIfInsn = (IfNode) nestedIfBlock.getInstructions().get(0); - IfCondition nestedCondition = IfCondition.fromIfNode(nestedIfInsn); - BlockNode nbThen = nestedIfInsn.getThenBlock(); - BlockNode nbElse = nestedIfInsn.getElseBlock(); - - IfCondition condition = info.getCondition(); - boolean inverted = false; - if (isPathExists(bElse, nestedIfBlock)) { - // else branch - if (!isEqualPaths(bThen, nbThen)) { - if (!isEqualPaths(bThen, nbElse)) { - // not connected conditions - return null; - } - nestedIfInsn.invertCondition(); - inverted = true; - } - condition = IfCondition.merge(Mode.OR, condition, nestedCondition); - } else { - // then branch - if (!isEqualPaths(bElse, nbElse)) { - if (!isEqualPaths(bElse, nbThen)) { - // not connected conditions - return null; - } - nestedIfInsn.invertCondition(); - inverted = true; - } - condition = IfCondition.merge(Mode.AND, condition, nestedCondition); - } - if (merged != null) { - merged.add(nestedIfBlock); - } - nestedIfBlock.add(AFlag.SKIP); - BlockNode blockToNestedIfBlock = BlockUtils.getNextBlockToPath(ifBlock, nestedIfBlock); - skipSimplePath(BlockUtils.selectOther(blockToNestedIfBlock, ifBlock.getCleanSuccessors())); - - IfInfo result = new IfInfo(); - result.setIfnode(nestedIfBlock); - result.setCondition(condition); - result.setThenBlock(inverted ? nbElse : nbThen); - result.setElseBlock(inverted ? nbThen : nbElse); - - // search next nested if block - IfInfo next = mergeNestedIfNodes(result, merged); - if (next != null) { - return next; - } - return result; - } - - private BlockNode getNextIfBlock(BlockNode block) { - for (BlockNode succ : block.getSuccessors()) { - BlockNode nestedIfBlock = getIfNode(succ); - if (nestedIfBlock != null && nestedIfBlock != block) { - return nestedIfBlock; - } - } - return null; - } - - private static BlockNode getIfNode(BlockNode block) { - if (block != null && !block.contains(AType.LOOP)) { - List insns = block.getInstructions(); - if (insns.size() == 1 && insns.get(0).getType() == InsnType.IF) { - return block; - } - // skip block - List successors = block.getSuccessors(); - if (successors.size() == 1) { - BlockNode next = successors.get(0); - boolean pass = true; - if (block.getInstructions().size() != 0) { - for (InsnNode insn : block.getInstructions()) { - RegisterArg res = insn.getResult(); - if (res == null) { - pass = false; - break; - } - List useList = res.getSVar().getUseList(); - if (useList.size() != 1) { - pass = false; - break; - } else { - InsnArg arg = useList.get(0); - InsnNode usePlace = arg.getParentInsn(); - if (!BlockUtils.blockContains(block, usePlace) - && !BlockUtils.blockContains(next, usePlace)) { - pass = false; - break; - } - } - } - } - if (pass) { - return getIfNode(next); - } - } - } - return null; - } - private BlockNode processSwitch(IRegion currentRegion, BlockNode block, SwitchNode insn, RegionStack stack) { SwitchRegion sw = new SwitchRegion(currentRegion, block); currentRegion.getSubBlocks().add(sw); @@ -749,7 +599,7 @@ public class RegionMaker { handler.getHandlerRegion().addAttr(excHandlerAttr); } - private void skipSimplePath(BlockNode block) { + static void skipSimplePath(BlockNode block) { while (block != null && block.getCleanSuccessors().size() < 2 && block.getPredecessors().size() == 1) { @@ -758,7 +608,7 @@ public class RegionMaker { } } - private static boolean isEqualPaths(BlockNode b1, BlockNode b2) { + static boolean isEqualPaths(BlockNode b1, BlockNode b2) { if (b1 == b2) { return true; } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ReturnVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ReturnVisitor.java index 03916183..c7944098 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ReturnVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ReturnVisitor.java @@ -43,7 +43,7 @@ public class ReturnVisitor extends AbstractVisitor { if (insns.size() == 1 && blockNotInLoop(mth, block) && noTrailInstructions(block)) { - insns.remove(insns.size() - 1); + insns.remove(0); block.remove(AFlag.RETURN); } } @@ -96,7 +96,7 @@ public class ReturnVisitor extends AbstractVisitor { /** * Check if container not contains instructions, * don't count one 'return' instruction (it will be removed later). - */ + */ private static boolean isEmpty(IContainer container) { if (container instanceof BlockNode) { BlockNode block = (BlockNode) container; @@ -104,7 +104,7 @@ public class ReturnVisitor extends AbstractVisitor { } else if (container instanceof IRegion) { IRegion region = (IRegion) container; for (IContainer block : region.getSubBlocks()) { - if(!isEmpty(block)) { + if (!isEmpty(block)) { return false; } } 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 d8b69623..e12fe227 100644 --- a/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java @@ -203,9 +203,14 @@ public class BlockUtils { } public static boolean isPathExists(BlockNode start, BlockNode end) { - if (start == end || end.isDominator(start)) { + if (start == end + || end.isDominator(start) + || start.getCleanSuccessors().contains(end)) { return true; } + if (start.getPredecessors().contains(end)) { + return false; + } return traverseSuccessorsUntil(start, end, new BitSet()); } @@ -257,6 +262,12 @@ public class BlockUtils { return end; } } + if (isPathExists(b1, b2)) { + return b2; + } + if (isPathExists(b2, b1)) { + return b1; + } return null; } 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 737ad5e5..a35ef1a1 100644 --- a/jadx-core/src/main/java/jadx/core/utils/RegionUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/RegionUtils.java @@ -39,7 +39,7 @@ public class RegionUtils { */ public static boolean hasExitBlock(IContainer container) { if (container instanceof BlockNode) { - return ((BlockNode) container).getSuccessors().size() == 0; + return ((BlockNode) container).getSuccessors().isEmpty(); } else if (container instanceof IRegion) { List blocks = ((IRegion) container).getSubBlocks(); return !blocks.isEmpty() diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions10.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions10.java new file mode 100644 index 00000000..696f486a --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions10.java @@ -0,0 +1,45 @@ +package jadx.tests.internal.conditions; + +import jadx.api.InternalJadxTest; +import jadx.core.dex.nodes.ClassNode; + +import org.junit.Test; + +import static jadx.tests.utils.JadxMatchers.containsOne; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +public class TestConditions10 extends InternalJadxTest { + + public static class TestCls { + + public void test(boolean a, int b) throws Exception { + if (a || b > 2) { + b++; + } + if (!a || (b >= 0 && b <= 11)) { + System.out.println("1"); + } else { + System.out.println("2"); + } + System.out.println("3"); + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + System.out.println(code); + + assertThat(code, not(containsString("return"))); + assertThat(code, containsOne("if (a || b > 2) {")); + assertThat(code, containsOne("b++;")); + assertThat(code, containsOne("if (!a || (b >= 0 && b <= 11)) {")); + assertThat(code, containsOne("System.out.println(\"1\");")); + assertThat(code, containsOne("} else {")); + assertThat(code, containsOne("System.out.println(\"2\");")); + assertThat(code, containsOne("System.out.println(\"3\");")); + } +} diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions11.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions11.java new file mode 100644 index 00000000..9f98b0f1 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions11.java @@ -0,0 +1,39 @@ +package jadx.tests.internal.conditions; + +import jadx.api.InternalJadxTest; +import jadx.core.dex.nodes.ClassNode; + +import org.junit.Test; + +import static jadx.tests.utils.JadxMatchers.containsOne; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +public class TestConditions11 extends InternalJadxTest { + + public static class TestCls { + + public void test(boolean a, int b) { + if (a || b > 2) { + f(); + } + } + + private void f() { + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + System.out.println(code); + + assertThat(code, containsOne("if (a || b > 2) {")); + assertThat(code, containsOne("f();")); + assertThat(code, not(containsString("return"))); + assertThat(code, not(containsString("else"))); + + } +} diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions12.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions12.java new file mode 100644 index 00000000..5906e669 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions12.java @@ -0,0 +1,71 @@ +package jadx.tests.internal.conditions; + +import jadx.api.InternalJadxTest; +import jadx.core.dex.nodes.ClassNode; + +import org.junit.Test; + +import static jadx.tests.utils.JadxMatchers.containsOne; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +public class TestConditions12 extends InternalJadxTest { + + public static class TestCls { + static boolean autoStop = true; + static boolean qualityReading = false; + static int lastValidRaw = -1; + + public static void main(String[] args) throws Exception { + int a = 5; + int b = 30; + dataProcess(a, b); + } + + public static void dataProcess(int raw, int quality) { + if (quality >= 10 && raw != 0) { + System.out.println("OK" + raw); + qualityReading = false; + } else if (raw == 0 || quality < 6 || !qualityReading) { + System.out.println("Not OK" + raw); + } else { + System.out.println("Quit OK" + raw); + } + if (quality < 30) { + int timeLeft = 30 - quality; + if (quality >= 10) { + System.out.println("Processing" + timeLeft); + } + } else { + System.out.println("Finish Processing"); + if (raw > 0) { + lastValidRaw = raw; + } + } + if (quality >= 30 && autoStop) { + System.out.println("Finished"); + } + if (!autoStop && lastValidRaw > -1 && quality < 10) { + System.out.println("Finished"); + } + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + System.out.println(code); + + assertThat(code, containsOne("if (quality >= 10 && raw != 0) {")); + assertThat(code, containsOne("} else if (raw == 0 || quality < 6 || !qualityReading) {")); + assertThat(code, containsOne("if (quality < 30) {")); + assertThat(code, containsOne("if (quality >= 10) {")); + assertThat(code, containsOne("if (raw > 0) {")); + assertThat(code, containsOne("if (quality >= 30 && autoStop) {")); + assertThat(code, containsOne("if (!autoStop && lastValidRaw > -1 && quality < 10) {")); + assertThat(code, not(containsString("return"))); + + } +} diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions13.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions13.java new file mode 100644 index 00000000..ca3b583e --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions13.java @@ -0,0 +1,43 @@ +package jadx.tests.internal.conditions; + +import jadx.api.InternalJadxTest; +import jadx.core.dex.nodes.ClassNode; + +import org.junit.Test; + +import static jadx.tests.utils.JadxMatchers.containsOne; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +public class TestConditions13 extends InternalJadxTest { + + public static class TestCls { + static boolean qualityReading; + + public static void dataProcess(int raw, int quality) { + if (quality >= 10 && raw != 0) { + System.out.println("OK" + raw); + qualityReading = false; + } else if (raw == 0 || quality < 6 || !qualityReading) { + System.out.println("Not OK" + raw); + } else { + System.out.println("Quit OK" + raw); + } + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + System.out.println(code); + + assertThat(code, containsOne("if (quality >= 10 && raw != 0) {")); + assertThat(code, containsOne("System.out.println(\"OK\" + raw);")); + assertThat(code, containsOne("qualityReading = false;")); + assertThat(code, containsOne("} else if (raw == 0 || quality < 6 || !qualityReading) {")); + assertThat(code, not(containsString("return"))); + + } +} diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions2.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions2.java index 8cf5c2b4..3f8381ba 100644 --- a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions2.java +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions2.java @@ -8,7 +8,6 @@ import org.junit.Test; public class TestConditions2 extends InternalJadxTest { public static class TestCls { - int c; String d; String f; diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions5.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions5.java index 40b09602..6204019f 100644 --- a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions5.java +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions5.java @@ -21,6 +21,18 @@ public class TestConditions5 extends InternalJadxTest { throw new AssertionError(a1 + " != " + a2); } } + + public static void assertEquals2(Object a1, Object a2) { + if (a1 != null) { + if (!a1.equals(a2)) { + throw new AssertionError(a1 + " != " + a2); + } + } else { + if (a2 != null) { + throw new AssertionError(a1 + " != " + a2); + } + } + } } @Test diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions9.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions9.java new file mode 100644 index 00000000..817b2a1f --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions9.java @@ -0,0 +1,37 @@ +package jadx.tests.internal.conditions; + +import jadx.api.InternalJadxTest; +import jadx.core.dex.nodes.ClassNode; + +import org.junit.Test; + +import static jadx.tests.utils.JadxMatchers.containsOne; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +public class TestConditions9 extends InternalJadxTest { + + public static class TestCls { + public void test(boolean a, int b) throws Exception { + if (!a || (b >= 0 && b <= 11)) { + System.out.println('1'); + } else { + System.out.println('2'); + } + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + System.out.println(code); + + assertThat(code, containsOne("if (!a || (b >= 0 && b <= 11)) {")); + assertThat(code, containsOne("System.out.println('1');")); + assertThat(code, containsOne("} else {")); + assertThat(code, containsOne("System.out.println('2');")); + assertThat(code, not(containsString("return;"))); + } +} diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestSimpleConditions.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestSimpleConditions.java new file mode 100644 index 00000000..4d501b85 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestSimpleConditions.java @@ -0,0 +1,32 @@ +package jadx.tests.internal.conditions; + +import jadx.api.InternalJadxTest; +import jadx.core.dex.nodes.ClassNode; + +import org.junit.Test; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.junit.Assert.assertThat; + +public class TestSimpleConditions extends InternalJadxTest { + + public static class TestCls { + public boolean test1(boolean[] a) { + return (a[0] && a[1] && a[2]) || (a[3] && a[4]); + } + + public boolean test2(boolean[] a) { + return a[0] || a[1] || a[2] || a[3]; + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + System.out.println(code); + + assertThat(code, containsString("return (a[0] && a[1] && a[2]) || (a[3] && a[4]);")); + assertThat(code, containsString("return a[0] || a[1] || a[2] || a[3];")); + } +} diff --git a/jadx-core/src/test/java/jadx/tests/internal/loops/TestLoopCondition2.java b/jadx-core/src/test/java/jadx/tests/internal/loops/TestLoopCondition2.java index aef25cb5..40a6597d 100644 --- a/jadx-core/src/test/java/jadx/tests/internal/loops/TestLoopCondition2.java +++ b/jadx-core/src/test/java/jadx/tests/internal/loops/TestLoopCondition2.java @@ -5,7 +5,7 @@ import jadx.core.dex.nodes.ClassNode; import org.junit.Test; -import static org.hamcrest.CoreMatchers.containsString; +import static jadx.tests.utils.JadxMatchers.containsOne; import static org.junit.Assert.assertThat; public class TestLoopCondition2 extends InternalJadxTest { @@ -27,6 +27,9 @@ public class TestLoopCondition2 extends InternalJadxTest { String code = cls.getCode().toString(); System.out.println(code); - assertThat(code, containsString("while (a && i < 10) {")); + assertThat(code, containsOne("int i = 0;")); + assertThat(code, containsOne("while (a && i < 10) {")); + assertThat(code, containsOne("i++;")); + assertThat(code, containsOne("return i;")); } } -- GitLab