fix: don't trust type info in signature, check before apply (#858)

jadx-core/src/main/java/jadx/api/JadxDecompiler.java

@@ -100,8 +100,8 @@ public final class JadxDecompiler implements Closeable {
 		root.loadClasses(loadedInputs);
 		root.initClassPath();
 		root.loadResources(getResources());
-		root.initPasses();
 		root.runPreDecompileStage();
+		root.initPasses();
 	}
 
 	private void loadInputFiles() {

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

@@ -37,6 +37,7 @@ import jadx.core.dex.visitors.ProcessInstructionsVisitor;
 import jadx.core.dex.visitors.ReSugarCode;
 import jadx.core.dex.visitors.RenameVisitor;
 import jadx.core.dex.visitors.ShadowFieldVisitor;
+import jadx.core.dex.visitors.SignatureProcessor;
 import jadx.core.dex.visitors.SimplifyVisitor;
 import jadx.core.dex.visitors.blocksmaker.BlockExceptionHandler;
 import jadx.core.dex.visitors.blocksmaker.BlockFinish;
@@ -78,6 +79,7 @@ public class Jadx {
 
 	public static List<IDexTreeVisitor> getPreDecompilePassesList() {
 		List<IDexTreeVisitor> passes = new ArrayList<>();
+		passes.add(new SignatureProcessor());
 		passes.add(new RenameVisitor());
 		passes.add(new UsageInfoVisitor());
 		return passes;

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

@@ -6,7 +6,6 @@ import java.util.HashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
@@ -35,7 +34,6 @@ import jadx.core.dex.info.FieldInfo;
 import jadx.core.dex.info.MethodInfo;
 import jadx.core.dex.instructions.args.ArgType;
 import jadx.core.dex.instructions.args.LiteralArg;
-import jadx.core.dex.nodes.parser.SignatureParser;
 import jadx.core.dex.visitors.ProcessAnonymous;
 import jadx.core.utils.Utils;
 import jadx.core.utils.exceptions.JadxRuntimeException;
@@ -108,9 +106,6 @@ public class ClassNode extends NotificationAttrNode implements ILoadable, ICodeN
 			AnnotationsList.attach(this, cls.getAnnotations());
 			loadStaticValues(cls, fields);
 			initAccessFlags(cls);
-			parseClassSignature();
-			setFieldsTypesFromSignature();
-			methods.forEach(MethodNode::initMethodTypes);
 
 			addSourceFilenameAttr(cls.getSourceFile());
 			buildCache();
@@ -119,6 +114,12 @@ public class ClassNode extends NotificationAttrNode implements ILoadable, ICodeN
 		}
 	}
 
+	public void updateGenericClsData(ArgType superClass, List<ArgType> interfaces, List<ArgType> generics) {
+		this.superClass = superClass;
+		this.interfaces = interfaces;
+		this.generics = generics;
+	}
+
 	/**
 	 * Restore original access flags from Dalvik annotation if present
 	 */
@@ -176,62 +177,6 @@ public class ClassNode extends NotificationAttrNode implements ILoadable, ICodeN
 		root().getConstValues().processConstFields(this, staticFields);
 	}
 
