From 95f9ab035db6ec0e9ed67eafaef8d48b73f12304 Mon Sep 17 00:00:00 2001 From: Skylot Date: Sat, 1 Dec 2018 13:28:28 +0300 Subject: [PATCH] fix: inline constants in chained move instructions (#399) --- .../core/dex/visitors/ConstInlineVisitor.java | 117 ++++++++++-------- .../test/java/jadx/tests/api/SmaliTest.java | 4 + .../integration/types/TestConstInline.java | 38 ++++++ .../test/smali/types/TestConstInline.smali | 72 +++++++++++ 4 files changed, 177 insertions(+), 54 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/types/TestConstInline.java create mode 100644 jadx-core/src/test/smali/types/TestConstInline.smali diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/ConstInlineVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/ConstInlineVisitor.java index 121c243f5..635cecc25 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/ConstInlineVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/ConstInlineVisitor.java @@ -22,6 +22,7 @@ import jadx.core.dex.visitors.ssa.SSATransform; import jadx.core.dex.visitors.typeinference.TypeInferenceVisitor; import jadx.core.utils.InstructionRemover; import jadx.core.utils.exceptions.JadxException; +import jadx.core.utils.exceptions.JadxOverflowException; @JadxVisitor( name = "Constants Inline", @@ -40,21 +41,23 @@ public class ConstInlineVisitor extends AbstractVisitor { for (BlockNode block : mth.getBasicBlocks()) { toRemove.clear(); for (InsnNode insn : block.getInstructions()) { - if (checkInsn(mth, insn)) { - toRemove.add(insn); - } + checkInsn(mth, insn, toRemove); } InstructionRemover.removeAll(mth, block, toRemove); } } - private static boolean checkInsn(MethodNode mth, InsnNode insn) { - if (insn.getType() != InsnType.CONST || insn.contains(AFlag.DONT_INLINE)) { - return false; + private static void checkInsn(MethodNode mth, InsnNode insn, List toRemove) { + if (insn.contains(AFlag.DONT_INLINE)) { + return; + } + InsnType insnType = insn.getType(); + if (insnType != InsnType.CONST && insnType != InsnType.MOVE) { + return; } InsnArg arg = insn.getArg(0); if (!arg.isLiteral()) { - return false; + return; } long lit = ((LiteralArg) arg).getLiteral(); @@ -66,9 +69,9 @@ public class ConstInlineVisitor extends AbstractVisitor { assignInsn.add(AFlag.DONT_INLINE); } } - return false; + return; } - return replaceConst(mth, insn, lit); + replaceConst(mth, insn, lit, toRemove); } /** @@ -98,54 +101,60 @@ public class ConstInlineVisitor extends AbstractVisitor { return false; } - private static boolean replaceConst(MethodNode mth, InsnNode constInsn, long literal) { - SSAVar sVar = constInsn.getResult().getSVar(); - List use = new ArrayList<>(sVar.getUseList()); + private static void replaceConst(MethodNode mth, InsnNode constInsn, long literal, List toRemove) { + SSAVar ssaVar = constInsn.getResult().getSVar(); + List useList = new ArrayList<>(ssaVar.getUseList()); int replaceCount = 0; - for (RegisterArg arg : use) { - InsnNode useInsn = arg.getParentInsn(); - if (useInsn == null - || useInsn.getType() == InsnType.PHI - || useInsn.getType() == InsnType.MERGE - ) { - continue; - } - LiteralArg litArg; - ArgType argType = arg.getType(); - if (argType.isObject() && literal != 0) { - argType = ArgType.NARROW_NUMBERS; - } - if (use.size() == 1 || arg.isTypeImmutable()) { - // arg used only in one place - litArg = InsnArg.lit(literal, argType); - } else if (useInsn.getType() == InsnType.MOVE - && !useInsn.getResult().getType().isTypeKnown()) { - // save type for 'move' instructions (hard to find type in chains of 'move') - litArg = InsnArg.lit(literal, argType); - } else { - // in most cases type not equal arg.getType() - // just set unknown type and run type fixer - litArg = InsnArg.lit(literal, ArgType.UNKNOWN); - } - if (useInsn.replaceArg(arg, litArg)) { - litArg.setType(arg.getInitType()); + for (RegisterArg arg : useList) { + if (replaceArg(mth, arg, literal, constInsn, toRemove)) { replaceCount++; - if (useInsn.getType() == InsnType.RETURN) { - useInsn.setSourceLine(constInsn.getSourceLine()); - } - - FieldNode f = null; - ArgType litArgType = litArg.getType(); - if (litArgType.isTypeKnown()) { - f = mth.getParentClass().getConstFieldByLiteralArg(litArg); - } else if (litArgType.contains(PrimitiveType.INT)) { - f = mth.getParentClass().getConstField((int) literal, false); - } - if (f != null) { - litArg.wrapInstruction(new IndexInsnNode(InsnType.SGET, f.getFieldInfo(), 0)); - } } } - return replaceCount == use.size(); + if (replaceCount == useList.size()) { + toRemove.add(constInsn); + } + } + + private static boolean replaceArg(MethodNode mth, RegisterArg arg, long literal, InsnNode constInsn, List toRemove) { + InsnNode useInsn = arg.getParentInsn(); + if (useInsn == null) { + return false; + } + InsnType insnType = useInsn.getType(); + if (insnType == InsnType.PHI || insnType == InsnType.MERGE) { + return false; + } + ArgType argType = arg.getInitType(); + if (argType.isObject() && literal != 0) { + argType = ArgType.NARROW_NUMBERS; + } + LiteralArg litArg = InsnArg.lit(literal, argType); + if (!useInsn.replaceArg(arg, litArg)) { + return false; + } + // arg replaced, made some optimizations + litArg.setType(arg.getInitType()); + + FieldNode fieldNode = null; + ArgType litArgType = litArg.getType(); + if (litArgType.isTypeKnown()) { + fieldNode = mth.getParentClass().getConstFieldByLiteralArg(litArg); + } else if (litArgType.contains(PrimitiveType.INT)) { + fieldNode = mth.getParentClass().getConstField((int) literal, false); + } + if (fieldNode != null) { + litArg.wrapInstruction(new IndexInsnNode(InsnType.SGET, fieldNode.getFieldInfo(), 0)); + } + + if (insnType == InsnType.RETURN) { + useInsn.setSourceLine(constInsn.getSourceLine()); + } else if (insnType == InsnType.MOVE) { + try { + replaceConst(mth, useInsn, literal, toRemove); + } catch (StackOverflowError e) { + throw new JadxOverflowException("Stack overflow at const inline visitor"); + } + } + return true; } } diff --git a/jadx-core/src/test/java/jadx/tests/api/SmaliTest.java b/jadx-core/src/test/java/jadx/tests/api/SmaliTest.java index 75d507ca4..e77e80b51 100644 --- a/jadx-core/src/test/java/jadx/tests/api/SmaliTest.java +++ b/jadx-core/src/test/java/jadx/tests/api/SmaliTest.java @@ -24,6 +24,10 @@ public abstract class SmaliTest extends IntegrationTest { return getClassNodeFromSmali(path + File.separatorChar + clsName, clsName); } + protected ClassNode getClassNodeFromSmaliWithPkg(String pkg, String clsName) { + return getClassNodeFromSmali(pkg + File.separatorChar + clsName, pkg + '.' + clsName); + } + protected ClassNode getClassNodeFromSmali(String clsName) { return getClassNodeFromSmali(clsName, clsName); } diff --git a/jadx-core/src/test/java/jadx/tests/integration/types/TestConstInline.java b/jadx-core/src/test/java/jadx/tests/integration/types/TestConstInline.java new file mode 100644 index 000000000..16051c1af --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/types/TestConstInline.java @@ -0,0 +1,38 @@ +package jadx.tests.integration.types; + +import org.junit.Test; + +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.SmaliTest; + +import static jadx.tests.api.utils.JadxMatchers.containsOne; +import static org.junit.Assert.assertThat; + +public class TestConstInline extends SmaliTest { + +// private static String test(boolean b) { +// List list; +// String str; +// if (b) { +// list = Collections.emptyList(); +// str = "1"; +// } else { +// list = null; +// str = list; // not correct assign in java but bytecode allow it +// } +// return use(list, str); +// } +// +// private static String use(List list, String str) { +// return list + str; +// } + + @Test + public void test() { + ClassNode cls = getClassNodeFromSmaliWithPkg("types", "TestConstInline"); + String code = cls.getCode().toString(); + + assertThat(code, containsOne("list = null;")); + assertThat(code, containsOne("str = null;")); + } +} diff --git a/jadx-core/src/test/smali/types/TestConstInline.smali b/jadx-core/src/test/smali/types/TestConstInline.smali new file mode 100644 index 000000000..0a6f2e90d --- /dev/null +++ b/jadx-core/src/test/smali/types/TestConstInline.smali @@ -0,0 +1,72 @@ +.class public Ltypes/TestConstInline; +.super Ljava/lang/Object; + +.method private static test(Z)Ljava/lang/String; + .registers 4 + .param p0, "b" # Z + + if-eqz p0, :cond_d + invoke-static {}, Ltypes/TestConstInline;->list()Ljava/util/List; + move-result-object v0 + const-string v1, "1" + goto :goto_return + + :cond_d + const/4 v2, 0x0 + # chained move instead zero const loading + move v0, v2 + move v1, v0 + goto :goto_return + + :goto_return + invoke-static {v0, v1}, Ltypes/TestConstInline;->use(Ljava/util/List;Ljava/lang/String;)Ljava/lang/String; + move-result-object v2 + return-object v2 +.end method + +.method private static use(Ljava/util/List;Ljava/lang/String;)Ljava/lang/String; + .registers 3 + .annotation system Ldalvik/annotation/Signature; + value = { + "(", + "Ljava/util/List", + "<", + "Ljava/lang/String;", + ">;", + "Ljava/lang/String;", + ")", + "Ljava/lang/String;" + } + .end annotation + + new-instance v0, Ljava/lang/StringBuilder; + invoke-direct {v0}, Ljava/lang/StringBuilder;->()V + + invoke-virtual {v0, p0}, Ljava/lang/StringBuilder;->append(Ljava/lang/Object;)Ljava/lang/StringBuilder; + move-result-object v0 + + invoke-virtual {v0, p1}, Ljava/lang/StringBuilder;->append(Ljava/lang/String;)Ljava/lang/StringBuilder; + move-result-object v0 + + invoke-virtual {v0}, Ljava/lang/StringBuilder;->toString()Ljava/lang/String; + move-result-object v0 + + return-object v0 +.end method + +.method private static list()Ljava/util/List; + .registers 1 + .annotation system Ldalvik/annotation/Signature; + value = { + "()", + "Ljava/util/List", + "<", + "Ljava/lang/String;", + ">;" + } + .end annotation + + invoke-static {}, Ljava/util/Collections;->emptyList()Ljava/util/List; + move-result-object v0 + return-object v0 +.end method