From 4e4c7f7d7b64ed491a117bc443cae8d7f8ac94a4 Mon Sep 17 00:00:00 2001 From: Skylot Date: Thu, 24 Sep 2020 17:43:16 +0100 Subject: [PATCH] fix: more visibility checks for @Override (#984) Signed-off-by: Skylot --- .../dex/visitors/OverrideMethodVisitor.java | 29 +++++--- .../TestOverridePackagePrivateMethod.java | 67 +++++++++++++++++++ .../TestOverridePackagePrivateMethod/A.smali | 7 ++ .../TestOverridePackagePrivateMethod/B.smali | 7 ++ .../TestOverridePackagePrivateMethod/C.smali | 7 ++ 5 files changed, 109 insertions(+), 8 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/others/TestOverridePackagePrivateMethod.java create mode 100644 jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/A.smali create mode 100644 jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/B.smali create mode 100644 jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/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 295b7a70..5739b137 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 @@ -31,14 +31,13 @@ public class OverrideMethodVisitor extends AbstractVisitor { @Override public boolean visit(ClassNode cls) throws JadxException { - RootNode root = cls.root(); List superTypes = collectSuperTypes(cls); for (MethodNode mth : cls.getMethods()) { if (mth.isConstructor() || mth.getAccessFlags().isStatic()) { continue; } String signature = mth.getMethodInfo().makeSignature(false); - List overrideList = collectOverrideMethods(root, superTypes, signature); + List overrideList = collectOverrideMethods(cls, superTypes, signature); if (!overrideList.isEmpty()) { mth.addAttr(new MethodOverrideAttr(overrideList)); fixMethodReturnType(mth, overrideList, superTypes); @@ -48,15 +47,14 @@ public class OverrideMethodVisitor extends AbstractVisitor { return true; } - private List collectOverrideMethods(RootNode root, List superTypes, String signature) { + private List collectOverrideMethods(ClassNode cls, List superTypes, String signature) { List overrideList = new ArrayList<>(); for (ArgType superType : superTypes) { - ClassNode classNode = root.resolveClass(superType); + ClassNode classNode = cls.root().resolveClass(superType); if (classNode != null) { for (MethodNode mth : classNode.getMethods()) { - AccessInfo accessFlags = mth.getAccessFlags(); - if (!accessFlags.isPrivate() - && !accessFlags.isStatic()) { + if (!mth.getAccessFlags().isStatic() + && isMethodVisibleInCls(mth, cls)) { String mthShortId = mth.getMethodInfo().getShortId(); if (mthShortId.startsWith(signature)) { overrideList.add(mth); @@ -64,7 +62,7 @@ public class OverrideMethodVisitor extends AbstractVisitor { } } } else { - ClspClass clsDetails = root.getClsp().getClsDetails(superType); + ClspClass clsDetails = cls.root().getClsp().getClsDetails(superType); if (clsDetails != null) { Map methodsMap = clsDetails.getMethodsMap(); for (Map.Entry entry : methodsMap.entrySet()) { @@ -79,6 +77,21 @@ public class OverrideMethodVisitor extends AbstractVisitor { return overrideList; } + /** + * NOTE: Simplified version of method from {@link ModVisitor#isFieldVisibleInMethod} + */ + private boolean isMethodVisibleInCls(MethodNode superMth, ClassNode cls) { + AccessInfo accessFlags = superMth.getAccessFlags(); + if (accessFlags.isPrivate()) { + return false; + } + if (accessFlags.isPublic() || accessFlags.isProtected()) { + return true; + } + // package-private + return Objects.equals(superMth.getParentClass().getPackage(), cls.getPackage()); + } + private List collectSuperTypes(ClassNode cls) { Map superTypes = new HashMap<>(); collectSuperTypes(cls, superTypes); diff --git a/jadx-core/src/test/java/jadx/tests/integration/others/TestOverridePackagePrivateMethod.java b/jadx-core/src/test/java/jadx/tests/integration/others/TestOverridePackagePrivateMethod.java new file mode 100644 index 00000000..3b392e1c --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/others/TestOverridePackagePrivateMethod.java @@ -0,0 +1,67 @@ +package jadx.tests.integration.others; + +import java.util.List; + +import org.junit.jupiter.api.Test; + +import jadx.NotYetImplemented; +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.SmaliTest; + +import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat; + +public class TestOverridePackagePrivateMethod extends SmaliTest { + // @formatter:off + /* + ----------------------------------------------------------- + package test; + + public class A { + void a() { // package-private + } + } + ----------------------------------------------------------- + package test; + + public class B extends A { + @Override // test.A + public void a() { + } + } + ----------------------------------------------------------- + package other; + + import test.A; + + public class C extends A { + // No @Override here + public void a() { + } + } + ----------------------------------------------------------- + */ + // @formatter:on + + @NotYetImplemented("Don't change access modifiers if not needed") + @Test + public void test() { + commonChecks(); + } + + @Test + public void testDontChangeAccFlags() { + getArgs().setRespectBytecodeAccModifiers(true); + commonChecks(); + } + + private void commonChecks() { + List classes = loadFromSmaliFiles(); + assertThat(searchCls(classes, "test.A")) + .code() + .doesNotContain("/* access modifiers changed") + .containsLine(1, "void a() {"); + + assertThat(searchCls(classes, "test.B")).code().containsOne("@Override"); + assertThat(searchCls(classes, "other.C")).code().doesNotContain("@Override"); + } +} diff --git a/jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/A.smali b/jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/A.smali new file mode 100644 index 00000000..7e4ecb88 --- /dev/null +++ b/jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/A.smali @@ -0,0 +1,7 @@ +.class public Ltest/A; +.super Ljava/lang/Object; + +.method a()V # package-private + .locals 1 + return-void +.end method diff --git a/jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/B.smali b/jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/B.smali new file mode 100644 index 00000000..61642037 --- /dev/null +++ b/jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/B.smali @@ -0,0 +1,7 @@ +.class public Ltest/B; +.super Ltest/A; + +.method public a()V + .locals 1 + return-void +.end method diff --git a/jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/C.smali b/jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/C.smali new file mode 100644 index 00000000..19824e7e --- /dev/null +++ b/jadx-core/src/test/smali/others/TestOverridePackagePrivateMethod/C.smali @@ -0,0 +1,7 @@ +.class public Lother/C; +.super Ltest/A; + +.method public a()V + .locals 1 + return-void +.end method -- GitLab