From e1b7d361b9f40f8979b8bc5d8602d555337df21f Mon Sep 17 00:00:00 2001 From: Skylot Date: Fri, 9 Dec 2022 17:06:48 +0000 Subject: [PATCH] fix: check full signature for search method override (#1743) --- .../dex/visitors/OverrideMethodVisitor.java | 26 ++++++- .../others/TestOverrideWithSameName.java | 67 +++++++++++++++++++ .../others/TestOverrideWithSameName/A.smali | 8 +++ .../others/TestOverrideWithSameName/B.smali | 10 +++ .../others/TestOverrideWithSameName/C.smali | 8 +++ 5 files changed, 116 insertions(+), 3 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/others/TestOverrideWithSameName.java create mode 100644 jadx-core/src/test/smali/others/TestOverrideWithSameName/A.smali create mode 100644 jadx-core/src/test/smali/others/TestOverrideWithSameName/B.smali create mode 100644 jadx-core/src/test/smali/others/TestOverrideWithSameName/C.smali diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/OverrideMethodVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/OverrideMethodVisitor.java index d55998e5..18a74258 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/OverrideMethodVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/OverrideMethodVisitor.java @@ -90,7 +90,7 @@ public class OverrideMethodVisitor extends AbstractVisitor { for (ArgType superType : superData.getSuperTypes()) { ClassNode classNode = mth.root().resolveClass(superType); if (classNode != null) { - MethodNode ovrdMth = searchOverriddenMethod(classNode, signature); + MethodNode ovrdMth = searchOverriddenMethod(classNode, mth, signature); if (ovrdMth != null) { if (isMethodVisibleInCls(ovrdMth, cls)) { overrideList.add(ovrdMth); @@ -107,6 +107,8 @@ public class OverrideMethodVisitor extends AbstractVisitor { Map methodsMap = clsDetails.getMethodsMap(); for (Map.Entry entry : methodsMap.entrySet()) { String mthShortId = entry.getKey(); + // do not check full signature, classpath methods can be trusted + // i.e. doesn't contain methods with same signature in one class if (mthShortId.startsWith(signature)) { overrideList.add(entry.getValue()); break; @@ -130,12 +132,30 @@ public class OverrideMethodVisitor extends AbstractVisitor { } @Nullable - private MethodNode searchOverriddenMethod(ClassNode cls, String signature) { + private MethodNode searchOverriddenMethod(ClassNode cls, MethodNode mth, String signature) { + // search by exact full signature (with return value) to fight obfuscation (see test + // 'TestOverrideWithSameName') + String shortId = mth.getMethodInfo().getShortId(); for (MethodNode supMth : cls.getMethods()) { - if (!supMth.getAccessFlags().isStatic() && supMth.getMethodInfo().getShortId().startsWith(signature)) { + if (supMth.getMethodInfo().getShortId().equals(shortId) && !supMth.getAccessFlags().isStatic()) { return supMth; } } + // search by signature without return value and check if return value is wider type + for (MethodNode supMth : cls.getMethods()) { + if (supMth.getMethodInfo().getShortId().startsWith(signature) && !supMth.getAccessFlags().isStatic()) { + TypeCompare typeCompare = cls.root().getTypeCompare(); + ArgType supRetType = supMth.getMethodInfo().getReturnType(); + ArgType mthRetType = mth.getMethodInfo().getReturnType(); + TypeCompareEnum res = typeCompare.compareTypes(supRetType, mthRetType); + if (res.isWider()) { + return supMth; + } + if (res == TypeCompareEnum.UNKNOWN || res == TypeCompareEnum.CONFLICT) { + mth.addDebugComment("Possible override for method " + supMth.getMethodInfo().getFullId()); + } + } + } return null; } diff --git a/jadx-core/src/test/java/jadx/tests/integration/others/TestOverrideWithSameName.java b/jadx-core/src/test/java/jadx/tests/integration/others/TestOverrideWithSameName.java new file mode 100644 index 00000000..ae5d3d69 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/others/TestOverrideWithSameName.java @@ -0,0 +1,67 @@ +package jadx.tests.integration.others; + +import java.util.List; + +import org.junit.jupiter.api.Test; + +import jadx.core.dex.attributes.AType; +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.SmaliTest; + +import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat; + +@SuppressWarnings("CommentedOutCode") +public class TestOverrideWithSameName extends SmaliTest { + + //@formatter:off + /* + interface A { + B a(); + C a(); + } + + abstract class B implements A { + @Override + public C a() { + return null; + } + } + + public class C extends B { + @Override + public B a() { + return null; + } + } + */ + //@formatter:on + + @Test + public void test() { + List clsNodes = loadFromSmaliFiles(); + assertThat(searchCls(clsNodes, "test.A")) + .code() + .containsOne("C mo0a();") // assume second method was renamed + .doesNotContain("@Override"); + + ClassNode bCls = searchCls(clsNodes, "test.B"); + assertThat(bCls) + .code() + .containsOne("C mo0a() {") + .containsOne("@Override"); + + assertThat(getMethod(bCls, "a").get(AType.METHOD_OVERRIDE).getOverrideList()) + .singleElement() + .satisfies(mth -> assertThat(mth.getMethodInfo().getDeclClass().getShortName()).isEqualTo("A")); + + ClassNode cCls = searchCls(clsNodes, "test.C"); + assertThat(cCls) + .code() + .containsOne("B a() {") + .containsOne("@Override"); + + assertThat(getMethod(cCls, "a").get(AType.METHOD_OVERRIDE).getOverrideList()) + .singleElement() + .satisfies(mth -> assertThat(mth.getMethodInfo().getDeclClass().getShortName()).isEqualTo("A")); + } +} diff --git a/jadx-core/src/test/smali/others/TestOverrideWithSameName/A.smali b/jadx-core/src/test/smali/others/TestOverrideWithSameName/A.smali new file mode 100644 index 00000000..9d6ac300 --- /dev/null +++ b/jadx-core/src/test/smali/others/TestOverrideWithSameName/A.smali @@ -0,0 +1,8 @@ +.class interface abstract Ltest/A; +.super Ljava/lang/Object; + +.method public abstract a()Ltest/B; +.end method + +.method public abstract a()Ltest/C; +.end method diff --git a/jadx-core/src/test/smali/others/TestOverrideWithSameName/B.smali b/jadx-core/src/test/smali/others/TestOverrideWithSameName/B.smali new file mode 100644 index 00000000..b35ee532 --- /dev/null +++ b/jadx-core/src/test/smali/others/TestOverrideWithSameName/B.smali @@ -0,0 +1,10 @@ +.class abstract Ltest/B; +.super Ljava/lang/Object; + +.implements Ltest/A; + +.method public a()Ltest/C; + .registers 2 + const/4 v0, 0x0 + return-object v0 +.end method diff --git a/jadx-core/src/test/smali/others/TestOverrideWithSameName/C.smali b/jadx-core/src/test/smali/others/TestOverrideWithSameName/C.smali new file mode 100644 index 00000000..731d3b1c --- /dev/null +++ b/jadx-core/src/test/smali/others/TestOverrideWithSameName/C.smali @@ -0,0 +1,8 @@ +.class public Ltest/C; +.super Ltest/B; + +.method public a()Ltest/B; + .registers 2 + const/4 v0, 0x0 + return-object v0 +.end method -- GitLab