From 691bf8facad42db189b5c1ac9a0fe9a603b00435 Mon Sep 17 00:00:00 2001 From: Skylot Date: Mon, 7 Sep 2020 10:35:40 +0100 Subject: [PATCH] fix: checks for casts in field access, move method inline to visitor (#962) Signed-off-by: Skylot --- jadx-core/src/main/java/jadx/core/Consts.java | 1 + jadx-core/src/main/java/jadx/core/Jadx.java | 7 +- .../src/main/java/jadx/core/ProcessClass.java | 6 +- .../main/java/jadx/core/codegen/InsnGen.java | 67 --------- .../java/jadx/core/dex/attributes/AFlag.java | 4 +- .../attributes/nodes/MethodInlineAttr.java | 29 +++- .../java/jadx/core/dex/nodes/ClassNode.java | 27 ++++ .../java/jadx/core/dex/nodes/LoadStage.java | 7 + .../core/dex/visitors/FixAccessModifiers.java | 2 +- .../jadx/core/dex/visitors/InlineMethods.java | 132 ++++++++++++++++++ ...Visitor.java => MarkMethodsForInline.java} | 82 ++++++----- .../jadx/core/dex/visitors/ModVisitor.java | 53 ++++++- .../visitors/typeinference/TypeCompare.java | 5 + .../java/jadx/core/utils/InsnRemover.java | 4 +- .../integration/types/TestFieldCast.java | 61 ++++++++ 15 files changed, 373 insertions(+), 114 deletions(-) create mode 100644 jadx-core/src/main/java/jadx/core/dex/nodes/LoadStage.java create mode 100644 jadx-core/src/main/java/jadx/core/dex/visitors/InlineMethods.java rename jadx-core/src/main/java/jadx/core/dex/visitors/{MethodInlineVisitor.java => MarkMethodsForInline.java} (61%) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/types/TestFieldCast.java diff --git a/jadx-core/src/main/java/jadx/core/Consts.java b/jadx-core/src/main/java/jadx/core/Consts.java index 794b125b0..60ca33a46 100644 --- a/jadx-core/src/main/java/jadx/core/Consts.java +++ b/jadx-core/src/main/java/jadx/core/Consts.java @@ -2,6 +2,7 @@ package jadx.core; public class Consts { public static final boolean DEBUG = false; + public static final boolean DEBUG_WITH_ERRORS = false; // TODO: fix errors public static final boolean DEBUG_USAGE = false; public static final boolean DEBUG_TYPE_INFERENCE = false; public static final boolean DEBUG_OVERLOADED_CASTS = false; diff --git a/jadx-core/src/main/java/jadx/core/Jadx.java b/jadx-core/src/main/java/jadx/core/Jadx.java index f7eba51b1..fba17d704 100644 --- a/jadx-core/src/main/java/jadx/core/Jadx.java +++ b/jadx-core/src/main/java/jadx/core/Jadx.java @@ -25,8 +25,9 @@ import jadx.core.dex.visitors.FixAccessModifiers; import jadx.core.dex.visitors.GenericTypesVisitor; import jadx.core.dex.visitors.IDexTreeVisitor; import jadx.core.dex.visitors.InitCodeVariables; +import jadx.core.dex.visitors.InlineMethods; import jadx.core.dex.visitors.MarkFinallyVisitor; -import jadx.core.dex.visitors.MethodInlineVisitor; +import jadx.core.dex.visitors.MarkMethodsForInline; import jadx.core.dex.visitors.MethodInvokeVisitor; import jadx.core.dex.visitors.ModVisitor; import jadx.core.dex.visitors.MoveInlineVisitor; @@ -119,6 +120,7 @@ public class Jadx { passes.add(new DebugInfoApplyVisitor()); } + passes.add(new InlineMethods()); passes.add(new GenericTypesVisitor()); passes.add(new ShadowFieldVisitor()); passes.add(new DeboxingVisitor()); @@ -144,9 +146,10 @@ public class Jadx { passes.add(new FixAccessModifiers()); passes.add(new ProcessAnonymous()); passes.add(new ClassModifier()); - passes.add(new MethodInlineVisitor()); passes.add(new LoopRegionVisitor()); + passes.add(new MarkMethodsForInline()); + passes.add(new ProcessVariables()); passes.add(new PrepareForCodeGen()); if (args.isCfgOutput()) { diff --git a/jadx-core/src/main/java/jadx/core/ProcessClass.java b/jadx-core/src/main/java/jadx/core/ProcessClass.java index 9ff97eb29..d08d9c908 100644 --- a/jadx-core/src/main/java/jadx/core/ProcessClass.java +++ b/jadx-core/src/main/java/jadx/core/ProcessClass.java @@ -59,7 +59,11 @@ public final class ProcessClass { cls.setState(NOT_LOADED); } try { - cls.getDependencies().forEach(ProcessClass::process); + for (ClassNode depCls : cls.getDependencies()) { + depCls.startProcessStage(); + process(depCls); + } + cls.startCodegenStage(); process(cls); ICodeInfo code = CodeGen.generate(cls); diff --git a/jadx-core/src/main/java/jadx/core/codegen/InsnGen.java b/jadx-core/src/main/java/jadx/core/codegen/InsnGen.java index 88931a2c7..21ec41797 100644 --- a/jadx-core/src/main/java/jadx/core/codegen/InsnGen.java +++ b/jadx-core/src/main/java/jadx/core/codegen/InsnGen.java @@ -1,6 +1,5 @@ package jadx.core.codegen; -import java.util.ArrayList; import java.util.EnumSet; import java.util.Iterator; import java.util.List; @@ -10,14 +9,12 @@ import org.jetbrains.annotations.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import jadx.core.Consts; import jadx.core.deobf.NameMapper; import jadx.core.dex.attributes.AFlag; import jadx.core.dex.attributes.AType; import jadx.core.dex.attributes.nodes.FieldReplaceAttr; import jadx.core.dex.attributes.nodes.GenericInfoAttr; import jadx.core.dex.attributes.nodes.LoopLabelAttr; -import jadx.core.dex.attributes.nodes.MethodInlineAttr; import jadx.core.dex.attributes.nodes.SkipMethodArgsAttr; import jadx.core.dex.info.ClassInfo; import jadx.core.dex.info.FieldInfo; @@ -43,7 +40,6 @@ import jadx.core.dex.instructions.args.InsnArg; import jadx.core.dex.instructions.args.InsnWrapArg; import jadx.core.dex.instructions.args.LiteralArg; import jadx.core.dex.instructions.args.Named; -import jadx.core.dex.instructions.args.NamedArg; import jadx.core.dex.instructions.args.RegisterArg; import jadx.core.dex.instructions.args.SSAVar; import jadx.core.dex.instructions.mods.ConstructorInsn; @@ -705,12 +701,7 @@ public class InsnGen { private void makeInvoke(InvokeNode insn, CodeWriter code) throws CodegenException { MethodInfo callMth = insn.getCallMth(); - - // inline method MethodNode callMthNode = mth.root().deepResolveMethod(callMth); - if (callMthNode != null && inlineMethod(callMthNode, insn, code)) { - return; - } int k = 0; InvokeType type = insn.getInvokeType(); @@ -844,64 +835,6 @@ public class InsnGen { return true; } - private boolean inlineMethod(MethodNode callMthNode, InvokeNode insn, CodeWriter code) throws CodegenException { - MethodInlineAttr mia = callMthNode.get(AType.METHOD_INLINE); - if (mia == null) { - return false; - } - InsnNode inl = mia.getInsn(); - if (Consts.DEBUG) { - code.add("/* inline method: ").add(callMthNode.toString()).add("*/").startLine(); - } - if (forceAssign(inl, insn, callMthNode)) { - ArgType varType = callMthNode.getReturnType(); - useType(code, varType); - code.add(' '); - code.add(mgen.getNameGen().assignNamedArg(new NamedArg("unused", varType))); - code.add(" = "); - } - if (callMthNode.getMethodInfo().getArgumentsTypes().isEmpty()) { - makeInsn(inl, code, Flags.BODY_ONLY); - } else { - // remap args - InsnArg[] regs = new InsnArg[callMthNode.getRegsCount()]; - int[] regNums = mia.getArgsRegNums(); - for (int i = 0; i < regNums.length; i++) { - InsnArg arg = insn.getArg(i); - regs[regNums[i]] = arg; - } - // replace args - InsnNode inlCopy = inl.copyWithoutResult(); - List inlArgs = new ArrayList<>(); - inlCopy.getRegisterArgs(inlArgs); - for (RegisterArg r : inlArgs) { - int regNum = r.getRegNum(); - if (regNum >= regs.length) { - LOG.warn("Unknown register number {} in method call: {} from {}", r, callMthNode, mth); - } else { - InsnArg repl = regs[regNum]; - if (repl == null) { - LOG.warn("Not passed register {} in method call: {} from {}", r, callMthNode, mth); - } else { - inlCopy.replaceArg(r, repl); - } - } - } - makeInsn(inlCopy, code, Flags.BODY_ONLY); - } - return true; - } - - private boolean forceAssign(InsnNode inlineInsn, InvokeNode parentInsn, MethodNode callMthNode) { - if (parentInsn.getResult() != null) { - return false; - } - if (parentInsn.contains(AFlag.WRAPPED)) { - return false; - } - return !callMthNode.isVoidReturn(); - } - private void makeTernary(TernaryInsn insn, CodeWriter code, Set state) throws CodegenException { boolean wrap = state.contains(Flags.BODY_ONLY); if (wrap) { diff --git a/jadx-core/src/main/java/jadx/core/dex/attributes/AFlag.java b/jadx-core/src/main/java/jadx/core/dex/attributes/AFlag.java index eab01e2f3..a7fd486e1 100644 --- a/jadx-core/src/main/java/jadx/core/dex/attributes/AFlag.java +++ b/jadx-core/src/main/java/jadx/core/dex/attributes/AFlag.java @@ -22,7 +22,9 @@ public enum AFlag { HIDDEN, // instruction used inside other instruction but not listed in args - RESTART_CODEGEN, + RESTART_CODEGEN, // codegen must be executed again + RELOAD_AT_CODEGEN_STAGE, // class can't be analyzed at 'process' stage => unload before 'codegen' stage + DONT_RENAME, // do not rename during deobfuscation ADDED_TO_REGION, diff --git a/jadx-core/src/main/java/jadx/core/dex/attributes/nodes/MethodInlineAttr.java b/jadx-core/src/main/java/jadx/core/dex/attributes/nodes/MethodInlineAttr.java index 272fb6ccd..27b6e428b 100644 --- a/jadx-core/src/main/java/jadx/core/dex/attributes/nodes/MethodInlineAttr.java +++ b/jadx-core/src/main/java/jadx/core/dex/attributes/nodes/MethodInlineAttr.java @@ -1,7 +1,10 @@ package jadx.core.dex.attributes.nodes; import java.util.List; +import java.util.Objects; +import jadx.core.Consts; +import jadx.core.dex.attributes.AFlag; import jadx.core.dex.attributes.AType; import jadx.core.dex.attributes.IAttribute; import jadx.core.dex.instructions.args.RegisterArg; @@ -10,7 +13,10 @@ import jadx.core.dex.nodes.MethodNode; public class MethodInlineAttr implements IAttribute { - public static void markForInline(MethodNode mth, InsnNode replaceInsn) { + private static final MethodInlineAttr INLINE_NOT_NEEDED = new MethodInlineAttr(null, null); + + public static MethodInlineAttr markForInline(MethodNode mth, InsnNode replaceInsn) { + Objects.requireNonNull(replaceInsn); List allArgRegs = mth.getAllArgRegs(); int argsCount = allArgRegs.size(); int[] regNums = new int[argsCount]; @@ -18,7 +24,19 @@ public class MethodInlineAttr implements IAttribute { RegisterArg reg = allArgRegs.get(i); regNums[i] = reg.getRegNum(); } - mth.addAttr(new MethodInlineAttr(replaceInsn, regNums)); + MethodInlineAttr mia = new MethodInlineAttr(replaceInsn, regNums); + mth.addAttr(mia); + if (Consts.DEBUG) { + mth.addAttr(AType.COMMENTS, "Removed for inline"); + } else { + mth.add(AFlag.DONT_GENERATE); + } + return mia; + } + + public static MethodInlineAttr inlineNotNeeded(MethodNode mth) { + mth.addAttr(INLINE_NOT_NEEDED); + return INLINE_NOT_NEEDED; } private final InsnNode insn; @@ -33,6 +51,10 @@ public class MethodInlineAttr implements IAttribute { this.argsRegNums = argsRegNums; } + public boolean notNeeded() { + return insn == null; + } + public InsnNode getInsn() { return insn; } @@ -48,6 +70,9 @@ public class MethodInlineAttr implements IAttribute { @Override public String toString() { + if (notNeeded()) { + return "INLINE_NOT_NEEDED"; + } return "INLINE: " + insn; } } diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/ClassNode.java b/jadx-core/src/main/java/jadx/core/dex/nodes/ClassNode.java index fe9518022..0dbb8da00 100644 --- a/jadx-core/src/main/java/jadx/core/dex/nodes/ClassNode.java +++ b/jadx-core/src/main/java/jadx/core/dex/nodes/ClassNode.java @@ -65,6 +65,7 @@ public class ClassNode extends NotificationAttrNode implements ILoadable, ICodeN private ClassNode parentClass; private volatile ProcessState state = ProcessState.NOT_LOADED; + private LoadStage loadStage = LoadStage.NONE; /** Top level classes used in this class (only for top level classes, empty for inners) */ private List dependencies = Collections.emptyList(); @@ -288,6 +289,7 @@ public class ClassNode extends NotificationAttrNode implements ILoadable, ICodeN fields.forEach(FieldNode::unloadAttributes); unloadAttributes(); setState(NOT_LOADED); + this.loadStage = LoadStage.NONE; this.smali = null; } @@ -577,6 +579,30 @@ public class ClassNode extends NotificationAttrNode implements ILoadable, ICodeN this.state = state; } + public LoadStage getLoadStage() { + return loadStage; + } + + public void startProcessStage() { + this.loadStage = LoadStage.PROCESS_STAGE; + } + + public void startCodegenStage() { + this.loadStage = LoadStage.CODEGEN_STAGE; + if (contains(AFlag.RELOAD_AT_CODEGEN_STAGE)) { + unload(); + remove(AFlag.RELOAD_AT_CODEGEN_STAGE); + } + } + + public void reloadAtCodegenStage() { + ClassNode topCls = this.getTopParentClass(); + if (topCls.getLoadStage() == LoadStage.CODEGEN_STAGE) { + throw new JadxRuntimeException("Class not yet loaded at codegen stage"); + } + topCls.add(AFlag.RELOAD_AT_CODEGEN_STAGE); + } + public List getDependencies() { return dependencies; } @@ -632,4 +658,5 @@ public class ClassNode extends NotificationAttrNode implements ILoadable, ICodeN public String toString() { return clsInfo.getFullName(); } + } diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/LoadStage.java b/jadx-core/src/main/java/jadx/core/dex/nodes/LoadStage.java new file mode 100644 index 000000000..bc9e36153 --- /dev/null +++ b/jadx-core/src/main/java/jadx/core/dex/nodes/LoadStage.java @@ -0,0 +1,7 @@ +package jadx.core.dex.nodes; + +public enum LoadStage { + NONE, + PROCESS_STAGE, // dependencies not yet loaded + CODEGEN_STAGE, // all dependencies loaded +} diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/FixAccessModifiers.java b/jadx-core/src/main/java/jadx/core/dex/visitors/FixAccessModifiers.java index 04d1c87a0..44fabb97b 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/FixAccessModifiers.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/FixAccessModifiers.java @@ -83,7 +83,7 @@ public class FixAccessModifiers extends AbstractVisitor { if (!accessFlags.isPublic()) { // if class is used in inlinable method => make it public for (MethodNode useMth : cls.getUseInMth()) { - boolean canInline = MethodInlineVisitor.canInline(useMth) || useMth.contains(AType.METHOD_INLINE); + boolean canInline = MarkMethodsForInline.canInline(useMth) || useMth.contains(AType.METHOD_INLINE); if (canInline && !useMth.getUseIn().isEmpty()) { return AccessFlags.PUBLIC; } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/InlineMethods.java b/jadx-core/src/main/java/jadx/core/dex/visitors/InlineMethods.java new file mode 100644 index 000000000..72e58ce11 --- /dev/null +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/InlineMethods.java @@ -0,0 +1,132 @@ +package jadx.core.dex.visitors; + +import java.util.ArrayList; +import java.util.List; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import jadx.core.dex.attributes.AFlag; +import jadx.core.dex.attributes.nodes.MethodInlineAttr; +import jadx.core.dex.info.MethodInfo; +import jadx.core.dex.instructions.InsnType; +import jadx.core.dex.instructions.InvokeNode; +import jadx.core.dex.instructions.args.ArgType; +import jadx.core.dex.instructions.args.InsnArg; +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.dex.visitors.typeinference.TypeInferenceVisitor; +import jadx.core.utils.BlockUtils; +import jadx.core.utils.exceptions.JadxException; +import jadx.core.utils.exceptions.JadxRuntimeException; + +@JadxVisitor( + name = "InlineMethods", + desc = "Inline methods (previously marked in MarkMethodsForInline)", + runAfter = TypeInferenceVisitor.class, + runBefore = ModVisitor.class +) +public class InlineMethods extends AbstractVisitor { + private static final Logger LOG = LoggerFactory.getLogger(InlineMethods.class); + + @Override + public void visit(MethodNode mth) throws JadxException { + if (mth.isNoCode()) { + return; + } + for (BlockNode block : mth.getBasicBlocks()) { + for (InsnNode insn : block.getInstructions()) { + if (insn.getType() == InsnType.INVOKE) { + processInvokeInsn(mth, block, ((InvokeNode) insn)); + } + } + } + } + + private void processInvokeInsn(MethodNode mth, BlockNode block, InvokeNode insn) { + MethodInfo callMthInfo = insn.getCallMth(); + MethodNode callMth = mth.root().deepResolveMethod(callMthInfo); + if (callMth == null) { + return; + } + try { + // TODO: sort inner classes process order by dependencies! + MethodInlineAttr mia = MarkMethodsForInline.process(callMth); + if (mia == null) { + // method not yet loaded => will retry at codegen stage + mth.getParentClass().reloadAtCodegenStage(); + return; + } + if (mia.notNeeded()) { + return; + } + inlineMethod(mth, callMth, mia, block, insn); + } catch (Exception e) { + throw new JadxRuntimeException("Failed to process method for inline", e); + } + } + + private void inlineMethod(MethodNode mth, MethodNode callMth, MethodInlineAttr mia, BlockNode block, InvokeNode insn) { + InsnNode inlCopy = mia.getInsn().copyWithoutResult(); + RegisterArg resultArg = insn.getResult(); + if (resultArg != null) { + inlCopy.setResult(resultArg.duplicate()); + } else if (isAssignNeeded(mia.getInsn(), insn, callMth)) { + // add fake result to make correct java expression (see test TestGetterInlineNegative) + inlCopy.setResult(makeFakeArg(mth, callMth.getReturnType(), "unused")); + } + if (!callMth.getMethodInfo().getArgumentsTypes().isEmpty()) { + // remap args + InsnArg[] regs = new InsnArg[callMth.getRegsCount()]; + int[] regNums = mia.getArgsRegNums(); + for (int i = 0; i < regNums.length; i++) { + InsnArg arg = insn.getArg(i); + regs[regNums[i]] = arg; + } + // replace args + List inlArgs = new ArrayList<>(); + inlCopy.getRegisterArgs(inlArgs); + for (RegisterArg r : inlArgs) { + int regNum = r.getRegNum(); + if (regNum >= regs.length) { + LOG.warn("Unknown register number {} in method call: {} from {}", r, callMth, mth); + } else { + InsnArg repl = regs[regNum]; + if (repl == null) { + LOG.warn("Not passed register {} in method call: {} from {}", r, callMth, mth); + } else { + inlCopy.replaceArg(r, repl); + } + } + } + } + if (!BlockUtils.replaceInsn(mth, block, insn, inlCopy)) { + mth.addWarnComment("Failed to inline method: " + callMth); + } + } + + private boolean isAssignNeeded(InsnNode inlineInsn, InvokeNode parentInsn, MethodNode callMthNode) { + if (parentInsn.getResult() != null) { + return false; + } + if (parentInsn.contains(AFlag.WRAPPED)) { + return false; + } + if (inlineInsn.getType() == InsnType.IPUT) { + return false; + } + return !callMthNode.isVoidReturn(); + } + + private RegisterArg makeFakeArg(MethodNode mth, ArgType varType, String name) { + RegisterArg fakeArg = RegisterArg.reg(0, varType); + SSAVar ssaVar = mth.makeNewSVar(fakeArg); + InitCodeVariables.initCodeVar(ssaVar); + fakeArg.setName(name); + ssaVar.setType(varType); + return fakeArg; + } +} diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/MethodInlineVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/MarkMethodsForInline.java similarity index 61% rename from jadx-core/src/main/java/jadx/core/dex/visitors/MethodInlineVisitor.java rename to jadx-core/src/main/java/jadx/core/dex/visitors/MarkMethodsForInline.java index a5f49af96..b7e726a11 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/MethodInlineVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/MarkMethodsForInline.java @@ -3,6 +3,8 @@ package jadx.core.dex.visitors; import java.util.ArrayList; import java.util.List; +import org.jetbrains.annotations.Nullable; + import jadx.api.plugins.input.data.AccessFlags; import jadx.core.Consts; import jadx.core.dex.attributes.AFlag; @@ -17,30 +19,50 @@ import jadx.core.dex.instructions.args.InsnArg; import jadx.core.dex.instructions.args.InsnWrapArg; import jadx.core.dex.instructions.args.RegisterArg; import jadx.core.dex.nodes.BlockNode; -import jadx.core.dex.nodes.FieldNode; import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.nodes.MethodNode; import jadx.core.utils.exceptions.JadxException; @JadxVisitor( - name = "InlineMethods", - desc = "Inline synthetic static methods", + name = "MarkMethodsForInline", + desc = "Mark synthetic static methods for inline", runAfter = { FixAccessModifiers.class, ClassModifier.class } ) -public class MethodInlineVisitor extends AbstractVisitor { +public class MarkMethodsForInline extends AbstractVisitor { @Override public void visit(MethodNode mth) throws JadxException { - if (canInline(mth) && mth.getBasicBlocks().size() == 2) { - BlockNode returnBlock = mth.getBasicBlocks().get(1); - if (returnBlock.contains(AFlag.RETURN) || returnBlock.getInstructions().isEmpty()) { - BlockNode firstBlock = mth.getBasicBlocks().get(0); - inlineMth(mth, firstBlock, returnBlock); + process(mth); + } + + /** + * @return null if method can't be analyzed (not loaded) + */ + @Nullable + public static MethodInlineAttr process(MethodNode mth) { + MethodInlineAttr mia = mth.get(AType.METHOD_INLINE); + if (mia != null) { + return mia; + } + if (canInline(mth)) { + List blocks = mth.getBasicBlocks(); + if (blocks == null) { + return null; + } + if (blocks.size() == 2) { + BlockNode returnBlock = blocks.get(1); + if (returnBlock.contains(AFlag.RETURN) || returnBlock.getInstructions().isEmpty()) { + MethodInlineAttr inlined = inlineMth(mth, blocks.get(0), returnBlock); + if (inlined != null) { + return inlined; + } + } } } + return MethodInlineAttr.inlineNotNeeded(mth); } public static boolean canInline(MethodNode mth) { @@ -51,41 +73,36 @@ public class MethodInlineVisitor extends AbstractVisitor { return accessFlags.isSynthetic() && accessFlags.isStatic(); } - private static void inlineMth(MethodNode mth, BlockNode firstBlock, BlockNode returnBlock) { + @Nullable + private static MethodInlineAttr inlineMth(MethodNode mth, BlockNode firstBlock, BlockNode returnBlock) { List insnList = firstBlock.getInstructions(); if (insnList.isEmpty()) { // synthetic field getter BlockNode block = mth.getBasicBlocks().get(1); InsnNode insn = block.getInstructions().get(0); // set arg from 'return' instruction - addInlineAttr(mth, InsnNode.wrapArg(insn.getArg(0))); - return; + return addInlineAttr(mth, InsnNode.wrapArg(insn.getArg(0))); } // synthetic field setter or method invoke if (insnList.size() == 1) { - addInlineAttr(mth, insnList.get(0)); - return; + return addInlineAttr(mth, insnList.get(0)); } - // TODO: inline field arithmetics. Disabled tests: TestAnonymousClass3a and TestAnonymousClass5 + return null; } - private static void addInlineAttr(MethodNode mth, InsnNode insn) { - if (fixVisibilityOfInlineCode(mth, insn)) { - if (Consts.DEBUG) { - mth.addAttr(AType.COMMENTS, "Removed for inline"); - } else { - InsnNode copy = insn.copyWithoutResult(); - // unbind SSA variables from copy instruction - List regArgs = new ArrayList<>(); - copy.getRegisterArgs(regArgs); - for (RegisterArg regArg : regArgs) { - copy.replaceArg(regArg, regArg.duplicate(regArg.getRegNum(), null)); - } - MethodInlineAttr.markForInline(mth, copy); - mth.add(AFlag.DONT_GENERATE); - } + private static MethodInlineAttr addInlineAttr(MethodNode mth, InsnNode insn) { + if (!fixVisibilityOfInlineCode(mth, insn)) { + return null; } + InsnNode copy = insn.copyWithoutResult(); + // unbind SSA variables from copy instruction + List regArgs = new ArrayList<>(); + copy.getRegisterArgs(regArgs); + for (RegisterArg regArg : regArgs) { + copy.replaceArg(regArg, regArg.duplicate(regArg.getRegNum(), null)); + } + return MethodInlineAttr.markForInline(mth, copy); } private static boolean fixVisibilityOfInlineCode(MethodNode mth, InsnNode insn) { @@ -109,10 +126,7 @@ public class MethodInlineVisitor extends AbstractVisitor { if (insn instanceof IndexInsnNode) { Object indexObj = ((IndexInsnNode) insn).getIndex(); if (indexObj instanceof FieldInfo) { - FieldNode fieldNode = mth.root().deepResolveField(((FieldInfo) indexObj)); - if (fieldNode != null) { - FixAccessModifiers.changeVisibility(fieldNode, newVisFlag); - } + // field access must be already fixed in ModVisitor.fixFieldUsage method return true; } } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/ModVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/ModVisitor.java index 9afe80673..335954159 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/ModVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/ModVisitor.java @@ -4,6 +4,7 @@ import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import org.jetbrains.annotations.Nullable; import org.slf4j.Logger; @@ -18,6 +19,7 @@ import jadx.core.dex.attributes.AType; import jadx.core.dex.attributes.AttrNode; import jadx.core.dex.attributes.annotations.AnnotationsList; import jadx.core.dex.attributes.nodes.FieldReplaceAttr; +import jadx.core.dex.info.AccessInfo; import jadx.core.dex.info.FieldInfo; import jadx.core.dex.info.MethodInfo; import jadx.core.dex.instructions.ArithNode; @@ -148,7 +150,7 @@ public class ModVisitor extends AbstractVisitor { case IPUT: case IGET: - fixTypeForFieldAccess(mth, (IndexInsnNode) insn); + fixFieldUsage(mth, (IndexInsnNode) insn); break; default: @@ -159,7 +161,10 @@ public class ModVisitor extends AbstractVisitor { } } - private static void fixTypeForFieldAccess(MethodNode mth, IndexInsnNode insn) { + /** + * If field is not visible from use site => cast to origin class + */ + private static void fixFieldUsage(MethodNode mth, IndexInsnNode insn) { InsnArg instanceArg = insn.getArg(insn.getType() == InsnType.IGET ? 0 : 1); if (instanceArg.contains(AFlag.SUPER)) { return; @@ -170,12 +175,25 @@ public class ModVisitor extends AbstractVisitor { FieldInfo fieldInfo = (FieldInfo) insn.getIndex(); ArgType clsType = fieldInfo.getDeclClass().getType(); ArgType instanceType = instanceArg.getType(); - TypeCompareEnum result = mth.root().getTypeCompare().compareTypes(instanceType, clsType); - if (result.isEqual() || (result == TypeCompareEnum.NARROW_BY_GENERIC && !instanceType.isGenericType())) { + if (Objects.equals(clsType, instanceType)) { + // cast not needed return; } + + FieldNode fieldNode = mth.root().resolveField(fieldInfo); + if (fieldNode == null) { + // unknown field + TypeCompareEnum result = mth.root().getTypeCompare().compareTypes(instanceType, clsType); + if (result.isEqual() || (result == TypeCompareEnum.NARROW_BY_GENERIC && !instanceType.isGenericType())) { + return; + } + } else if (isFieldVisibleInMethod(fieldNode, mth)) { + return; + } + // insert cast IndexInsnNode castInsn = new IndexInsnNode(InsnType.CAST, clsType, 1); castInsn.addArg(instanceArg.duplicate()); + castInsn.add(AFlag.SYNTHETIC); castInsn.add(AFlag.EXPLICIT_CAST); InsnArg castArg = InsnArg.wrapInsnIntoArg(castInsn); @@ -184,6 +202,33 @@ public class ModVisitor extends AbstractVisitor { InsnRemover.unbindArgUsage(mth, instanceArg); } + private static boolean isFieldVisibleInMethod(FieldNode field, MethodNode mth) { + AccessInfo accessFlags = field.getAccessFlags(); + if (accessFlags.isPublic()) { + return true; + } + ClassNode useCls = mth.getParentClass(); + ClassNode fieldCls = field.getParentClass(); + boolean sameScope = Objects.equals(useCls, fieldCls) && !mth.getAccessFlags().isStatic(); + if (sameScope) { + return true; + } + if (accessFlags.isPrivate()) { + return false; + } + // package-private or protected + if (Objects.equals(useCls.getClassInfo().getPackage(), fieldCls.getClassInfo().getPackage())) { + // same package + return true; + } + if (accessFlags.isPackagePrivate()) { + return false; + } + // protected + TypeCompareEnum result = mth.root().getTypeCompare().compareTypes(useCls, fieldCls); + return result == TypeCompareEnum.NARROW; // true if use class is subclass of field class + } + private static void replaceConstKeys(ClassNode parentClass, SwitchInsn insn) { int[] keys = insn.getKeys(); int len = keys.length; diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/typeinference/TypeCompare.java b/jadx-core/src/main/java/jadx/core/dex/visitors/typeinference/TypeCompare.java index e2742cc1c..49f3dce5f 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/typeinference/TypeCompare.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/typeinference/TypeCompare.java @@ -12,6 +12,7 @@ import org.slf4j.LoggerFactory; import jadx.core.dex.instructions.args.ArgType; import jadx.core.dex.instructions.args.ArgType.WildcardBound; import jadx.core.dex.instructions.args.PrimitiveType; +import jadx.core.dex.nodes.ClassNode; import jadx.core.dex.nodes.RootNode; import jadx.core.utils.exceptions.JadxRuntimeException; @@ -37,6 +38,10 @@ public class TypeCompare { this.reversedComparator = comparator.reversed(); } + public TypeCompareEnum compareTypes(ClassNode first, ClassNode second) { + return compareObjects(first.getType(), second.getType()); + } + /** * Compare two type and return result for first argument (narrow, wider or conflict) */ diff --git a/jadx-core/src/main/java/jadx/core/utils/InsnRemover.java b/jadx-core/src/main/java/jadx/core/utils/InsnRemover.java index d42bb5aa1..386d6100d 100644 --- a/jadx-core/src/main/java/jadx/core/utils/InsnRemover.java +++ b/jadx-core/src/main/java/jadx/core/utils/InsnRemover.java @@ -135,7 +135,7 @@ public class InsnRemover { mth.removeSVar(ssaVar); return; } - if (Consts.DEBUG) { // TODO: enable this + if (Consts.DEBUG_WITH_ERRORS) { throw new JadxRuntimeException("Can't remove SSA var, still in use, count: " + useCount + ", list:" + NL + " " + ssaVar.getUseList().stream() .map(arg -> arg + " from " + arg.getParentInsn()) @@ -172,7 +172,7 @@ public class InsnRemover { break; } } - if (!found && Consts.DEBUG) { // TODO: enable this + if (!found && Consts.DEBUG_WITH_ERRORS) { throw new JadxRuntimeException("Can't remove insn:" + NL + " " + rem + NL + " not found in list:" diff --git a/jadx-core/src/test/java/jadx/tests/integration/types/TestFieldCast.java b/jadx-core/src/test/java/jadx/tests/integration/types/TestFieldCast.java new file mode 100644 index 000000000..33c93fdcd --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/types/TestFieldCast.java @@ -0,0 +1,61 @@ +package jadx.tests.integration.types; + +import org.junit.jupiter.api.Test; + +import jadx.tests.api.IntegrationTest; + +import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat; + +/** + * Issue #962 + */ +public class TestFieldCast extends IntegrationTest { + + public static class TestCls { + public static class A { + public boolean publicField; + boolean packagePrivateField; + protected boolean protectedField; + private boolean privateField; + } + + public static class B extends A { + public void test() { + ((A) this).publicField = false; + ((A) this).protectedField = false; + ((A) this).packagePrivateField = false; + ((A) this).privateField = false; // cast to 'A' needed only here + } + } + + public static class C { + public void test(B b) { + ((A) b).publicField = false; + ((A) b).protectedField = false; + ((A) b).packagePrivateField = false; + ((A) b).privateField = false; // cast to 'A' needed only here + } + } + + private static class D { + public void test(T t) { + ((A) t).publicField = false; + ((A) t).protectedField = false; + ((A) t).packagePrivateField = false; + ((A) t).privateField = false; // cast to 'A' needed only here + } + } + } + + @Test + public void test() { + noDebugInfo(); + assertThat(getClassNode(TestCls.class)) + .code() + .containsOne("((A) this)") + .containsOne("((A) b)") + .containsOne("((A) t)") + .doesNotContain("unused =") + .doesNotContain("access modifiers changed"); + } +}