-	/**
-	 * Class signature format:
-	 * https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.7.9.1
-	 */
-	private void parseClassSignature() {
-		SignatureParser sp = SignatureParser.fromNode(this);
-		if (sp == null) {
-			return;
-		}
-		try {
-			// parse class generic map
-			generics = sp.consumeGenericTypeParameters();
-			// parse super class signature
-			superClass = validateSuperCls(sp.consumeType(), superClass);
-			// parse interfaces signatures
-			for (int i = 0; i < interfaces.size(); i++) {
-				ArgType type = sp.consumeType();
-				if (type != null) {
-					interfaces.set(i, type);
-				} else {
-					break;
-				}
-			}
-		} catch (Exception e) {
-			LOG.error("Class signature parse error: {}", this, e);
-		}
-	}
-
-	private ArgType validateSuperCls(ArgType candidateType, ArgType currentType) {
-		if (!candidateType.isObject()) {
-			this.addComment("Incorrect class signature, super class is not object: " + SignatureParser.getSignature(this));
-			return currentType;
-		}
-		if (Objects.equals(candidateType.getObject(), this.getClassInfo().getType().getObject())) {
-			this.addComment("Incorrect class signature, super class is equals to this class: " + SignatureParser.getSignature(this));
-			return currentType;
-		}
-		return candidateType;
-	}
-
-	private void setFieldsTypesFromSignature() {
-		for (FieldNode field : fields) {
-			try {
-				SignatureParser sp = SignatureParser.fromNode(field);
-				if (sp != null) {
-					ArgType gType = sp.consumeType();
-					if (gType != null) {
-						field.setType(root.getTypeUtils().expandTypeVariables(this, gType));
-					}
-				}
-			} catch (Exception e) {
-				LOG.error("Field signature parse error: {}.{}", this.getFullName(), field.getName(), e);
-			}
-		}
-	}
-
 	private void addSourceFilenameAttr(String fileName) {
 		if (fileName == null) {
 			return;

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

@@ -35,6 +35,10 @@ public class FieldNode extends LineAttrNode implements ICodeNode {
 		this.accFlags = new AccessInfo(accessFlags, AFType.FIELD);
 	}
 
+	public void updateType(ArgType type) {
+		this.type = type;
+	}
+
 	public FieldInfo getFieldInfo() {
 		return fieldInfo;
 	}
@@ -65,10 +69,6 @@ public class FieldNode extends LineAttrNode implements ICodeNode {
 		return type;
 	}
 
-	public void setType(ArgType type) {
-		this.type = type;
-	}
-
 	public ClassNode getParentClass() {
 		return parentClass;
 	}

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

@@ -30,7 +30,6 @@ 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.parser.SignatureParser;
 import jadx.core.dex.nodes.utils.TypeUtils;
 import jadx.core.dex.regions.Region;
 import jadx.core.dex.trycatch.ExceptionHandler;
@@ -96,6 +95,10 @@ public class MethodNode extends NotificationAttrNode implements IMethodDetails,
 			this.codeReader = codeReader.copy();
 			this.insnsCount = codeReader.getInsnsCount();
 		}
+
+		this.retType = mthInfo.getReturnType();
+		this.argTypes = mthInfo.getArgumentsTypes();
+		this.typeParameters = Collections.emptyList();
 		unload();
 	}
 
@@ -119,6 +122,15 @@ public class MethodNode extends NotificationAttrNode implements IMethodDetails,
 		unloadAttributes();
 	}
 
+	public void updateTypes(List<ArgType> argTypes, ArgType retType) {
+		this.argTypes = argTypes;
+		this.retType = retType;
+	}
+
+	public void updateTypeParameters(List<ArgType> typeParameters) {
+		this.typeParameters = typeParameters;
+	}
+
 	@Override
 	public void load() throws DecodeException {
 		if (loaded) {
@@ -180,64 +192,6 @@ public class MethodNode extends NotificationAttrNode implements IMethodDetails,
 		}
 	}
 
