From 424a8ffaf4a123beba126c46960c1433c879a67d Mon Sep 17 00:00:00 2001 From: Skylot Date: Fri, 5 Jul 2019 17:13:12 +0300 Subject: [PATCH] fix: inline constant strings (#685) --- .../core/dex/instructions/args/InsnArg.java | 6 + .../dex/instructions/args/InsnWrapArg.java | 5 + .../core/dex/visitors/ConstInlineVisitor.java | 109 +++++++++++------- .../visitors/shrink/CodeShrinkVisitor.java | 86 +++++++------- .../TestTernaryOneBranchInConstructor2.java | 37 ++++++ .../TestTernaryOneBranchInConstructor2.smali | 42 +++++++ 6 files changed, 201 insertions(+), 84 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/conditions/TestTernaryOneBranchInConstructor2.java create mode 100644 jadx-core/src/test/smali/conditions/TestTernaryOneBranchInConstructor2.smali diff --git a/jadx-core/src/main/java/jadx/core/dex/instructions/args/InsnArg.java b/jadx-core/src/main/java/jadx/core/dex/instructions/args/InsnArg.java index 3e578d530..dd91473de 100644 --- a/jadx-core/src/main/java/jadx/core/dex/instructions/args/InsnArg.java +++ b/jadx-core/src/main/java/jadx/core/dex/instructions/args/InsnArg.java @@ -159,6 +159,12 @@ public abstract class InsnArg extends Typed { return contains(AFlag.THIS); } + protected InsnArg copyCommonParams(InsnArg copy) { + copy.copyAttributesFrom(this); + copy.setParentInsn(parentInsn); + return copy; + } + public InsnArg duplicate() { return this; } diff --git a/jadx-core/src/main/java/jadx/core/dex/instructions/args/InsnWrapArg.java b/jadx-core/src/main/java/jadx/core/dex/instructions/args/InsnWrapArg.java index d65eea995..d19f78c53 100644 --- a/jadx-core/src/main/java/jadx/core/dex/instructions/args/InsnWrapArg.java +++ b/jadx-core/src/main/java/jadx/core/dex/instructions/args/InsnWrapArg.java @@ -27,6 +27,11 @@ public final class InsnWrapArg extends InsnArg { this.parentInsn = parentInsn; } + @Override + public InsnArg duplicate() { + return copyCommonParams(new InsnWrapArg(wrappedInsn.copy())); + } + @Override public boolean isInsnWrap() { return true; 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 9c6e2f32e..4cc8bcbc1 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 @@ -4,6 +4,7 @@ import java.util.ArrayList; import java.util.List; import jadx.core.dex.attributes.AFlag; +import jadx.core.dex.instructions.ConstStringNode; import jadx.core.dex.instructions.IndexInsnNode; import jadx.core.dex.instructions.InsnType; import jadx.core.dex.instructions.InvokeNode; @@ -51,36 +52,53 @@ public class ConstInlineVisitor extends AbstractVisitor { } private static void checkInsn(MethodNode mth, InsnNode insn, List toRemove) { - if (insn.contains(AFlag.DONT_INLINE) || insn.contains(AFlag.DONT_GENERATE)) { + if (insn.contains(AFlag.DONT_INLINE) + || insn.contains(AFlag.DONT_GENERATE) + || insn.getResult() == null) { return; } - InsnType insnType = insn.getType(); - if (insnType != InsnType.CONST && insnType != InsnType.MOVE) { - return; - } - InsnArg arg = insn.getArg(0); - if (!arg.isLiteral()) { - return; - } - long lit = ((LiteralArg) arg).getLiteral(); SSAVar sVar = insn.getResult().getSVar(); - if (lit == 0 && checkObjectInline(sVar)) { - if (sVar.getUseCount() == 1) { - InsnNode assignInsn = insn.getResult().getAssignInsn(); - if (assignInsn != null) { - assignInsn.add(AFlag.DONT_INLINE); - } + InsnArg constArg; + + InsnType insnType = insn.getType(); + if (insnType == InsnType.CONST || insnType == InsnType.MOVE) { + constArg = insn.getArg(0); + if (!constArg.isLiteral()) { + return; } + long lit = ((LiteralArg) constArg).getLiteral(); + if (lit == 0 && checkObjectInline(sVar)) { + if (sVar.getUseCount() == 1) { + InsnNode assignInsn = insn.getResult().getAssignInsn(); + if (assignInsn != null) { + assignInsn.add(AFlag.DONT_INLINE); + } + } + return; + } + } else if (insnType == InsnType.CONST_STR) { + if (sVar.isUsedInPhi()) { + return; + } + String s = ((ConstStringNode) insn).getString(); + FieldNode f = mth.getParentClass().getConstField(s); + if (f == null) { + constArg = InsnArg.wrapArg(insn.copy()); + } else { + InsnNode constGet = new IndexInsnNode(InsnType.SGET, f.getFieldInfo(), 0); + constGet.setResult(insn.getResult().duplicate()); + constArg = InsnArg.wrapArg(constGet); + } + } else { return; } - if (checkForFinallyBlock(sVar)) { return; } // all check passed, run replace - replaceConst(mth, insn, lit, toRemove); + replaceConst(mth, insn, constArg, toRemove); } private static boolean checkForFinallyBlock(SSAVar sVar) { @@ -131,12 +149,12 @@ public class ConstInlineVisitor extends AbstractVisitor { return false; } - private static int replaceConst(MethodNode mth, InsnNode constInsn, long literal, List toRemove) { + private static int replaceConst(MethodNode mth, InsnNode constInsn, InsnArg constArg, List toRemove) { SSAVar ssaVar = constInsn.getResult().getSVar(); List useList = new ArrayList<>(ssaVar.getUseList()); int replaceCount = 0; for (RegisterArg arg : useList) { - if (replaceArg(mth, arg, literal, constInsn, toRemove)) { + if (replaceArg(mth, arg, constArg, constInsn, toRemove)) { replaceCount++; } } @@ -146,7 +164,7 @@ public class ConstInlineVisitor extends AbstractVisitor { return replaceCount; } - private static boolean replaceArg(MethodNode mth, RegisterArg arg, long literal, InsnNode constInsn, List toRemove) { + private static boolean replaceArg(MethodNode mth, RegisterArg arg, InsnArg constArg, InsnNode constInsn, List toRemove) { InsnNode useInsn = arg.getParentInsn(); if (useInsn == null) { return false; @@ -155,33 +173,40 @@ public class ConstInlineVisitor extends AbstractVisitor { if (insnType == InsnType.PHI) { 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 (constArg.isLiteral()) { + long literal = ((LiteralArg) constArg).getLiteral(); + 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)); + } + } else { + if (!useInsn.replaceArg(arg, constArg.duplicate())) { + return false; + } + } if (insnType == InsnType.RETURN) { useInsn.setSourceLine(constInsn.getSourceLine()); } else if (insnType == InsnType.MOVE) { try { - replaceConst(mth, useInsn, literal, toRemove); + replaceConst(mth, useInsn, constArg, toRemove); } catch (StackOverflowError e) { throw new JadxOverflowException("Stack overflow at const inline visitor"); } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/shrink/CodeShrinkVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/shrink/CodeShrinkVisitor.java index b5d7618a5..c70c02f56 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/shrink/CodeShrinkVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/shrink/CodeShrinkVisitor.java @@ -57,48 +57,11 @@ public class CodeShrinkVisitor extends AbstractVisitor { List wrapList = new ArrayList<>(); for (ArgsInfo argsInfo : argsList) { List args = argsInfo.getArgs(); - if (args.isEmpty()) { - continue; - } - ListIterator it = args.listIterator(args.size()); - while (it.hasPrevious()) { - RegisterArg arg = it.previous(); - // if (arg.getName() != null) { - // continue; - // } - SSAVar sVar = arg.getSVar(); - // allow inline only one use arg - if (sVar == null - || sVar.getVariableUseCount() != 1 - || sVar.contains(AFlag.DONT_INLINE)) { - continue; - } - InsnNode assignInsn = sVar.getAssign().getParentInsn(); - if (assignInsn == null || assignInsn.contains(AFlag.DONT_INLINE)) { - continue; - } - List useList = sVar.getUseList(); - if (!useList.isEmpty()) { - InsnNode parentInsn = useList.get(0).getParentInsn(); - if (parentInsn != null && parentInsn.contains(AFlag.DONT_GENERATE)) { - continue; - } - } - - int assignPos = insnList.getIndex(assignInsn); - if (assignPos != -1) { - WrapInfo wrapInfo = argsInfo.checkInline(assignPos, arg); - if (wrapInfo != null) { - wrapList.add(wrapInfo); - } - } else { - // another block - BlockNode assignBlock = BlockUtils.getBlockByInsn(mth, assignInsn); - if (assignBlock != null - && assignInsn != arg.getParentInsn() - && canMoveBetweenBlocks(assignInsn, assignBlock, block, argsInfo.getInsn())) { - inline(arg, assignInsn, assignBlock); - } + if (!args.isEmpty()) { + ListIterator it = args.listIterator(args.size()); + while (it.hasPrevious()) { + RegisterArg arg = it.previous(); + checkInline(mth, block, insnList, wrapList, argsInfo, arg); } } } @@ -109,6 +72,45 @@ public class CodeShrinkVisitor extends AbstractVisitor { } } + private static void checkInline(MethodNode mth, BlockNode block, InsnList insnList, + List wrapList, ArgsInfo argsInfo, RegisterArg arg) { + SSAVar sVar = arg.getSVar(); + if (sVar == null || sVar.contains(AFlag.DONT_INLINE)) { + return; + } + // allow inline only one use arg + if (sVar.getVariableUseCount() != 1) { + return; + } + InsnNode assignInsn = sVar.getAssign().getParentInsn(); + if (assignInsn == null || assignInsn.contains(AFlag.DONT_INLINE)) { + return; + } + List useList = sVar.getUseList(); + if (!useList.isEmpty()) { + InsnNode parentInsn = useList.get(0).getParentInsn(); + if (parentInsn != null && parentInsn.contains(AFlag.DONT_GENERATE)) { + return; + } + } + + int assignPos = insnList.getIndex(assignInsn); + if (assignPos != -1) { + WrapInfo wrapInfo = argsInfo.checkInline(assignPos, arg); + if (wrapInfo != null) { + wrapList.add(wrapInfo); + } + } else { + // another block + BlockNode assignBlock = BlockUtils.getBlockByInsn(mth, assignInsn); + if (assignBlock != null + && assignInsn != arg.getParentInsn() + && canMoveBetweenBlocks(assignInsn, assignBlock, block, argsInfo.getInsn())) { + inline(arg, assignInsn, assignBlock); + } + } + } + private static boolean inline(RegisterArg arg, InsnNode insn, BlockNode block) { InsnNode parentInsn = arg.getParentInsn(); if (parentInsn != null && parentInsn.getType() == InsnType.RETURN) { diff --git a/jadx-core/src/test/java/jadx/tests/integration/conditions/TestTernaryOneBranchInConstructor2.java b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestTernaryOneBranchInConstructor2.java new file mode 100644 index 000000000..55523cd7d --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestTernaryOneBranchInConstructor2.java @@ -0,0 +1,37 @@ +package jadx.tests.integration.conditions; + +import org.junit.jupiter.api.Test; + +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.SmaliTest; + +import static jadx.tests.api.utils.JadxMatchers.containsOne; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; + +public class TestTernaryOneBranchInConstructor2 extends SmaliTest { + + // @formatter:off + /* + public class A { + public A(String str, String str2, String str3, boolean z) {} + + public A(String str, String str2, String str3, boolean z, int i, int i2) { + this(str, (i & 2) != 0 ? "" : str2, (i & 4) != 0 ? "" : str3, (i & 8) != 0 ? false : z); + } + } + */ + // @formatter:on + + @Test + public void test() { + ClassNode cls = getClassNodeFromSmali(); + String code = cls.getCode().toString(); + + assertThat(code, containsOne("this(str, (i & 2) != 0 ? \"\" : str2," + + " (i & 4) != 0 ? \"\" : str3," + + " (i & 8) != 0 ? false : z);")); + assertThat(code, not(containsString("//"))); + } +} diff --git a/jadx-core/src/test/smali/conditions/TestTernaryOneBranchInConstructor2.smali b/jadx-core/src/test/smali/conditions/TestTernaryOneBranchInConstructor2.smali new file mode 100644 index 000000000..74bb31a24 --- /dev/null +++ b/jadx-core/src/test/smali/conditions/TestTernaryOneBranchInConstructor2.smali @@ -0,0 +1,42 @@ +.class public final Lconditions/TestTernaryOneBranchInConstructor2; +.super Ljava/lang/Object; + + +.method public constructor (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Z)V + .locals 1 + + invoke-direct {p0}, Ljava/lang/Object;->()V + + return-void +.end method + +.method public synthetic constructor (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;ZII)V + .locals 1 + + and-int/lit8 p6, p5, 0x2 + + const-string v0, "" + + if-eqz p6, :cond_0 + + move-object p2, v0 + + :cond_0 + and-int/lit8 p6, p5, 0x4 + + if-eqz p6, :cond_1 + + move-object p3, v0 + + :cond_1 + and-int/lit8 p5, p5, 0x8 + + if-eqz p5, :cond_2 + + const/4 p4, 0x0 + + :cond_2 + invoke-direct {p0, p1, p2, p3, p4}, Lconditions/TestTernaryOneBranchInConstructor2;->(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Z)V + + return-void +.end method