From c8de7b97ddbd0becb8a70fa27be487842b3dee27 Mon Sep 17 00:00:00 2001 From: Skylot Date: Sun, 21 Jul 2019 19:45:22 +0300 Subject: [PATCH] fix: instead commenting move constructor call to the top (#704) --- .../instructions/mods/ConstructorInsn.java | 2 +- .../core/dex/visitors/PrepareForCodeGen.java | 126 +++++++----------- .../main/java/jadx/core/utils/BlockUtils.java | 8 ++ .../others/TestInsnsBeforeSuper.java | 2 +- .../others/TestInsnsBeforeThis.java | 2 +- 5 files changed, 62 insertions(+), 78 deletions(-) diff --git a/jadx-core/src/main/java/jadx/core/dex/instructions/mods/ConstructorInsn.java b/jadx-core/src/main/java/jadx/core/dex/instructions/mods/ConstructorInsn.java index cfdfea86f..d4dc6b6bb 100644 --- a/jadx-core/src/main/java/jadx/core/dex/instructions/mods/ConstructorInsn.java +++ b/jadx-core/src/main/java/jadx/core/dex/instructions/mods/ConstructorInsn.java @@ -15,7 +15,7 @@ public class ConstructorInsn extends InsnNode implements CallMthInterface { private final CallType callType; private final RegisterArg instanceArg; - private enum CallType { + public enum CallType { CONSTRUCTOR, // just new instance SUPER, // super call THIS, // call constructor from other constructor diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/PrepareForCodeGen.java b/jadx-core/src/main/java/jadx/core/dex/visitors/PrepareForCodeGen.java index 7e9c41aae..6cc675f04 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/PrepareForCodeGen.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/PrepareForCodeGen.java @@ -1,9 +1,16 @@ package jadx.core.dex.visitors; +import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Set; + +import org.jetbrains.annotations.Nullable; import jadx.core.dex.attributes.AFlag; +import jadx.core.dex.attributes.AType; +import jadx.core.dex.attributes.nodes.DeclareVariablesAttr; import jadx.core.dex.instructions.ArithNode; import jadx.core.dex.instructions.ArithOp; import jadx.core.dex.instructions.InsnType; @@ -13,16 +20,16 @@ import jadx.core.dex.instructions.args.RegisterArg; import jadx.core.dex.instructions.mods.ConstructorInsn; import jadx.core.dex.instructions.mods.TernaryInsn; import jadx.core.dex.nodes.BlockNode; -import jadx.core.dex.nodes.IBlock; -import jadx.core.dex.nodes.IRegion; +import jadx.core.dex.nodes.InsnContainer; import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.nodes.MethodNode; +import jadx.core.dex.regions.Region; import jadx.core.dex.regions.conditions.IfCondition; import jadx.core.dex.regions.conditions.IfCondition.Mode; -import jadx.core.dex.visitors.regions.AbstractRegionVisitor; -import jadx.core.dex.visitors.regions.DepthRegionTraversal; import jadx.core.dex.visitors.regions.variables.ProcessVariables; import jadx.core.dex.visitors.shrink.CodeShrinkVisitor; +import jadx.core.utils.BlockUtils; +import jadx.core.utils.InsnList; import jadx.core.utils.exceptions.JadxException; /** @@ -52,7 +59,7 @@ public class PrepareForCodeGen extends AbstractVisitor { removeParenthesis(block); modifyArith(block); } - commentOutInsnsInConstructor(mth); + moveConstructorInConstructor(mth); } private static void removeInstructions(BlockNode block) { @@ -179,15 +186,52 @@ public class PrepareForCodeGen extends AbstractVisitor { } } - private void commentOutInsnsInConstructor(MethodNode mth) { + /** + * Check that 'super' or 'this' call in constructor is a first instruction. + * Otherwise move to top and add a warning if code breaks. + */ + private void moveConstructorInConstructor(MethodNode mth) { if (mth.isConstructor()) { ConstructorInsn constrInsn = searchConstructorCall(mth); if (constrInsn != null && !constrInsn.contains(AFlag.DONT_GENERATE)) { - DepthRegionTraversal.traverse(mth, new ConstructorRegionVisitor(constrInsn)); + Region oldRootRegion = mth.getRegion(); + boolean firstInsn = BlockUtils.isFirstInsn(mth, constrInsn); + DeclareVariablesAttr declVarsAttr = oldRootRegion.get(AType.DECLARE_VARIABLES); + if (firstInsn && declVarsAttr == null) { + // move not needed + return; + } + + // move constructor instruction to new root region + String callType = constrInsn.getCallType().toString().toLowerCase(); + BlockNode blockByInsn = BlockUtils.getBlockByInsn(mth, constrInsn); + if (blockByInsn == null) { + mth.addWarn("Failed to move " + callType + " instruction to top"); + return; + } + InsnList.remove(blockByInsn, constrInsn); + + Region region = new Region(null); + region.add(new InsnContainer(Collections.singletonList(constrInsn))); + region.add(oldRootRegion); + mth.setRegion(region); + + if (!firstInsn) { + Set regArgs = new HashSet<>(); + constrInsn.getRegisterArgs(regArgs); + regArgs.remove(mth.getThisArg()); + regArgs.removeAll(mth.getArguments(false)); + if (!regArgs.isEmpty()) { + mth.addWarn("Illegal instructions before constructor call"); + } else { + mth.addComment("JADX INFO: " + callType + " call moved to the top of the method (can break code semantics)"); + } + } } } } + @Nullable private ConstructorInsn searchConstructorCall(MethodNode mth) { for (BlockNode block : mth.getBasicBlocks()) { for (InsnNode insn : block.getInstructions()) { @@ -202,72 +246,4 @@ public class PrepareForCodeGen extends AbstractVisitor { } return null; } - - private static final class ConstructorRegionVisitor extends AbstractRegionVisitor { - private final ConstructorInsn constrInsn; - private int regionDepth; - private boolean found; - private boolean brokenCode; - private int commentedCount; - - public ConstructorRegionVisitor(ConstructorInsn constrInsn) { - this.constrInsn = constrInsn; - } - - @Override - public boolean enterRegion(MethodNode mth, IRegion region) { - if (found) { - return false; - } - regionDepth++; - return true; - } - - @Override - public void leaveRegion(MethodNode mth, IRegion region) { - if (!found) { - regionDepth--; - region.add(AFlag.COMMENT_OUT); - commentedCount++; - } - } - - @Override - public void processBlock(MethodNode mth, IBlock container) { - if (found) { - return; - } - for (InsnNode insn : container.getInstructions()) { - if (insn == constrInsn) { - found = true; - addMethodMsg(mth); - break; - } - insn.add(AFlag.COMMENT_OUT); - commentedCount++; - if (!brokenCode) { - RegisterArg resArg = insn.getResult(); - if (resArg != null) { - for (RegisterArg arg : resArg.getSVar().getUseList()) { - if (arg.getParentInsn() == constrInsn) { - brokenCode = true; - break; - } - } - } - } - } - } - - private void addMethodMsg(MethodNode mth) { - if (commentedCount > 0) { - String msg = "Illegal instructions before constructor call commented (this can break semantics)"; - if (brokenCode || regionDepth > 1) { - mth.addWarn(msg); - } else { - mth.addComment("JADX WARN: " + msg); - } - } - } - } } 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 48d58bd7b..c4b2bd124 100644 --- a/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java @@ -574,6 +574,14 @@ public class BlockUtils { return insns; } + public static boolean isFirstInsn(MethodNode mth, InsnNode insn) { + BlockNode enterBlock = mth.getEnterBlock(); + if (enterBlock == null || enterBlock.getInstructions().isEmpty()) { + return false; + } + return enterBlock.getInstructions().get(0) == insn; + } + /** * Replace insn by index i in block, * for proper copy attributes, assume attributes are not overlap diff --git a/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeSuper.java b/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeSuper.java index c0185d659..f8fd01e1e 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeSuper.java +++ b/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeSuper.java @@ -37,6 +37,6 @@ public class TestInsnsBeforeSuper extends SmaliTest { ClassNode cls = getClassNodeFromSmaliFiles("B"); String code = cls.getCode().toString(); - assertThat(code, containsOne("// checkNull(str);")); + assertThat(code, containsOne("checkNull(str);")); } } diff --git a/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeThis.java b/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeThis.java index 0fe08b2e2..c425766e3 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeThis.java +++ b/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeThis.java @@ -35,6 +35,6 @@ public class TestInsnsBeforeThis extends SmaliTest { ClassNode cls = getClassNodeFromSmali(); String code = cls.getCode().toString(); - assertThat(code, containsOne("// checkNull(str);")); + assertThat(code, containsOne("checkNull(str);")); } }