From 88ccba166e9cbf36791cd9bdd0ec98560e2a57b8 Mon Sep 17 00:00:00 2001 From: Skylot Date: Fri, 8 Aug 2014 22:10:10 +0400 Subject: [PATCH] core: don't inline variables defined in 'try' and used in 'catch' --- jadx-core/src/main/java/jadx/core/Jadx.java | 6 +-- .../java/jadx/core/dex/nodes/MethodNode.java | 4 ++ .../dex/visitors/ConstInlinerVisitor.java | 49 +++++++++++++------ .../main/java/jadx/core/utils/BlockUtils.java | 23 +++++++++ .../jadx/core/utils/InstructionRemover.java | 8 ++- .../test/java/jadx/api/InternalJadxTest.java | 12 +++++ .../tests/internal/others/TestIfInTry.java | 1 - .../internal/trycatch/TestInlineInCatch.java | 46 +++++++++++++++++ 8 files changed, 128 insertions(+), 21 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/internal/trycatch/TestInlineInCatch.java diff --git a/jadx-core/src/main/java/jadx/core/Jadx.java b/jadx-core/src/main/java/jadx/core/Jadx.java index 4a6284685..0664a9783 100644 --- a/jadx-core/src/main/java/jadx/core/Jadx.java +++ b/jadx-core/src/main/java/jadx/core/Jadx.java @@ -57,13 +57,13 @@ public class Jadx { passes.add(new SSATransform()); passes.add(new DebugInfoVisitor()); passes.add(new TypeInference()); + if (args.isRawCFGOutput()) { + passes.add(new DotGraphVisitor(outDir, false, true)); + } 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 ModVisitor()); passes.add(new EnumVisitor()); diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/MethodNode.java b/jadx-core/src/main/java/jadx/core/dex/nodes/MethodNode.java index 3adff5d66..75087ce85 100644 --- a/jadx-core/src/main/java/jadx/core/dex/nodes/MethodNode.java +++ b/jadx-core/src/main/java/jadx/core/dex/nodes/MethodNode.java @@ -442,6 +442,10 @@ public class MethodNode extends LineAttrNode implements ILoadable { return exceptionHandlers.isEmpty(); } + public int getExceptionHandlersCount() { + return exceptionHandlers.size(); + } + /** * Return true if exists method with same name and arguments count */ diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/ConstInlinerVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/ConstInlinerVisitor.java index 262acbbdc..7e41cd547 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/ConstInlinerVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/ConstInlinerVisitor.java @@ -8,9 +8,11 @@ import jadx.core.dex.instructions.args.ArgType; import jadx.core.dex.instructions.args.InsnArg; import jadx.core.dex.instructions.args.LiteralArg; import jadx.core.dex.instructions.args.RegisterArg; +import jadx.core.dex.instructions.args.SSAVar; import jadx.core.dex.nodes.BlockNode; import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.nodes.MethodNode; +import jadx.core.utils.BlockUtils; import jadx.core.utils.InstructionRemover; import jadx.core.utils.exceptions.JadxException; @@ -24,35 +26,52 @@ public class ConstInlinerVisitor extends AbstractVisitor { if (mth.isNoCode()) { return; } + List toRemove = new ArrayList(); for (BlockNode block : mth.getBasicBlocks()) { - InstructionRemover remover = new InstructionRemover(mth, block); + toRemove.clear(); for (InsnNode insn : block.getInstructions()) { if (checkInsn(mth, block, insn)) { - remover.add(insn); + toRemove.add(insn); } } - remover.perform(); + if (!toRemove.isEmpty()) { + InstructionRemover.removeAll(mth, block, toRemove); + } } } private static boolean checkInsn(MethodNode mth, BlockNode block, InsnNode insn) { - if (insn.getType() == InsnType.CONST) { - InsnArg arg = insn.getArg(0); - if (arg.isLiteral()) { - ArgType resType = insn.getResult().getType(); - // make sure arg has correct type - if (!arg.getType().isTypeKnown()) { - arg.merge(resType); + if (insn.getType() != InsnType.CONST) { + return false; + } + InsnArg arg = insn.getArg(0); + if (!arg.isLiteral()) { + return false; + } + SSAVar sVar = insn.getResult().getSVar(); + if (mth.getExceptionHandlersCount() != 0) { + for (RegisterArg useArg : sVar.getUseList()) { + InsnNode parentInsn = useArg.getParentInsn(); + if (parentInsn != null) { + // TODO: speed up expensive operations + BlockNode useBlock = BlockUtils.getBlockByInsn(mth, parentInsn); + if (!BlockUtils.isCleanPathExists(block, useBlock)) { + return false; + } } - long lit = ((LiteralArg) arg).getLiteral(); - return replaceConst(mth, insn, lit); } } - return false; + ArgType resType = insn.getResult().getType(); + // make sure arg has correct type + if (!arg.getType().isTypeKnown()) { + arg.merge(resType); + } + long lit = ((LiteralArg) arg).getLiteral(); + return replaceConst(mth, sVar, lit); } - private static boolean replaceConst(MethodNode mth, InsnNode insn, long literal) { - List use = new ArrayList(insn.getResult().getSVar().getUseList()); + private static boolean replaceConst(MethodNode mth, SSAVar sVar, long literal) { + List use = new ArrayList(sVar.getUseList()); int replaceCount = 0; for (RegisterArg arg : use) { // if (arg.getSVar().isUsedInPhi()) { 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 34a9f8fe3..2b37f1130 100644 --- a/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java @@ -216,6 +216,29 @@ public class BlockUtils { return traverseSuccessorsUntil(start, end, new BitSet()); } + public static boolean isCleanPathExists(BlockNode start, BlockNode end) { + if (start == end || start.getCleanSuccessors().contains(end)) { + return true; + } + return traverseCleanSuccessorsUntil(start, end, new BitSet()); + } + + private static boolean traverseCleanSuccessorsUntil(BlockNode from, BlockNode until, BitSet visited) { + for (BlockNode s : from.getCleanSuccessors()) { + if (s == until) { + return true; + } + int id = s.getId(); + if (!visited.get(id) && !s.contains(AType.EXC_HANDLER)) { + visited.set(id); + if (traverseSuccessorsUntil(s, until, visited)) { + return true; + } + } + } + return false; + } + public static boolean isOnlyOnePathExists(BlockNode start, BlockNode end) { if (start == end) { return true; diff --git a/jadx-core/src/main/java/jadx/core/utils/InstructionRemover.java b/jadx-core/src/main/java/jadx/core/utils/InstructionRemover.java index f0a781e3b..8be17aca5 100644 --- a/jadx-core/src/main/java/jadx/core/utils/InstructionRemover.java +++ b/jadx-core/src/main/java/jadx/core/utils/InstructionRemover.java @@ -47,7 +47,7 @@ public class InstructionRemover { if (toRemove.isEmpty()) { return; } - removeAll(insns, toRemove); + removeAll(mth, insns, toRemove); toRemove.clear(); } @@ -86,7 +86,7 @@ public class InstructionRemover { // Don't use 'insns.removeAll(toRemove)' because it will remove instructions by content // and here can be several instructions with same content - private void removeAll(List insns, List toRemove) { + private static void removeAll(MethodNode mth, List insns, List toRemove) { for (InsnNode rem : toRemove) { unbindInsn(mth, rem); int insnsCount = insns.size(); @@ -119,6 +119,10 @@ public class InstructionRemover { } } + public static void removeAll(MethodNode mth, BlockNode block, List insns) { + removeAll(mth, block.getInstructions(), insns); + } + public static void remove(MethodNode mth, BlockNode block, int index) { List instructions = block.getInstructions(); unbindInsn(mth, instructions.get(index)); diff --git a/jadx-core/src/test/java/jadx/api/InternalJadxTest.java b/jadx-core/src/test/java/jadx/api/InternalJadxTest.java index 961ad4f40..68e8ec050 100644 --- a/jadx-core/src/test/java/jadx/api/InternalJadxTest.java +++ b/jadx-core/src/test/java/jadx/api/InternalJadxTest.java @@ -29,6 +29,7 @@ import static org.junit.Assert.fail; public abstract class InternalJadxTest extends TestUtils { protected boolean outputCFG = false; + protected boolean isFallback = false; protected boolean deleteTmpJar = true; protected String outDir = "test-out-tmp"; @@ -76,6 +77,11 @@ public abstract class InternalJadxTest extends TestUtils { public boolean isRawCFGOutput() { return outputCFG; } + + @Override + public boolean isFallbackMode() { + return isFallback; + } }, new File(outDir)); } @@ -136,6 +142,12 @@ public abstract class InternalJadxTest extends TestUtils { this.outputCFG = true; } + // Use only for debug purpose + @Deprecated + protected void setFallback() { + this.isFallback = true; + } + // Use only for debug purpose @Deprecated protected void notDeleteTmpJar() { diff --git a/jadx-core/src/test/java/jadx/tests/internal/others/TestIfInTry.java b/jadx-core/src/test/java/jadx/tests/internal/others/TestIfInTry.java index 8d55e548c..8d49e0d16 100644 --- a/jadx-core/src/test/java/jadx/tests/internal/others/TestIfInTry.java +++ b/jadx-core/src/test/java/jadx/tests/internal/others/TestIfInTry.java @@ -41,7 +41,6 @@ public class TestIfInTry extends InternalJadxTest { @Test public void test() { - setOutputCFG(); ClassNode cls = getClassNode(TestCls.class); String code = cls.getCode().toString(); System.out.println(code); diff --git a/jadx-core/src/test/java/jadx/tests/internal/trycatch/TestInlineInCatch.java b/jadx-core/src/test/java/jadx/tests/internal/trycatch/TestInlineInCatch.java new file mode 100644 index 000000000..bb3661d50 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/internal/trycatch/TestInlineInCatch.java @@ -0,0 +1,46 @@ +package jadx.tests.internal.trycatch; + +import jadx.api.InternalJadxTest; +import jadx.core.dex.nodes.ClassNode; + +import java.io.File; + +import org.junit.Test; + +import static jadx.tests.utils.JadxMatchers.containsOne; +import static org.junit.Assert.assertThat; + +public class TestInlineInCatch extends InternalJadxTest { + + public static class TestCls { + private File dir; + + public int test() { + File output = null; + try { + output = File.createTempFile("f", "a", dir); + return 0; + } catch (Exception e) { + if (output != null) { + output.delete(); + } + return 2; + } + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + System.out.println(code); + + assertThat(code, containsOne("File output = null;")); + assertThat(code, containsOne("output = File.createTempFile(\"f\", \"a\", ")); + assertThat(code, containsOne("return 0;")); + assertThat(code, containsOne("} catch (Exception e) {")); + assertThat(code, containsOne("if (output != null) {")); + assertThat(code, containsOne("output.delete();")); + assertThat(code, containsOne("return 2;")); + } +}