From 51555667cfd4a8d473d1275dc8de12893293658a Mon Sep 17 00:00:00 2001 From: Skylot Date: Thu, 7 Jul 2022 16:42:42 +0100 Subject: [PATCH] fix: add more checks before remove or rename enum methods (#1572) --- .../main/java/jadx/core/codegen/InsnGen.java | 4 +- .../java/jadx/core/dex/attributes/AFlag.java | 2 + .../jadx/core/dex/visitors/EnumVisitor.java | 99 +++++++++++++++++-- .../main/java/jadx/core/utils/BlockUtils.java | 3 + .../main/java/jadx/core/utils/InsnUtils.java | 32 ++++++ .../integration/enums/TestEnumObfuscated.java | 25 ++++- .../test/smali/enums/TestEnumObfuscated.smali | 21 ++++ 7 files changed, 173 insertions(+), 13 deletions(-) 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 dc71665ef..fa3a703f2 100644 --- a/jadx-core/src/main/java/jadx/core/codegen/InsnGen.java +++ b/jadx-core/src/main/java/jadx/core/codegen/InsnGen.java @@ -823,7 +823,9 @@ public class InsnGen { } if (callMthNode != null) { code.attachAnnotation(callMthNode); - code.add(callMthNode.getAlias()); + } + if (insn.contains(AFlag.FORCE_RAW_NAME)) { + code.add(callMth.getName()); } else { code.add(callMth.getAlias()); } 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 3a77ad214..c43f89453 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 @@ -25,6 +25,8 @@ public enum AFlag { HIDDEN, // instruction used inside other instruction but not listed in args DONT_RENAME, // do not rename during deobfuscation + FORCE_RAW_NAME, // force use of raw name instead alias + ADDED_TO_REGION, EXC_TOP_SPLITTER, diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/EnumVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/EnumVisitor.java index 1acf67445..5285b1d04 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/EnumVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/EnumVisitor.java @@ -18,6 +18,7 @@ import jadx.core.dex.attributes.AFlag; import jadx.core.dex.attributes.AType; import jadx.core.dex.attributes.nodes.EnumClassAttr; import jadx.core.dex.attributes.nodes.EnumClassAttr.EnumField; +import jadx.core.dex.attributes.nodes.RenameReasonAttr; import jadx.core.dex.attributes.nodes.SkipMethodArgsAttr; import jadx.core.dex.info.AccessInfo; import jadx.core.dex.info.ClassInfo; @@ -26,6 +27,7 @@ import jadx.core.dex.info.MethodInfo; import jadx.core.dex.instructions.IndexInsnNode; import jadx.core.dex.instructions.InsnType; import jadx.core.dex.instructions.InvokeNode; +import jadx.core.dex.instructions.InvokeType; import jadx.core.dex.instructions.args.ArgType; import jadx.core.dex.instructions.args.InsnArg; import jadx.core.dex.instructions.args.InsnWrapArg; @@ -59,6 +61,7 @@ import static jadx.core.utils.InsnUtils.getWrappedInsn; public class EnumVisitor extends AbstractVisitor { private MethodInfo enumValueOfMth; + private MethodInfo cloneMth; @Override public void init(RootNode root) { @@ -68,6 +71,12 @@ public class EnumVisitor extends AbstractVisitor { "valueOf", Arrays.asList(ArgType.CLASS, ArgType.STRING), ArgType.ENUM); + + cloneMth = MethodInfo.fromDetails(root, + ClassInfo.fromType(root, ArgType.OBJECT), + "clone", + Collections.emptyList(), + ArgType.OBJECT); } @Override @@ -377,6 +386,7 @@ public class EnumVisitor extends AbstractVisitor { return enumFieldNode; } + @SuppressWarnings("StatementWithEmptyBody") private EnumField createEnumFieldByConstructor(ClassNode cls, FieldNode enumFieldNode, ConstructorInsn co) { // usually constructor signature is '(Ljava/lang/String;I)V'. // sometimes for one field enum second arg can be omitted @@ -417,13 +427,12 @@ public class EnumVisitor extends AbstractVisitor { } private void removeEnumMethods(ClassNode cls, ArgType clsType, FieldNode valuesField) { - String valuesMethod = "values()" + TypeGen.signature(ArgType.array(clsType)); - FieldInfo valuesFieldInfo = valuesField.getFieldInfo(); - + String valuesMethodShortId = "values()" + TypeGen.signature(ArgType.array(clsType)); + MethodNode valuesMethod = null; // remove compiler generated methods for (MethodNode mth : cls.getMethods()) { MethodInfo mi = mth.getMethodInfo(); - if (mi.isClassInit()) { + if (mi.isClassInit() || mth.isNoCode()) { continue; } String shortId = mi.getShortId(); @@ -432,12 +441,33 @@ public class EnumVisitor extends AbstractVisitor { mth.add(AFlag.DONT_GENERATE); } markArgsForSkip(mth); - } else if (shortId.equals(valuesMethod) - || usesValuesField(mth, valuesFieldInfo) - || simpleValueOfMth(mth, clsType)) { + } else if (mi.getShortId().equals(valuesMethodShortId)) { + if (isValuesMethod(mth, clsType)) { + valuesMethod = mth; + mth.add(AFlag.DONT_GENERATE); + } else { + // custom values method => rename to resolve conflict with enum method + mth.getMethodInfo().setAlias("valuesCustom"); + mth.addAttr(new RenameReasonAttr(mth).append("to resolve conflict with enum method")); + } + } else if (isValuesMethod(mth, clsType)) { + if (!mth.getMethodInfo().getAlias().equals("values") && !mth.getUseIn().isEmpty()) { + // rename to use default values method + mth.getMethodInfo().setAlias("values"); + mth.addAttr(new RenameReasonAttr(mth).append("to match enum method name")); + mth.add(AFlag.DONT_RENAME); + } + valuesMethod = mth; + mth.add(AFlag.DONT_GENERATE); + } else if (simpleValueOfMth(mth, clsType)) { mth.add(AFlag.DONT_GENERATE); } } + FieldInfo valuesFieldInfo = valuesField.getFieldInfo(); + for (MethodNode mth : cls.getMethods()) { + // fix access to 'values' field and 'values()' method + fixValuesAccess(mth, valuesFieldInfo, clsType, valuesMethod); + } } private void markArgsForSkip(MethodNode mth) { @@ -458,6 +488,25 @@ public class EnumVisitor extends AbstractVisitor { return false; } + // TODO: support other method patterns ??? + private boolean isValuesMethod(MethodNode mth, ArgType clsType) { + ArgType retType = mth.getReturnType(); + if (!retType.isArray() || !retType.getArrayElement().equals(clsType)) { + return false; + } + InsnNode returnInsn = BlockUtils.getOnlyOneInsnFromMth(mth); + if (returnInsn == null || returnInsn.getType() != InsnType.RETURN || returnInsn.getArgsCount() != 1) { + return false; + } + InsnNode wrappedInsn = getWrappedInsn(getSingleArg(returnInsn)); + IndexInsnNode castInsn = (IndexInsnNode) checkInsnType(wrappedInsn, InsnType.CHECK_CAST); + if (castInsn != null && Objects.equals(castInsn.getIndex(), ArgType.array(clsType))) { + InvokeNode invokeInsn = (InvokeNode) checkInsnType(getWrappedInsn(getSingleArg(castInsn)), InsnType.INVOKE); + return invokeInsn != null && invokeInsn.getCallMth().equals(cloneMth); + } + return false; + } + private boolean simpleValueOfMth(MethodNode mth, ArgType clsType) { InsnNode returnInsn = InsnUtils.searchSingleReturnInsn(mth, insn -> insn.getArgsCount() == 1); if (returnInsn == null) { @@ -472,9 +521,41 @@ public class EnumVisitor extends AbstractVisitor { return false; } - private boolean usesValuesField(MethodNode mth, FieldInfo valuesFieldInfo) { + private void fixValuesAccess(MethodNode mth, FieldInfo valuesFieldInfo, ArgType clsType, @Nullable MethodNode valuesMethod) { + MethodInfo mi = mth.getMethodInfo(); + if (mi.isConstructor() || mi.isClassInit() || mth.isNoCode() || mth == valuesMethod) { + return; + } + // search value field usage Predicate insnTest = insn -> Objects.equals(((IndexInsnNode) insn).getIndex(), valuesFieldInfo); - return InsnUtils.searchInsn(mth, InsnType.SGET, insnTest) != null; + InsnNode useInsn = InsnUtils.searchInsn(mth, InsnType.SGET, insnTest); + if (useInsn == null) { + return; + } + // replace 'values' field with 'values()' method + InsnUtils.replaceInsns(mth, insn -> { + if (insn.getType() == InsnType.SGET && insnTest.test(insn)) { + MethodInfo valueMth = valuesMethod == null + ? getValueMthInfo(mth.root(), clsType) + : valuesMethod.getMethodInfo(); + InvokeNode invokeNode = new InvokeNode(valueMth, InvokeType.STATIC, 0); + invokeNode.setResult(insn.getResult()); + if (valuesMethod == null) { + // forcing enum method (can overlap and get renamed by custom method) + invokeNode.add(AFlag.FORCE_RAW_NAME); + } + mth.addDebugComment("Replace access to removed values field (" + valuesFieldInfo.getName() + ") with 'values()' method"); + return invokeNode; + } + return null; + }); + } + + private MethodInfo getValueMthInfo(RootNode root, ArgType clsType) { + return MethodInfo.fromDetails(root, + ClassInfo.fromType(root, clsType), + "values", + Collections.emptyList(), ArgType.array(clsType)); } private static void processEnumCls(ClassNode cls, EnumField field, ClassNode innerCls) { 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 daee89e1b..db9434639 100644 --- a/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java @@ -1013,6 +1013,9 @@ public class BlockUtils { */ @Nullable public static InsnNode getOnlyOneInsnFromMth(MethodNode mth) { + if (mth.isNoCode()) { + return null; + } InsnNode insn = null; for (BlockNode block : mth.getBasicBlocks()) { List blockInsns = block.getInstructions(); diff --git a/jadx-core/src/main/java/jadx/core/utils/InsnUtils.java b/jadx-core/src/main/java/jadx/core/utils/InsnUtils.java index 3399210f6..d935a743e 100644 --- a/jadx-core/src/main/java/jadx/core/utils/InsnUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/InsnUtils.java @@ -1,6 +1,7 @@ package jadx.core.utils; import java.util.List; +import java.util.function.Function; import java.util.function.Predicate; import org.jetbrains.annotations.Nullable; @@ -140,6 +141,37 @@ public class InsnUtils { return null; } + public static void replaceInsns(MethodNode mth, Function replaceFunction) { + for (BlockNode block : mth.getBasicBlocks()) { + List insns = block.getInstructions(); + int insnsCount = insns.size(); + for (int i = 0; i < insnsCount; i++) { + InsnNode insn = insns.get(i); + replaceInsnsInInsn(mth, insn, replaceFunction); + InsnNode replace = replaceFunction.apply(insn); + if (replace != null) { + BlockUtils.replaceInsn(mth, block, i, replace); + } + } + } + } + + public static void replaceInsnsInInsn(MethodNode mth, InsnNode insn, Function replaceFunction) { + int argsCount = insn.getArgsCount(); + for (int i = 0; i < argsCount; i++) { + InsnArg arg = insn.getArg(i); + if (arg.isInsnWrap()) { + InsnNode wrapInsn = ((InsnWrapArg) arg).getWrapInsn(); + replaceInsnsInInsn(mth, wrapInsn, replaceFunction); + InsnNode replace = replaceFunction.apply(wrapInsn); + if (replace != null) { + InsnRemover.unbindArgUsage(mth, arg); + insn.setArg(i, InsnArg.wrapInsnIntoArg(replace)); + } + } + } + } + @Nullable public static RegisterArg getRegFromInsn(List regs, InsnType insnType) { for (RegisterArg reg : regs) { diff --git a/jadx-core/src/test/java/jadx/tests/integration/enums/TestEnumObfuscated.java b/jadx-core/src/test/java/jadx/tests/integration/enums/TestEnumObfuscated.java index f4e789e42..a0052e47b 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/enums/TestEnumObfuscated.java +++ b/jadx-core/src/test/java/jadx/tests/integration/enums/TestEnumObfuscated.java @@ -1,8 +1,10 @@ package jadx.tests.integration.enums; +import java.util.Collections; + import org.junit.jupiter.api.Test; -import jadx.core.dex.nodes.ClassNode; +import jadx.api.CommentsLevel; import jadx.tests.api.SmaliTest; import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat; @@ -32,14 +34,31 @@ public class TestEnumObfuscated extends SmaliTest { public synthetic int getNum() { return this.num; } + + // custom values method + // should be kept and renamed to avoid collision to enum 'values()' method + public static int values() { + return new TestEnumObfuscated[0]; + } + + // usage of renamed 'values()' method, should be renamed back to 'values' + public static int valuesCount() { + return vs().length; + } + + // usage of renamed '$VALUES' field, should be replaced with 'values()' method call + public static int valuesFieldUse() { + return $VLS.length; + } } */ // @formatter:on @Test public void test() { - ClassNode cls = getClassNodeFromSmali(); - assertThat(cls) + getArgs().setCommentsLevel(CommentsLevel.WARN); + getArgs().setRenameFlags(Collections.emptySet()); + assertThat(getClassNodeFromSmali()) .code() .doesNotContain("$VLS") .doesNotContain("vo(") diff --git a/jadx-core/src/test/smali/enums/TestEnumObfuscated.smali b/jadx-core/src/test/smali/enums/TestEnumObfuscated.smali index 99e91e69c..1125dd35d 100644 --- a/jadx-core/src/test/smali/enums/TestEnumObfuscated.smali +++ b/jadx-core/src/test/smali/enums/TestEnumObfuscated.smali @@ -72,6 +72,27 @@ return-object v0 .end method +.method public static values()[Lenums/TestEnumObfuscated; + .registers 1 + sget-object v0, Lenums/TestEnumObfuscated;->$VLS:[Lenums/TestEnumObfuscated; + return v0 +.end method + +.method public static valuesCount()I + .registers 2 + invoke-static {v0, p0}, Lenums/TestEnumObfuscated;->vs()[Lenums/TestEnumObfuscated; + move-result-object v0 + array-length v1, v0 + return v1 +.end method + +.method public static valuesFieldUse()I + .registers 2 + sget-object v0, Lenums/TestEnumObfuscated;->$VLS:[Lenums/TestEnumObfuscated; + array-length v1, v0 + return v1 +.end method + .method public synthetic getNum()I .registers 2