From 2d641bf049ea217632134b7f6b0588a5df5ae955 Mon Sep 17 00:00:00 2001 From: Skylot Date: Mon, 17 Aug 2020 21:11:33 +0100 Subject: [PATCH] fix: don't trust type info in signature, check before apply (#858) --- .../main/java/jadx/api/JadxDecompiler.java | 2 +- jadx-core/src/main/java/jadx/core/Jadx.java | 2 + .../java/jadx/core/dex/nodes/ClassNode.java | 67 +------ .../java/jadx/core/dex/nodes/FieldNode.java | 8 +- .../java/jadx/core/dex/nodes/MethodNode.java | 72 ++------ .../jadx/core/dex/nodes/utils/TypeUtils.java | 10 +- .../core/dex/visitors/SignatureProcessor.java | 163 ++++++++++++++++++ .../generics/TestClassSignature.java | 1 + .../integration/generics/TestGeneric8.java | 32 ++++ .../others/TestIncorrectMethodSignature.java | 22 +++ .../others/TestIncorrectMethodSignature.smali | 17 ++ 11 files changed, 266 insertions(+), 130 deletions(-) create mode 100644 jadx-core/src/main/java/jadx/core/dex/visitors/SignatureProcessor.java create mode 100644 jadx-core/src/test/java/jadx/tests/integration/generics/TestGeneric8.java create mode 100644 jadx-core/src/test/java/jadx/tests/integration/others/TestIncorrectMethodSignature.java create mode 100644 jadx-core/src/test/smali/others/TestIncorrectMethodSignature.smali diff --git a/jadx-core/src/main/java/jadx/api/JadxDecompiler.java b/jadx-core/src/main/java/jadx/api/JadxDecompiler.java index 05b63121c..34c026712 100644 --- a/jadx-core/src/main/java/jadx/api/JadxDecompiler.java +++ b/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() { diff --git a/jadx-core/src/main/java/jadx/core/Jadx.java b/jadx-core/src/main/java/jadx/core/Jadx.java index 09e97771e..f7eba51b1 100644 --- a/jadx-core/src/main/java/jadx/core/Jadx.java +++ b/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 getPreDecompilePassesList() { List passes = new ArrayList<>(); + passes.add(new SignatureProcessor()); passes.add(new RenameVisitor()); passes.add(new UsageInfoVisitor()); return passes; 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 fe9ff68bf..fe9518022 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 @@ -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 interfaces, List 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; diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/FieldNode.java b/jadx-core/src/main/java/jadx/core/dex/nodes/FieldNode.java index ce7d0541c..1c30a1fae 100644 --- a/jadx-core/src/main/java/jadx/core/dex/nodes/FieldNode.java +++ b/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; } diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/MethodNode.java b/jadx-core/src/main/java/jadx/core/dex/nodes/MethodNode.java index 65813824d..e8016ba9d 100644 --- a/jadx-core/src/main/java/jadx/core/dex/nodes/MethodNode.java +++ b/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 argTypes, ArgType retType) { + this.argTypes = argTypes; + this.retType = retType; + } + + public void updateTypeParameters(List 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 parsedArgTypes = sp.consumeMethodArgs(); - ArgType parsedRetType = sp.consumeType(); - - List 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 argsTypes, List 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 args) { int pos; if (noCode) { diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/utils/TypeUtils.java b/jadx-core/src/main/java/jadx/core/dex/nodes/utils/TypeUtils.java index 3cb27ae16..ebd15acd9 100644 --- a/jadx-core/src/main/java/jadx/core/dex/nodes/utils/TypeUtils.java +++ b/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 typeVars) { + private void expandTypeVar(NotificationAttrNode node, ArgType type, Collection 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; }); diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/SignatureProcessor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/SignatureProcessor.java new file mode 100644 index 000000000..608d37ae7 --- /dev/null +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/SignatureProcessor.java @@ -0,0 +1,163 @@ +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 generics = sp.consumeGenericTypeParameters(); + ArgType superClass = validateClsType(cls, sp.consumeType(), cls.getSuperClass()); + List 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 typeParameters = sp.consumeGenericTypeParameters(); + List 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 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 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 checkArgTypes(MethodNode mth, SignatureParser sp, List parsedArgTypes) { + MethodInfo mthInfo = mth.getMethodInfo(); + List 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 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; + } +} diff --git a/jadx-core/src/test/java/jadx/tests/integration/generics/TestClassSignature.java b/jadx-core/src/test/java/jadx/tests/integration/generics/TestClassSignature.java index d9a51126c..9bfcdec5e 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/generics/TestClassSignature.java +++ b/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") diff --git a/jadx-core/src/test/java/jadx/tests/integration/generics/TestGeneric8.java b/jadx-core/src/test/java/jadx/tests/integration/generics/TestGeneric8.java new file mode 100644 index 000000000..9a5a0089b --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/generics/TestGeneric8.java @@ -0,0 +1,32 @@ +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 { + 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"); + } +} diff --git a/jadx-core/src/test/java/jadx/tests/integration/others/TestIncorrectMethodSignature.java b/jadx-core/src/test/java/jadx/tests/integration/others/TestIncorrectMethodSignature.java new file mode 100644 index 000000000..ae7cf8442 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/others/TestIncorrectMethodSignature.java @@ -0,0 +1,22 @@ +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) {"); + } +} diff --git a/jadx-core/src/test/smali/others/TestIncorrectMethodSignature.smali b/jadx-core/src/test/smali/others/TestIncorrectMethodSignature.smali new file mode 100644 index 000000000..56506b00d --- /dev/null +++ b/jadx-core/src/test/smali/others/TestIncorrectMethodSignature.smali @@ -0,0 +1,17 @@ +.class public Lothers/TestIncorrectMethodSignature; +.super Ljava/lang/RuntimeException; +.source "TestIncorrectMethodSignature.java" + +.method public constructor (Ljava/lang/String;)V + .registers 2 + + .annotation system Ldalvik/annotation/Signature; + value = { + "(J)V" + } + .end annotation + + invoke-direct {p0, p1}, Ljava/lang/RuntimeException;->(Ljava/lang/String;)V + + return-void +.end method