From 4f02864e12dc16dbaa7f5985a51338abd22aadc9 Mon Sep 17 00:00:00 2001 From: Skylot Date: Sat, 26 May 2018 19:57:02 +0300 Subject: [PATCH] core: fix variable declaration in else-if chain (#273) --- jadx-core/src/main/java/jadx/core/Jadx.java | 8 +-- .../src/main/java/jadx/core/ProcessClass.java | 4 +- .../java/jadx/core/codegen/RegionGen.java | 21 +++---- .../java/jadx/core/dex/nodes/IRegion.java | 2 + .../jadx/core/dex/regions/AbstractRegion.java | 7 +++ .../java/jadx/core/dex/regions/Region.java | 5 +- .../core/dex/regions/conditions/IfRegion.java | 4 +- .../core/dex/visitors/DepthTraversal.java | 8 +-- .../dex/visitors/regions/CleanRegions.java | 36 +++-------- .../regions/DepthRegionTraversal.java | 4 +- .../dex/visitors/regions/IfRegionVisitor.java | 57 ++++++++--------- .../visitors/regions/ProcessVariables.java | 15 +++-- .../main/java/jadx/core/utils/DebugUtils.java | 6 +- .../variables/TestVariablesIfElseChain.java | 63 +++++++++++++++++++ 14 files changed, 144 insertions(+), 96 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/variables/TestVariablesIfElseChain.java diff --git a/jadx-core/src/main/java/jadx/core/Jadx.java b/jadx-core/src/main/java/jadx/core/Jadx.java index eb0ffea86..37d27f9fe 100644 --- a/jadx-core/src/main/java/jadx/core/Jadx.java +++ b/jadx-core/src/main/java/jadx/core/Jadx.java @@ -94,10 +94,6 @@ public class Jadx { passes.add(new SimplifyVisitor()); passes.add(new CheckRegions()); - if (args.isCfgOutput()) { - passes.add(DotGraphVisitor.dumpRegions()); - } - passes.add(new MethodInlineVisitor()); passes.add(new ExtractFieldInit()); passes.add(new ClassModifier()); @@ -106,6 +102,10 @@ public class Jadx { passes.add(new LoopRegionVisitor()); passes.add(new ProcessVariables()); + if (args.isCfgOutput()) { + passes.add(DotGraphVisitor.dumpRegions()); + } + passes.add(new DependencyCollector()); passes.add(new RenameVisitor()); diff --git a/jadx-core/src/main/java/jadx/core/ProcessClass.java b/jadx-core/src/main/java/jadx/core/ProcessClass.java index 671d529da..42214dd13 100644 --- a/jadx-core/src/main/java/jadx/core/ProcessClass.java +++ b/jadx-core/src/main/java/jadx/core/ProcessClass.java @@ -56,8 +56,6 @@ public final class ProcessClass { } private static void processDependencies(ClassNode cls, List passes) { - for (ClassNode depCls : cls.getDependencies()) { - process(depCls, passes, null); - } + cls.getDependencies().forEach(depCls -> process(depCls, passes, null)); } } diff --git a/jadx-core/src/main/java/jadx/core/codegen/RegionGen.java b/jadx-core/src/main/java/jadx/core/codegen/RegionGen.java index 6f8c84973..f6ad43c50 100644 --- a/jadx-core/src/main/java/jadx/core/codegen/RegionGen.java +++ b/jadx-core/src/main/java/jadx/core/codegen/RegionGen.java @@ -134,17 +134,16 @@ public class RegionGen extends InsnGen { * Connect if-else-if block */ private boolean connectElseIf(CodeWriter code, IContainer els) throws CodegenException { - if (!els.contains(AFlag.ELSE_IF_CHAIN)) { - return false; - } - if (!(els instanceof Region)) { - return false; - } - List subBlocks = ((Region) els).getSubBlocks(); - if (subBlocks.size() == 1 - && subBlocks.get(0) instanceof IfRegion) { - makeIf((IfRegion) subBlocks.get(0), code, false); - return true; + if (els.contains(AFlag.ELSE_IF_CHAIN) && els instanceof Region) { + List subBlocks = ((Region) els).getSubBlocks(); + if (subBlocks.size() == 1) { + IContainer elseBlock = subBlocks.get(0); + if (elseBlock instanceof IfRegion) { + declareVars(code, elseBlock); + makeIf((IfRegion) elseBlock, code, false); + return true; + } + } } return false; } diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/IRegion.java b/jadx-core/src/main/java/jadx/core/dex/nodes/IRegion.java index 720e32baf..71d1918f5 100644 --- a/jadx-core/src/main/java/jadx/core/dex/nodes/IRegion.java +++ b/jadx-core/src/main/java/jadx/core/dex/nodes/IRegion.java @@ -6,6 +6,8 @@ public interface IRegion extends IContainer { IRegion getParent(); + void setParent(IRegion parent); + List getSubBlocks(); boolean replaceSubBlock(IContainer oldBlock, IContainer newBlock); diff --git a/jadx-core/src/main/java/jadx/core/dex/regions/AbstractRegion.java b/jadx-core/src/main/java/jadx/core/dex/regions/AbstractRegion.java index f74a4ec62..3ae310c7b 100644 --- a/jadx-core/src/main/java/jadx/core/dex/regions/AbstractRegion.java +++ b/jadx-core/src/main/java/jadx/core/dex/regions/AbstractRegion.java @@ -21,6 +21,7 @@ public abstract class AbstractRegion extends AttrNode implements IRegion { return parent; } + @Override public void setParent(IRegion parent) { this.parent = parent; } @@ -30,4 +31,10 @@ public abstract class AbstractRegion extends AttrNode implements IRegion { LOG.warn("Replace sub block not supported for class \"{}\"", this.getClass()); return false; } + + public void updateParent(IContainer container, IRegion newParent) { + if (container instanceof IRegion) { + ((IRegion) container).setParent(newParent); + } + } } diff --git a/jadx-core/src/main/java/jadx/core/dex/regions/Region.java b/jadx-core/src/main/java/jadx/core/dex/regions/Region.java index a8da4ed09..31d547f29 100644 --- a/jadx-core/src/main/java/jadx/core/dex/regions/Region.java +++ b/jadx-core/src/main/java/jadx/core/dex/regions/Region.java @@ -21,9 +21,7 @@ public final class Region extends AbstractRegion { } public void add(IContainer region) { - if (region instanceof AbstractRegion) { - ((AbstractRegion) region).setParent(this); - } + updateParent(region, this); blocks.add(region); } @@ -32,6 +30,7 @@ public final class Region extends AbstractRegion { int i = blocks.indexOf(oldBlock); if (i != -1) { blocks.set(i, newBlock); + updateParent(newBlock, this); return true; } return false; diff --git a/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfRegion.java b/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfRegion.java index e1a9b83fc..694703940 100644 --- a/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfRegion.java +++ b/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfRegion.java @@ -105,10 +105,12 @@ public final class IfRegion extends AbstractRegion implements IBranchRegion { public boolean replaceSubBlock(IContainer oldBlock, IContainer newBlock) { if (oldBlock == thenRegion) { thenRegion = newBlock; + updateParent(thenRegion, this); return true; } if (oldBlock == elseRegion) { elseRegion = newBlock; + updateParent(elseRegion, this); return true; } return false; @@ -128,6 +130,6 @@ public final class IfRegion extends AbstractRegion implements IBranchRegion { @Override public String toString() { - return "IF " + header + " then " + thenRegion + " else " + elseRegion; + return "IF " + header + " then (" + thenRegion + ") else (" + elseRegion + ")"; } } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/DepthTraversal.java b/jadx-core/src/main/java/jadx/core/dex/visitors/DepthTraversal.java index 3bab51335..42da47df7 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/DepthTraversal.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/DepthTraversal.java @@ -10,12 +10,8 @@ public class DepthTraversal { public static void visit(IDexTreeVisitor visitor, ClassNode cls) { try { if (visitor.visit(cls)) { - for (ClassNode inCls : cls.getInnerClasses()) { - visit(visitor, inCls); - } - for (MethodNode mth : cls.getMethods()) { - visit(visitor, mth); - } + cls.getInnerClasses().forEach(inCls -> visit(visitor, inCls)); + cls.getMethods().forEach(mth -> visit(visitor, mth)); } } catch (Exception e) { ErrorsCounter.classError(cls, diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CleanRegions.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CleanRegions.java index 5ac99b6f4..cd3b72e7e 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CleanRegions.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CleanRegions.java @@ -1,21 +1,11 @@ package jadx.core.dex.visitors.regions; -import java.util.Iterator; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import jadx.core.dex.nodes.BlockNode; -import jadx.core.dex.nodes.IContainer; import jadx.core.dex.nodes.IRegion; import jadx.core.dex.nodes.MethodNode; import jadx.core.dex.regions.Region; public class CleanRegions { - private static final Logger LOG = LoggerFactory.getLogger(CleanRegions.class); - - private CleanRegions() { - } public static void process(MethodNode mth) { if (mth.isNoCode() || mth.getBasicBlocks().isEmpty()) { @@ -24,27 +14,21 @@ public class CleanRegions { IRegionVisitor removeEmptyBlocks = new AbstractRegionVisitor() { @Override public boolean enterRegion(MethodNode mth, IRegion region) { - if (!(region instanceof Region)) { - return true; - } - - for (Iterator it = region.getSubBlocks().iterator(); it.hasNext(); ) { - IContainer container = it.next(); - if (container instanceof BlockNode) { - BlockNode block = (BlockNode) container; - if (block.getInstructions().isEmpty()) { - try { - it.remove(); - } catch (UnsupportedOperationException e) { - LOG.warn("Can't remove block: {} from: {}, mth: {}", block, region, mth); - } + if (region instanceof Region) { + region.getSubBlocks().removeIf(container -> { + if (container instanceof BlockNode) { + BlockNode block = (BlockNode) container; + return block.getInstructions().isEmpty(); } - } - + return false; + }); } return true; } }; DepthRegionTraversal.traverse(mth, removeEmptyBlocks); } + + private CleanRegions() { + } } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraversal.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraversal.java index d67e77cb8..cbdda2eae 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraversal.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraversal.java @@ -54,9 +54,7 @@ public class DepthRegionTraversal { } else if (container instanceof IRegion) { IRegion region = (IRegion) container; if (visitor.enterRegion(mth, region)) { - for (IContainer subCont : region.getSubBlocks()) { - traverseInternal(mth, visitor, subCont); - } + region.getSubBlocks().forEach(subCont -> traverseInternal(mth, visitor, subCont)); } visitor.leaveRegion(mth, region); } 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 05b6940e8..8429ad7f8 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 @@ -4,7 +4,6 @@ import java.util.List; import jadx.core.dex.attributes.AFlag; import jadx.core.dex.instructions.args.ArgType; -import jadx.core.dex.nodes.IBlock; import jadx.core.dex.nodes.IContainer; import jadx.core.dex.nodes.IRegion; import jadx.core.dex.nodes.MethodNode; @@ -17,18 +16,22 @@ import jadx.core.utils.RegionUtils; import static jadx.core.utils.RegionUtils.insnsCount; -public class IfRegionVisitor extends AbstractVisitor implements IRegionVisitor, IRegionIterativeVisitor { +public class IfRegionVisitor extends AbstractVisitor { private static final TernaryVisitor TERNARY_VISITOR = new TernaryVisitor(); + private static final ProcessIfRegionVisitor PROCESS_IF_REGION_VISITOR = new ProcessIfRegionVisitor(); + private static final RemoveRedundantElseVisitor REMOVE_REDUNDANT_ELSE_VISITOR = new RemoveRedundantElseVisitor(); @Override public void visit(MethodNode mth) { - // collapse ternary operators DepthRegionTraversal.traverseIterative(mth, TERNARY_VISITOR); - DepthRegionTraversal.traverse(mth, this); - DepthRegionTraversal.traverseIterative(mth, this); + DepthRegionTraversal.traverse(mth, PROCESS_IF_REGION_VISITOR); + DepthRegionTraversal.traverseIterative(mth, REMOVE_REDUNDANT_ELSE_VISITOR); } + /** + * Collapse ternary operators + */ private static class TernaryVisitor implements IRegionIterativeVisitor { @Override public boolean visitRegion(MethodNode mth, IRegion region) { @@ -37,35 +40,28 @@ public class IfRegionVisitor extends AbstractVisitor implements IRegionVisitor, } } - @Override - public boolean enterRegion(MethodNode mth, IRegion region) { - if (region instanceof IfRegion) { - processIfRegion(mth, (IfRegion) region); + private static class ProcessIfRegionVisitor extends AbstractRegionVisitor { + @Override + public boolean enterRegion(MethodNode mth, IRegion region) { + if (region instanceof IfRegion) { + IfRegion ifRegion = (IfRegion) region; + simplifyIfCondition(ifRegion); + moveReturnToThenBlock(mth, ifRegion); + moveBreakToThenBlock(ifRegion); + markElseIfChains(ifRegion); + } + return true; } - return true; } - @Override - public boolean visitRegion(MethodNode mth, IRegion region) { - if (region instanceof IfRegion) { - return removeRedundantElseBlock(mth, (IfRegion) region); + private static class RemoveRedundantElseVisitor implements IRegionIterativeVisitor { + @Override + public boolean visitRegion(MethodNode mth, IRegion region) { + if (region instanceof IfRegion) { + return removeRedundantElseBlock(mth, (IfRegion) region); + } + return false; } - return false; - } - - @Override - public void processBlock(MethodNode mth, IBlock container) { - } - - @Override - public void leaveRegion(MethodNode mth, IRegion region) { - } - - private static void processIfRegion(MethodNode mth, IfRegion ifRegion) { - simplifyIfCondition(ifRegion); - moveReturnToThenBlock(mth, ifRegion); - moveBreakToThenBlock(ifRegion); - markElseIfChains(ifRegion); } private static void simplifyIfCondition(IfRegion ifRegion) { @@ -90,7 +86,6 @@ public class IfRegionVisitor extends AbstractVisitor implements IRegionVisitor, && !isIfRegion(elseRegion)) { invertIfRegion(ifRegion); } - } } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessVariables.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessVariables.java index bf185eb69..eaa6b4a50 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessVariables.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessVariables.java @@ -64,7 +64,7 @@ public class ProcessVariables extends AbstractVisitor { @Override public String toString() { - return regNum + " " + type; + return "r" + regNum + ":" + type; } } @@ -119,7 +119,7 @@ public class ProcessVariables extends AbstractVisitor { public CollectUsageRegionVisitor(Map usageMap) { this.usageMap = usageMap; - args = new ArrayList<>(); + this.args = new ArrayList<>(); } @Override @@ -177,8 +177,10 @@ public class ProcessVariables extends AbstractVisitor { if (mth.isNoCode()) { return; } + List mthArguments = mth.getArguments(true); + Map usageMap = new LinkedHashMap<>(); - for (RegisterArg arg : mth.getArguments(true)) { + for (RegisterArg arg : mthArguments) { addToUsageMap(arg, usageMap); } @@ -187,8 +189,7 @@ public class ProcessVariables extends AbstractVisitor { DepthRegionTraversal.traverse(mth, collect); // reduce assigns map - List mthArgs = mth.getArguments(true); - for (RegisterArg arg : mthArgs) { + for (RegisterArg arg : mthArguments) { usageMap.remove(new Variable(arg)); } @@ -350,6 +351,10 @@ public class ProcessVariables extends AbstractVisitor { } } } + // can't declare in else-if chain between 'else' and next 'if' + if (region.contains(AFlag.ELSE_IF_CHAIN)) { + return false; + } return isAllRegionsAfter(region, pos, u.getAssigns(), regionsOrder) && isAllRegionsAfter(region, pos, u.getUseRegions(), regionsOrder); } diff --git a/jadx-core/src/main/java/jadx/core/utils/DebugUtils.java b/jadx-core/src/main/java/jadx/core/utils/DebugUtils.java index b7ae061b9..b3e7e8cfe 100644 --- a/jadx-core/src/main/java/jadx/core/utils/DebugUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/DebugUtils.java @@ -76,7 +76,7 @@ public class DebugUtils { } private static void printRegion(MethodNode mth, IRegion region, String indent, boolean printInsns) { - LOG.debug("{}{}", indent, region); + LOG.debug("{}{} {}", indent, region, region.getAttributesString()); indent += "| "; for (IContainer container : region.getSubBlocks()) { if (container instanceof IRegion) { @@ -99,9 +99,9 @@ public class DebugUtils { CodeWriter code = new CodeWriter(); ig.makeInsn(insn, code); String insnStr = code.toString().substring(CodeWriter.NL.length()); - LOG.debug("{} - {}", indent, insnStr); + LOG.debug("{}> {}", indent, insnStr); } catch (CodegenException e) { - LOG.debug("{} - {}", indent, insn); + LOG.debug("{}>!! {}", indent, insn); } } } diff --git a/jadx-core/src/test/java/jadx/tests/integration/variables/TestVariablesIfElseChain.java b/jadx-core/src/test/java/jadx/tests/integration/variables/TestVariablesIfElseChain.java new file mode 100644 index 000000000..4a4ba32ff --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/variables/TestVariablesIfElseChain.java @@ -0,0 +1,63 @@ +package jadx.tests.integration.variables; + +import org.junit.Test; + +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.IntegrationTest; + +import static jadx.tests.api.utils.JadxMatchers.containsOne; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertThat; + +public class TestVariablesIfElseChain extends IntegrationTest { + + public static class TestCls { + String used; + + public String test(int a) { + if (a == 0) { + use("zero"); + } else if (a == 1) { + String r = m(a); + if (r != null) { + use(r); + } + } else if (a == 2) { + String r = m(a); + if (r != null) { + use(r); + } + } else { + return "miss"; + } + return null; + } + + public String m(int a) { + return "hit" + a; + } + + public void use(String s) { + used = s; + } + + public void check() { + test(0); + assertThat(used, is("zero")); + test(1); + assertThat(used, is("hit1")); + test(2); + assertThat(used, is("hit2")); + assertThat(test(5), is("miss")); + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + + assertThat(code, containsOne("return \"miss\";")); + // and compilable + } +}