fix: additional checks for synthetic methods remove, rename and inline (#452)

jadx-core/src/main/java/jadx/core/Jadx.java

@@ -95,10 +95,10 @@ public class Jadx {
 			passes.add(new SimplifyVisitor());
 			passes.add(new CheckRegions());
 
-			passes.add(new MethodInlineVisitor());
 			passes.add(new ExtractFieldInit());
 			passes.add(new FixAccessModifiers());
 			passes.add(new ClassModifier());
+			passes.add(new MethodInlineVisitor());
 			passes.add(new EnumVisitor());
 			passes.add(new PrepareForCodeGen());
 			passes.add(new LoopRegionVisitor());

jadx-core/src/main/java/jadx/core/clsp/ClsSet.java

@@ -97,11 +97,11 @@ public class ClsSet {
 	}
 
 	private static NClass getCls(String fullName, Map<String, NClass> names) {
-		NClass id = names.get(fullName);
-		if (id == null && !names.containsKey(fullName)) {
+		NClass cls = names.get(fullName);
+		if (cls == null) {
 			LOG.debug("Class not found: {}", fullName);
 		}
-		return id;
+		return cls;
 	}
 
 	void save(File output) throws IOException {

jadx-core/src/main/java/jadx/core/clsp/NClass.java

@@ -7,7 +7,7 @@ public class NClass {
 
 	private final String name;
 	private NClass[] parents;
-	private int id;
+	private final int id;
 
 	public NClass(String name, int id) {
 		this.name = name;
@@ -22,10 +22,6 @@ public class NClass {
 		return id;
 	}
 
-	public void setId(int id) {
-		this.id = id;
-	}
-
 	public NClass[] getParents() {
 		return parents;
 	}

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

@@ -63,12 +63,7 @@ public class AnnotationGen {
 		}
 		for (Annotation a : aList.getAll()) {
 			String aCls = a.getAnnotationClass();
-			if (aCls.startsWith(Consts.DALVIK_ANNOTATION_PKG)) {
-				// skip
-				if (Consts.DEBUG) {
-					code.startLine("// " + a);
-				}
-			} else {
+			if (!aCls.startsWith(Consts.DALVIK_ANNOTATION_PKG)) {
 				code.startLine();
 				formatAnnotation(code, a);
 			}

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

@@ -11,6 +11,7 @@ import org.jetbrains.annotations.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import jadx.core.Consts;
 import jadx.core.dex.attributes.AFlag;
 import jadx.core.dex.attributes.AType;
 import jadx.core.dex.attributes.nodes.FieldReplaceAttr;
@@ -637,6 +638,11 @@ public class InsnGen {
 				break;
 
 			case SUPER:
+				ClassInfo superCallCls = getClassForSuperCall(code, callMth);
+				if (superCallCls != null) {
+					useClass(code, superCallCls);
+					code.add('.');
+				}
 				// use 'super' instead 'this' in 0 arg
 				code.add("super").add('.');
 				k++;
@@ -658,6 +664,36 @@ public class InsnGen {
 		generateMethodArguments(code, insn, k, callMthNode);
 	}
 
+	@Nullable
+	private ClassInfo getClassForSuperCall(CodeWriter code, MethodInfo callMth) {
+		ClassNode useCls = mth.getParentClass();
+		ClassInfo insnCls = useCls.getAlias();
+		ClassInfo declClass = callMth.getDeclClass();
+		if (insnCls.equals(declClass)) {
+			return null;
+		}
+		ClassNode topClass = useCls.getTopParentClass();
+		if (topClass.getClassInfo().equals(declClass)) {
+			return declClass;
+		}
+		// search call class
+		ClassNode nextParent = useCls;
+		do {
+			ClassInfo nextClsInfo = nextParent.getClassInfo();
+			if (nextClsInfo.equals(declClass)
+					|| ArgType.isInstanceOf(mth.dex(), nextClsInfo.getType(), declClass.getType())) {
+				if (nextParent == useCls) {
+					return null;
+				}
+				return nextClsInfo;
+			}
+			nextParent = nextParent.getParentClass();
+		} while (nextParent != null && nextParent != topClass);
+
+		// search failed, just return parent class
+		return useCls.getParentClass().getClassInfo();
+	}
+
 	void generateMethodArguments(CodeWriter code, InsnNode insn, int startArgNum,
 	                             @Nullable MethodNode callMth) throws CodegenException {
 		int k = startArgNum;
@@ -748,6 +784,9 @@ public class InsnGen {
 			return false;
 		}
 		InsnNode inl = mia.getInsn();
+		if (Consts.DEBUG) {
+			code.add("/* inline method: ").add(callMthNode.toString()).add("*/").startLine();
+		}
 		if (callMthNode.getMethodInfo().getArgumentsTypes().isEmpty()) {
 			makeInsn(inl, code, Flags.BODY_ONLY);
 		} else {

jadx-core/src/main/java/jadx/core/dex/info/MethodInfo.java

@@ -43,8 +43,12 @@ public final class MethodInfo {
 	}
 
 	public String makeSignature(boolean includeRetType) {
+		return makeSignature(false, includeRetType);
+	}
+
+	public String makeSignature(boolean useAlias, boolean includeRetType) {
 		StringBuilder signature = new StringBuilder();
-		signature.append(name);
+		signature.append(useAlias ? alias : name);
 		signature.append('(');
 		for (ArgType arg : args) {
 			signature.append(TypeGen.signature(arg));

jadx-core/src/main/java/jadx/core/dex/nodes/ClassNode.java

@@ -38,12 +38,12 @@ import jadx.core.utils.exceptions.JadxRuntimeException;
 
 import static jadx.core.dex.nodes.ProcessState.UNLOADED;
 
-public class ClassNode extends LineAttrNode implements ILoadable, IDexNode {
+public class ClassNode extends LineAttrNode implements ILoadable, ICodeNode {
 	private static final Logger LOG = LoggerFactory.getLogger(ClassNode.class);
 
 	private final DexNode dex;
 	private final ClassInfo clsInfo;
-	private final AccessInfo accessFlags;
+	private AccessInfo accessFlags;
 	private ArgType superClass;
 	private List<ArgType> interfaces;
 	private Map<ArgType, List<ArgType>> genericMap;
@@ -409,11 +409,17 @@ public class ClassNode extends LineAttrNode implements ILoadable, IDexNode {
 		return null;
 	}
 
+	@Override
 	public AccessInfo getAccessFlags() {
 		return accessFlags;
 	}
 
 	@Override
+	public void setAccessFlags(AccessInfo accessFlags) {
+		this.accessFlags = accessFlags;
+	}
+
+	@Override
 	public DexNode dex() {
 		return dex;
 	}

jadx-core/src/main/java/jadx/core/dex/nodes/FieldNode.java

@@ -8,11 +8,11 @@ import jadx.core.dex.info.AccessInfo.AFType;
 import jadx.core.dex.info.FieldInfo;
 import jadx.core.dex.instructions.args.ArgType;
 
-public class FieldNode extends LineAttrNode {
+public class FieldNode extends LineAttrNode implements ICodeNode {
 
 	private final ClassNode parent;
 	private final FieldInfo fieldInfo;
-	private final AccessInfo accFlags;
+	private AccessInfo accFlags;
 
 	private ArgType type; // store signature
 
@@ -32,10 +32,16 @@ public class FieldNode extends LineAttrNode {
 		return fieldInfo;
 	}
 
+	@Override
 	public AccessInfo getAccessFlags() {
 		return accFlags;
 	}
 
+	@Override
+	public void setAccessFlags(AccessInfo accFlags) {
+		this.accFlags = accFlags;
+	}
+
 	public String getName() {
 		return fieldInfo.getName();
 	}
@@ -57,6 +63,21 @@ public class FieldNode extends LineAttrNode {
 	}
 
 	@Override
+	public String typeName() {
+		return "field";
+	}
+
+	@Override
+	public DexNode dex() {
+		return parent.dex();
+	}
+
+	@Override
+	public RootNode root() {
+		return parent.root();
+	}
+
+	@Override
 	public int hashCode() {
 		return fieldInfo.hashCode();
 	}

jadx-core/src/main/java/jadx/core/dex/nodes/ICodeNode.java

+package jadx.core.dex.nodes;
+
+import jadx.core.dex.attributes.IAttributeNode;
+import jadx.core.dex.info.AccessInfo;
+
+public interface ICodeNode extends IDexNode, IAttributeNode {
+	AccessInfo getAccessFlags();
+
+	void setAccessFlags(AccessInfo newAccessFlags);
+}

jadx-core/src/main/java/jadx/core/dex/nodes/MethodNode.java

@@ -46,7 +46,7 @@ import jadx.core.utils.exceptions.JadxRuntimeException;
 
 import static jadx.core.utils.Utils.lockList;
 
-public class MethodNode extends LineAttrNode implements ILoadable, IDexNode {
+public class MethodNode extends LineAttrNode implements ILoadable, ICodeNode {
 	private static final Logger LOG = LoggerFactory.getLogger(MethodNode.class);
 
 	private final MethodInfo mthInfo;
@@ -595,12 +595,14 @@ public class MethodNode extends LineAttrNode implements ILoadable, IDexNode {
 		return sVars;
 	}
 
+	@Override
 	public AccessInfo getAccessFlags() {
 		return accFlags;
 	}
 
-	public void setAccFlags(AccessInfo accFlags) {
-		this.accFlags = accFlags;
+	@Override
+	public void setAccessFlags(AccessInfo newAccessFlags) {
+		this.accFlags = newAccessFlags;
 	}
 
 	public Region getRegion() {

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

@@ -5,6 +5,7 @@ import java.util.Objects;
 
 import com.android.dx.rop.code.AccessFlags;
 
+import jadx.core.Consts;
 import jadx.core.dex.attributes.AFlag;
 import jadx.core.dex.attributes.AType;
 import jadx.core.dex.attributes.nodes.FieldReplaceAttr;
@@ -15,6 +16,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;
@@ -140,7 +142,11 @@ public class ClassModifier extends AbstractVisitor {
 			return;
 		}
 		if (removeBridgeMethod(cls, mth)) {
-			mth.add(AFlag.DONT_GENERATE);
+			if (Consts.DEBUG) {
+				mth.addAttr(AType.COMMENTS, "Removed as synthetic bridge method");
+			} else {
+				mth.add(AFlag.DONT_GENERATE);
+			}
 			return;
 		}
 		// remove synthetic constructor for inner classes
@@ -219,40 +225,45 @@ public class ClassModifier extends AbstractVisitor {
 
 	private static boolean checkSyntheticWrapper(MethodNode mth, InsnNode insn) {
 		InsnType insnType = insn.getType();
-		if (insnType == InsnType.INVOKE) {
-			MethodInfo callMth = ((InvokeNode) insn).getCallMth();
-			MethodNode wrappedMth = mth.root().deepResolveMethod(callMth);
-			if (wrappedMth != null) {
-				AccessInfo wrappedAccFlags = wrappedMth.getAccessFlags();
-				if (wrappedAccFlags.isStatic()) {
-					return false;
-				}
-				if (callMth.getArgsCount() != mth.getMethodInfo().getArgsCount()) {
-					return false;
-				}
-				// rename method only from current class
-				if (!mth.getParentClass().equals(wrappedMth.getParentClass())) {
-					return false;
-				}
-				// all args must be registers passed from method args (allow only casts insns)
-				for (InsnArg arg : insn.getArguments()) {
-					if (!registersAndCastsOnly(arg)) {
-						return false;
-					}
-				}
-				String alias = mth.getAlias();
-				if (Objects.equals(wrappedMth.getAlias(), alias)) {
-					return true;
-				}
-				if (!wrappedAccFlags.isPublic()) {
-					// must be public
-					FixAccessModifiers.changeVisibility(wrappedMth, AccessFlags.ACC_PUBLIC);
-				}
-				wrappedMth.getMethodInfo().setAlias(alias);
-				return true;
+		if (insnType != InsnType.INVOKE) {
+			return false;
+		}
+		InvokeNode invokeInsn = (InvokeNode) insn;
+		if (invokeInsn.getInvokeType() == InvokeType.SUPER) {
+			return false;
+		}
+		MethodInfo callMth = invokeInsn.getCallMth();
+		MethodNode wrappedMth = mth.root().deepResolveMethod(callMth);
+		if (wrappedMth == null) {
+			return false;
+		}
+		AccessInfo wrappedAccFlags = wrappedMth.getAccessFlags();
+		if (wrappedAccFlags.isStatic()) {
+			return false;
+		}
+		if (callMth.getArgsCount() != mth.getMethodInfo().getArgsCount()) {
+			return false;
+		}
+		// rename method only from current class
+		if (!mth.getParentClass().equals(wrappedMth.getParentClass())) {
+			return false;
+		}
+		// all args must be registers passed from method args (allow only casts insns)
+		for (InsnArg arg : insn.getArguments()) {
+			if (!registersAndCastsOnly(arg)) {
+				return false;
 			}
 		}
-		return false;
+		// remove confirmed, change visibility and name if needed
+		if (!wrappedAccFlags.isPublic()) {
+			// must be public
+			FixAccessModifiers.changeVisibility(wrappedMth, AccessFlags.ACC_PUBLIC);
+		}
+		String alias = mth.getAlias();
+		if (!Objects.equals(wrappedMth.getAlias(), alias)) {
+			wrappedMth.getMethodInfo().setAlias(alias);
+		}
+		return true;
 	}
 
 	private static boolean registersAndCastsOnly(InsnArg arg) {
@@ -274,8 +285,7 @@ public class ClassModifier extends AbstractVisitor {
 			if (otherMth != mth) {
 				MethodInfo omi = otherMth.getMethodInfo();
 				if (omi.getName().equals(mi.getName())
-						&& omi.getArgumentsTypes().size() == mi.getArgumentsTypes().size()) {
-					// TODO: check objects types
+						&& Objects.equals(omi.getArgumentsTypes(), mi.getArgumentsTypes())) {
 					return false;
 				}
 			}

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

@@ -4,6 +4,7 @@ import com.android.dx.rop.code.AccessFlags;
 
 import jadx.core.dex.attributes.AType;
 import jadx.core.dex.info.AccessInfo;
+import jadx.core.dex.nodes.ICodeNode;
 import jadx.core.dex.nodes.MethodNode;
 import jadx.core.dex.nodes.RootNode;
 
@@ -32,12 +33,12 @@ public class FixAccessModifiers extends AbstractVisitor {
 		}
 	}
 
-	public static void changeVisibility(MethodNode mth, int newVisFlag) {
-		AccessInfo accessFlags = mth.getAccessFlags();
+	public static void changeVisibility(ICodeNode node, int newVisFlag) {
+		AccessInfo accessFlags = node.getAccessFlags();
 		AccessInfo newAccFlags = accessFlags.changeVisibility(newVisFlag);
 		if (newAccFlags != accessFlags) {
-			mth.setAccFlags(newAccFlags);
-			mth.addAttr(AType.COMMENTS, "access modifiers changed from: " + accessFlags.rawString());
+			node.setAccessFlags(newAccFlags);
+			node.addAttr(AType.COMMENTS, "access modifiers changed from: " + accessFlags.rawString());
 		}
 	}
 

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

@@ -2,28 +2,44 @@ package jadx.core.dex.visitors;
 
 import java.util.List;
 
+import com.android.dx.rop.code.AccessFlags;
+
+import jadx.core.Consts;
 import jadx.core.dex.attributes.AFlag;
+import jadx.core.dex.attributes.AType;
 import jadx.core.dex.attributes.nodes.MethodInlineAttr;
 import jadx.core.dex.info.AccessInfo;
+import jadx.core.dex.info.FieldInfo;
+import jadx.core.dex.instructions.IndexInsnNode;
 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.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;
 
-/**
- * Inline synthetic methods.
- */
+@JadxVisitor(
+		name = "InlineMethods",
+		desc = "Inline synthetic static methods",
+		runAfter = {
+				FixAccessModifiers.class,
+				ClassModifier.class
+		}
+)
 public class MethodInlineVisitor extends AbstractVisitor {
 
 	@Override
 	public void visit(MethodNode mth) throws JadxException {
+		if (mth.isNoCode() || mth.contains(AFlag.DONT_GENERATE)) {
+			return;
+		}
 		AccessInfo accessFlags = mth.getAccessFlags();
-		if (accessFlags.isSynthetic()
-				&& accessFlags.isStatic()
+		if (accessFlags.isSynthetic() && accessFlags.isStatic()
 				&& mth.getBasicBlocks().size() == 2) {
 			BlockNode returnBlock = mth.getBasicBlocks().get(1);
 			if (returnBlock.contains(AFlag.RETURN) || returnBlock.getInstructions().isEmpty()) {
@@ -72,7 +88,47 @@ public class MethodInlineVisitor extends AbstractVisitor {
 	}
 
 	private static void addInlineAttr(MethodNode mth, InsnNode insn) {
-		mth.addAttr(new MethodInlineAttr(insn));
-		mth.add(AFlag.DONT_GENERATE);
+		if (fixVisibilityOfInlineCode(mth, insn)) {
+			if (Consts.DEBUG) {
+				mth.addAttr(AType.COMMENTS, "Removed for inline");
+			} else {
+				mth.addAttr(new MethodInlineAttr(insn));
+				mth.add(AFlag.DONT_GENERATE);
+			}
+		}
+	}
+
+	private static boolean fixVisibilityOfInlineCode(MethodNode mth, InsnNode insn) {
+		int newVisFlag = AccessFlags.ACC_PUBLIC; // TODO: calculate more precisely
+		InsnType insnType = insn.getType();
+		if (insnType == InsnType.INVOKE) {
+			InvokeNode invoke = (InvokeNode) insn;
+			MethodNode callMthNode = mth.root().deepResolveMethod(invoke.getCallMth());
+			if (callMthNode != null) {
+				FixAccessModifiers.changeVisibility(callMthNode, newVisFlag);
+			}
+			return true;
+		}
+		if (insnType == InsnType.ONE_ARG) {
+			InsnArg arg = insn.getArg(0);
+			if (!arg.isInsnWrap()) {
+				return false;
+			}
+			return fixVisibilityOfInlineCode(mth, ((InsnWrapArg) arg).getWrapInsn());
+		}
+		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);
+				}
+				return true;
+			}
+		}
+		if (Consts.DEBUG) {
+			mth.addAttr(AType.COMMENTS, "JADX DEBUG: can't inline method, not implemented redirect type: " + insn);
+		}
+		return false;
 	}
 }

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

@@ -107,17 +107,22 @@ public class RenameVisitor extends AbstractVisitor {
 	}
 
 	private void checkMethods(ClassNode cls) {
+		for (MethodNode mth : cls.getMethods()) {
+			if (!NameMapper.isValidIdentifier(mth.getAlias())) {
+				deobfuscator.forceRenameMethod(mth);
+			}
+		}
 		Set<String> names = new HashSet<>();
 		for (MethodNode mth : cls.getMethods()) {
 			AccessInfo accessFlags = mth.getAccessFlags();
 			if (accessFlags.isConstructor()
 					|| accessFlags.isBridge()
 					|| accessFlags.isSynthetic()
-					|| mth.contains(AFlag.DONT_GENERATE)) {
+					|| mth.contains(AFlag.DONT_GENERATE) /* this flag not set yet */) {
 				continue;
 			}
-			String signature = mth.getMethodInfo().makeSignature(false);
-			if (!names.add(signature) || !NameMapper.isValidIdentifier(mth.getAlias())) {
+			String signature = mth.getMethodInfo().makeSignature(true, false);
+			if (!names.add(signature)) {
 				deobfuscator.forceRenameMethod(mth);
 			}
 		}

jadx-core/src/test/java/jadx/tests/api/IntegrationTest.java

@@ -214,12 +214,14 @@ public abstract class IntegrationTest extends TestUtils {
 				rethrow("Original check failed", ie);
 			}
 			// run 'check' method from decompiled class
-			try {
-				invoke("check");
-			} catch (InvocationTargetException ie) {
-				rethrow("Decompiled check failed", ie);
+			if (compile) {
+				try {
+					invoke("check");
+				} catch (InvocationTargetException ie) {
+					rethrow("Decompiled check failed", ie);
+				}
+				System.out.println("Auto check: PASSED");
 			}
-			System.out.println("Auto check: PASSED");
 		} catch (Exception e) {
 			e.printStackTrace();
 			fail("Auto check exception: " + e.getMessage());

jadx-core/src/test/java/jadx/tests/integration/inline/TestSyntheticInline2.java

+package jadx.tests.integration.inline;
+
+import org.junit.Test;
+
+import jadx.core.dex.nodes.ClassNode;
+import jadx.tests.api.IntegrationTest;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertThat;
+
+public class TestSyntheticInline2 extends IntegrationTest {
+
+	public static class Base {
+		protected void call() {
+			System.out.println("base call");
+		}
+	}
+
+	public static class TestCls extends Base {
+		public class A {
+			public void invokeCall() {
+				TestCls.this.call();
+			}
+
+			public void invokeSuperCall() {
+				TestCls.super.call();
+			}
+		}
+
+		@Override
+		public void call() {
+			System.out.println("TestCls call");
+		}
+
+		public void check() {
+			A a = new A();
+			a.invokeSuperCall();
+			a.invokeCall();
+		}
+	}
+
+	@Test
+	public void test() {
+		disableCompilation(); // strange java compiler bug
+		ClassNode cls = getClassNode(TestCls.class); // Base class in unknown
+		String code = cls.getCode().toString();
+
+		assertThat(code, not(containsString("synthetic")));
+		assertThat(code, not(containsString("access$")));
+		assertThat(code, containsString("TestSyntheticInline2$TestCls.this.call();"));
+		assertThat(code, containsString("TestSyntheticInline2$TestCls.super.call();"));
+	}
+
+	@Test
+	public void testTopClass() {
+		ClassNode cls = getClassNode(TestSyntheticInline2.class);
+		String code = cls.getCode().toString();
+
+		assertThat(code, containsString(indent(1) + "TestCls.super.call();"));
+	}
+}

jadx-core/src/test/java/jadx/tests/integration/names/TestDuplicatedNames.java

@@ -46,6 +46,6 @@ public class TestDuplicatedNames extends SmaliTest {
 		assertThat(code, containsOne("this.f0fieldName"));
 
 		assertThat(code, containsOne("public Object run() {"));
-		assertThat(code, containsOne("public String m0run() {"));
+		assertThat(code, containsOne("public String m1run() {"));
 	}
 }