-	public void initMethodTypes() {
-		if (!parseSignature()) {
-			this.retType = mthInfo.getReturnType();
-			this.argTypes = mthInfo.getArgumentsTypes();
-			this.typeParameters = Collections.emptyList();
-		}
-	}
-
-	private boolean parseSignature() {
-		SignatureParser sp = SignatureParser.fromNode(this);
-		if (sp == null) {
-			return false;
-		}
-		try {
-			this.typeParameters = sp.consumeGenericTypeParameters();
-			List<ArgType> parsedArgTypes = sp.consumeMethodArgs();
-			ArgType parsedRetType = sp.consumeType();
-
-			List<ArgType> mthArgs = mthInfo.getArgumentsTypes();
-			if (parsedArgTypes.size() != mthArgs.size()) {
-				if (parsedArgTypes.isEmpty()) {
-					return false;
-				}
-				if (!tryFixArgsCounts(parsedArgTypes, mthArgs)) {
-					addComment("Incorrect method signature, types: " + Utils.listToString(parsedArgTypes));
-					return false;
-				}
-			}
-			TypeUtils typeUtils = root().getTypeUtils();
-			this.retType = typeUtils.expandTypeVariables(this, parsedRetType);
-			this.argTypes = Collections.unmodifiableList(Utils.collectionMap(parsedArgTypes,
-					t -> typeUtils.expandTypeVariables(this, t)));
-			return true;
-		} catch (Exception e) {
-			addWarnComment("Failed to parse method signature: " + sp.getSignature(), e);
-			return false;
-		}
-	}
-
-	private boolean tryFixArgsCounts(List<ArgType> argsTypes, List<ArgType> mthArgs) {
-		if (!mthInfo.isConstructor()) {
-			return false;
-		}
-		if (getParentClass().getAccessFlags().isEnum()) {
-			if (mthArgs.size() >= 2) {
-				// TODO:
-				argsTypes.add(0, mthArgs.get(0));
-				argsTypes.add(1, mthArgs.get(1));
-			}
-		} else {
-			if (!mthArgs.isEmpty()) {
-				// add synthetic arg for outer class
-				argsTypes.add(0, mthArgs.get(0));
-			}
-		}
-		return argsTypes.size() == mthArgs.size();
-	}
-
 	private void initArguments(List<ArgType> args) {
 		int pos;
 		if (noCode) {

jadx-core/src/main/java/jadx/core/dex/nodes/utils/TypeUtils.java

@@ -13,6 +13,7 @@ import org.jetbrains.annotations.Nullable;
 import jadx.core.clsp.ClspClass;
 import jadx.core.dex.attributes.AType;
 import jadx.core.dex.attributes.nodes.MethodTypeVarsAttr;
+import jadx.core.dex.attributes.nodes.NotificationAttrNode;
 import jadx.core.dex.instructions.BaseInvokeNode;
 import jadx.core.dex.instructions.args.ArgType;
 import jadx.core.dex.instructions.args.InsnArg;
@@ -21,7 +22,6 @@ import jadx.core.dex.nodes.IMethodDetails;
 import jadx.core.dex.nodes.MethodNode;
 import jadx.core.dex.nodes.RootNode;
 import jadx.core.utils.Utils;
-import jadx.core.utils.exceptions.JadxRuntimeException;
 
 import static jadx.core.utils.Utils.isEmpty;
 import static jadx.core.utils.Utils.notEmpty;
@@ -48,19 +48,19 @@ public class TypeUtils {
 
 	public ArgType expandTypeVariables(ClassNode cls, ArgType type) {
 		if (type.containsTypeVariable()) {
-			expandTypeVar(type, cls.getGenericTypeParameters());
+			expandTypeVar(cls, type, cls.getGenericTypeParameters());
 		}
 		return type;
 	}
 
 	public ArgType expandTypeVariables(MethodNode mth, ArgType type) {
 		if (type.containsTypeVariable()) {
-			expandTypeVar(type, getKnownTypeVarsAtMethod(mth));
+			expandTypeVar(mth, type, getKnownTypeVarsAtMethod(mth));
 		}
 		return type;
 	}
 
-	private void expandTypeVar(ArgType type, Collection<ArgType> typeVars) {
+	private void expandTypeVar(NotificationAttrNode node, ArgType type, Collection<ArgType> typeVars) {
 		boolean allExtendsEmpty = true;
 		for (ArgType argType : typeVars) {
 			if (notEmpty(argType.getExtendTypes())) {
@@ -80,7 +80,7 @@ public class TypeUtils {
 						return null;
 					}
 				}
-				throw new JadxRuntimeException("Can't find type var definition: " + typeVarName);
+				node.addWarnComment("Unknown type variable: " + typeVarName + " in type: " + type);
 			}
 			return null;
 		});

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

+package jadx.core.dex.visitors;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+import jadx.core.dex.info.MethodInfo;
+import jadx.core.dex.instructions.args.ArgType;
+import jadx.core.dex.nodes.ClassNode;
+import jadx.core.dex.nodes.FieldNode;
+import jadx.core.dex.nodes.MethodNode;
+import jadx.core.dex.nodes.RootNode;
+import jadx.core.dex.nodes.parser.SignatureParser;
+import jadx.core.dex.nodes.utils.TypeUtils;
+import jadx.core.dex.visitors.typeinference.TypeCompareEnum;
+import jadx.core.utils.Utils;
+
+import static java.util.Collections.unmodifiableList;
+
+public class SignatureProcessor extends AbstractVisitor {
+
+	private RootNode root;
+
+	@Override
+	public void init(RootNode root) {
+		this.root = root;
+		for (ClassNode cls : this.root.getClasses()) {
+			processCls(cls);
+		}
+	}
+
+	private void processCls(ClassNode cls) {
+		parseClassSignature(cls);
+		for (FieldNode field : cls.getFields()) {
+			parseFieldSignature(field);
+		}
+		for (MethodNode mth : cls.getMethods()) {
+			parseMethodSignature(mth);
+		}
+	}
+
+	private void parseClassSignature(ClassNode cls) {
+		SignatureParser sp = SignatureParser.fromNode(cls);
+		if (sp == null) {
+			return;
+		}
+		try {
+			List<ArgType> generics = sp.consumeGenericTypeParameters();
+			ArgType superClass = validateClsType(cls, sp.consumeType(), cls.getSuperClass());
+			List<ArgType> interfaces = cls.getInterfaces();
+			for (int i = 0; i < interfaces.size(); i++) {
+				ArgType type = sp.consumeType();
+				if (type != null) {
+					interfaces.set(i, validateClsType(cls, type, interfaces.get(i)));
+				} else {
+					break;
+				}
+			}
+			cls.updateGenericClsData(superClass, interfaces, generics);
+		} catch (Exception e) {
+			cls.addWarnComment("Failed to parse class signature: " + sp.getSignature(), e);
+		}
+	}
+
+	private ArgType validateClsType(ClassNode cls, ArgType candidateType, ArgType currentType) {
+		if (!candidateType.isObject()) {
+			cls.addWarnComment("Incorrect class signature, class is not object: " + SignatureParser.getSignature(cls));
+			return currentType;
+		}
+		if (Objects.equals(candidateType.getObject(), cls.getClassInfo().getType().getObject())) {
+			cls.addWarnComment("Incorrect class signature, class is equals to this class: " + SignatureParser.getSignature(cls));
+			return currentType;
+		}
+		return candidateType;
+	}
+
+	private void parseFieldSignature(FieldNode field) {
+		SignatureParser sp = SignatureParser.fromNode(field);
+		if (sp == null) {
+			return;
+		}
+		ClassNode cls = field.getParentClass();
+		try {
+			ArgType gType = sp.consumeType();
+			if (gType == null) {
+				return;
+			}
+			ArgType type = root.getTypeUtils().expandTypeVariables(cls, gType);
+			if (!validateParsedType(type, field.getType())) {
+				cls.addWarnComment("Incorrect field signature: " + sp.getSignature());
+				return;
+			}
+			field.updateType(type);
+		} catch (Exception e) {
+			cls.addWarnComment("Field signature parse error: " + field.getName(), e);
+		}
+	}
+
+	private void parseMethodSignature(MethodNode mth) {
+		SignatureParser sp = SignatureParser.fromNode(mth);
+		if (sp == null) {
+			return;
+		}
+		try {
+			List<ArgType> typeParameters = sp.consumeGenericTypeParameters();
+			List<ArgType> parsedArgTypes = sp.consumeMethodArgs();
+			ArgType parsedRetType = sp.consumeType();
+
+			if (!validateParsedType(parsedRetType, mth.getMethodInfo().getReturnType())) {
+				mth.addWarnComment("Incorrect return type in method signature: " + sp.getSignature());
+				return;
+			}
+			List<ArgType> checkedArgTypes = checkArgTypes(mth, sp, parsedArgTypes);
+			if (checkedArgTypes == null) {
+				return;
+			}
+			mth.updateTypeParameters(typeParameters); // apply before expand args
+
+			TypeUtils typeUtils = root.getTypeUtils();
+			ArgType retType = typeUtils.expandTypeVariables(mth, parsedRetType);
+			List<ArgType> resultArgTypes = Utils.collectionMap(checkedArgTypes, t -> typeUtils.expandTypeVariables(mth, t));
+			mth.updateTypes(unmodifiableList(resultArgTypes), retType);
+		} catch (Exception e) {
+			mth.addWarnComment("Failed to parse method signature: " + sp.getSignature(), e);
+		}
+	}
+
+	private List<ArgType> checkArgTypes(MethodNode mth, SignatureParser sp, List<ArgType> parsedArgTypes) {
+		MethodInfo mthInfo = mth.getMethodInfo();
+		List<ArgType> mthArgTypes = mthInfo.getArgumentsTypes();
+		int len = parsedArgTypes.size();
+		if (len != mthArgTypes.size()) {
+			if (mth.getParentClass().getAccessFlags().isEnum()) {
+				// ignore for enums
+				return null;
+			}
+			if (mthInfo.isConstructor() && !mthArgTypes.isEmpty() && !parsedArgTypes.isEmpty()) {
+				// add synthetic arg for outer class (see test TestGeneric8)
+				ArrayList<ArgType> newArgTypes = new ArrayList<>(parsedArgTypes);
+				newArgTypes.add(0, mthArgTypes.get(0));
+				if (newArgTypes.size() == mthArgTypes.size()) {
+					return newArgTypes;
+				}
+			}
+			mth.addWarnComment("Incorrect args count in method signature: " + sp.getSignature());
+			return null;
+		}
+		for (int i = 0; i < len; i++) {
+			ArgType parsedType = parsedArgTypes.get(i);
+			ArgType mthArgType = mthArgTypes.get(i);
+			if (!validateParsedType(parsedType, mthArgType)) {
+				mth.addWarnComment("Incorrect types in method signature: " + sp.getSignature());
+				return null;
+			}
+		}
+		return parsedArgTypes;
+	}
+
+	private boolean validateParsedType(ArgType parsedType, ArgType currentType) {
+		TypeCompareEnum result = root.getTypeCompare().compareTypes(parsedType, currentType);
+		return result != TypeCompareEnum.CONFLICT;
+	}
+}

jadx-core/src/test/java/jadx/tests/integration/generics/TestClassSignature.java

@@ -15,6 +15,7 @@ public class TestClassSignature extends SmaliTest {
 
 	@Test
 	public void test() {
+		allowWarnInCode();
 		assertThat(getClassNodeFromSmali())
 				.code()
 				.containsOne("Incorrect class signature")

jadx-core/src/test/java/jadx/tests/integration/generics/TestGeneric8.java

+package jadx.tests.integration.generics;
+
+import org.junit.jupiter.api.Test;
+
+import jadx.tests.api.IntegrationTest;
+
+import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat;
+
+public class TestGeneric8 extends IntegrationTest {
+
+	public static class TestCls {
+		@SuppressWarnings("InnerClassMayBeStatic")
+		public class TestNumber<T extends Integer> {
+			private final T n;
+
+			public TestNumber(T n) {
+				this.n = n;
+			}
+
+			public boolean isEven() {
+				return n.intValue() % 2 == 0;
+			}
+		}
+	}
+
+	@Test
+	public void test() {
+		assertThat(getClassNode(TestCls.class))
+				.code()
+				.containsOne("public TestNumber(T n");
+	}
+}

jadx-core/src/test/java/jadx/tests/integration/others/TestIncorrectMethodSignature.java

+package jadx.tests.integration.others;
+
+import org.junit.jupiter.api.Test;
+
+import jadx.tests.api.SmaliTest;
+
+import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat;
+
+/**
+ * Issue #858.
+ * Incorrect method signature change argument type and shift register numbers
+ */
+public class TestIncorrectMethodSignature extends SmaliTest {
+
+	@Test
+	public void test() {
+		allowWarnInCode();
+		assertThat(getClassNodeFromSmali())
+				.code()
+				.containsOne("public TestIncorrectMethodSignature(String str) {");
+	}
+}

jadx-core/src/test/smali/others/TestIncorrectMethodSignature.smali

+.class public Lothers/TestIncorrectMethodSignature;
+.super Ljava/lang/RuntimeException;
+.source "TestIncorrectMethodSignature.java"
+
+.method public constructor <init>(Ljava/lang/String;)V
+    .registers 2
+
+    .annotation system Ldalvik/annotation/Signature;
+        value = {
+            "(J)V"
+        }
+    .end annotation
+
+    invoke-direct {p0, p1}, Ljava/lang/RuntimeException;-><init>(Ljava/lang/String;)V
+
+    return-void
+.end method