fix: add more checks before remove or rename enum methods (#1572)

jadx-core/src/main/java/jadx/core/codegen/InsnGen.java

@@ -823,7 +823,9 @@ private void makeInvoke(InvokeNode insn, ICodeWriter code) throws CodegenExcepti
 		}
 		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());
 		}

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,

jadx-core/src/main/java/jadx/core/dex/visitors/EnumVisitor.java

@@ -18,6 +18,7 @@
 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.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 @@
 public class EnumVisitor extends AbstractVisitor {
 
 	private MethodInfo enumValueOfMth;
+	private MethodInfo cloneMth;
 
 	@Override
 	public void init(RootNode root) {
@@ -68,6 +71,12 @@ public void init(RootNode root) {
 				"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 @@ private FieldNode searchEnumField(ClassNode cls, SSAVar ssaVar, List<InsnNode> t
 		return enumFieldNode;
 	}
 
+	@SuppressWarnings("StatementWithEmptyBody")
 	private EnumField createEnumFieldByConstructor(ClassNode cls, FieldNode enumFieldNode, ConstructorInsn co) {
 		// usually constructor signature is '<init>(Ljava/lang/String;I)V'.
 		// sometimes for one field enum second arg can be omitted
@@ -417,13 +427,12 @@ private InsnNode searchFieldPutInsn(ClassNode cls, BlockNode staticBlock, FieldN
 	}
 
 	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 @@ private void removeEnumMethods(ClassNode cls, ArgType clsType, FieldNode valuesF
 					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 @@ private boolean isDefaultConstructor(MethodNode mth, String shortId) {
 		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 @@ private boolean simpleValueOfMth(MethodNode mth, ArgType clsType) {
 		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<InsnNode> 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) {

jadx-core/src/main/java/jadx/core/utils/BlockUtils.java

@@ -1013,6 +1013,9 @@ public static List<InsnNode> collectInsnsWithLimit(List<BlockNode> blocks, int l
 	 */
 	@Nullable
 	public static InsnNode getOnlyOneInsnFromMth(MethodNode mth) {
+		if (mth.isNoCode()) {
+			return null;
+		}
 		InsnNode insn = null;
 		for (BlockNode block : mth.getBasicBlocks()) {
 			List<InsnNode> blockInsns = block.getInstructions();

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 static InsnNode searchInsn(MethodNode mth, InsnType insnType, Predicate<I
 		return null;
 	}
 
+	public static void replaceInsns(MethodNode mth, Function<InsnNode, InsnNode> replaceFunction) {
+		for (BlockNode block : mth.getBasicBlocks()) {
+			List<InsnNode> 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<InsnNode, InsnNode> 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<RegisterArg> regs, InsnType insnType) {
 		for (RegisterArg reg : regs) {

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 static TestEnumObfuscated[] vs() {
 			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(")

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