Warn on overriding of hidden methods
We could prevent apps from overriding hidden methods in the same
manner they cannot override a package-private method - by creating
a separate vtable entry for the child method. For now, start by
printing a warning when a hidden method is being overridden but do
not change the semantics.
Bug: 64382372
Test: art/test.py -r -t 674-hiddenapi
Change-Id: I9d5bfa6b833a4c0f5aaffa5f82dbe9b1e1f03f1f
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 9990a37..f720558 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -5836,6 +5836,14 @@
// smaller as we go on.
uint32_t hash_index = hash_table.FindAndRemove(&super_method_name_comparator);
if (hash_index != hash_table.GetNotFoundIndex()) {
+ // Run a check whether we are going to override a method which is hidden
+ // to `klass`, but ignore the result as we only warn at the moment.
+ // We cannot do this test earlier because we need to establish that
+ // a method is being overridden first. ShouldBlockAccessToMember would
+ // print bogus warnings otherwise.
+ hiddenapi::ShouldBlockAccessToMember(
+ super_method, klass->GetClassLoader(), hiddenapi::kOverride);
+
ArtMethod* virtual_method = klass->GetVirtualMethodDuringLinking(
hash_index, image_pointer_size_);
if (super_method->IsFinal()) {
diff --git a/runtime/hidden_api.h b/runtime/hidden_api.h
index 7ca2378..f2ea2fd 100644
--- a/runtime/hidden_api.h
+++ b/runtime/hidden_api.h
@@ -38,6 +38,7 @@
kReflection,
kJNI,
kLinking,
+ kOverride,
};
inline std::ostream& operator<<(std::ostream& os, AccessMethod value) {
@@ -51,6 +52,9 @@
case kLinking:
os << "linking";
break;
+ case kOverride:
+ os << "override";
+ break;
}
return os;
}
diff --git a/test/674-hiddenapi/src-ex/ChildClass.java b/test/674-hiddenapi/src-ex/ChildClass.java
index 582e907..8cd237a 100644
--- a/test/674-hiddenapi/src-ex/ChildClass.java
+++ b/test/674-hiddenapi/src-ex/ChildClass.java
@@ -123,6 +123,9 @@
// Check whether one can use an interface default method.
String name = "method" + visibility.name() + "Default" + hiddenness.name();
checkMethod(ParentInterface.class, name, /*isStatic*/ false, visibility, expected);
+
+ // Check whether one can override this method.
+ checkOverriding(suffix, isStatic, visibility, expected);
}
// Test whether static linking succeeds.
@@ -403,6 +406,37 @@
}
}
+ private static void checkOverriding(String suffix,
+ boolean isStatic,
+ Visibility visibility,
+ Behaviour behaviour) throws Exception {
+ if (isStatic || visibility == Visibility.Private) {
+ // Does not make sense to override a static or private method.
+ return;
+ }
+
+ // The classes are in the same package, but will be able to access each
+ // other only if loaded with the same class loader, here the boot class loader.
+ boolean canAccess = (visibility != Visibility.Package) || (isParentInBoot && isChildInBoot);
+ boolean setsWarning = false; // warnings may be set during vtable linking
+
+ String methodName = "callMethod" + visibility.name() + suffix;
+
+ // Force the test class to link its vtable, which may cause warnings, before
+ // the actual test.
+ new OverrideClass().methodPublicWhitelist();
+
+ clearWarning();
+ if (Linking.canOverride(methodName) != canAccess) {
+ throw new RuntimeException("Expected to " + (canAccess ? "" : "not ") +
+ "be able to override " + methodName + "." +
+ "isParentInBoot = " + isParentInBoot + ", " + "isChildInBoot = " + isChildInBoot);
+ }
+ if (canAccess && hasPendingWarning() != setsWarning) {
+ throwWarningException(ParentClass.class, methodName, false, "static linking", setsWarning);
+ }
+ }
+
private static void throwDiscoveryException(Class<?> klass, String name, boolean isField,
String fn, boolean canAccess) {
throw new RuntimeException("Expected " + (isField ? "field " : "method ") + klass.getName() +
diff --git a/test/674-hiddenapi/src-ex/Linking.java b/test/674-hiddenapi/src-ex/Linking.java
index a89b92b..b416250 100644
--- a/test/674-hiddenapi/src-ex/Linking.java
+++ b/test/674-hiddenapi/src-ex/Linking.java
@@ -14,6 +14,7 @@
* limitations under the License.
*/
+import java.lang.reflect.Method;
import java.lang.reflect.InvocationTargetException;
public class Linking {
@@ -34,6 +35,16 @@
}
}
}
+
+ public static boolean canOverride(String methodName) throws Exception {
+ // ParentClass returns only positive numbers, OverrideClass only negative.
+ // This way we can tell if OverrideClass managed to override the original
+ // method or not.
+ Method method = ParentClass.class.getDeclaredMethod(methodName);
+ int result1 = (int) method.invoke(new ParentClass());
+ int result2 = (int) method.invoke(new OverrideClass());
+ return (result1 > 0) && (result2 < 0);
+ }
}
// INSTANCE FIELD GET
diff --git a/test/674-hiddenapi/src-ex/OverrideClass.java b/test/674-hiddenapi/src-ex/OverrideClass.java
new file mode 100644
index 0000000..1f1f4d6
--- /dev/null
+++ b/test/674-hiddenapi/src-ex/OverrideClass.java
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2018 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+public class OverrideClass extends ParentClass {
+
+ @Override public int methodPublicWhitelist() { return -411; }
+ @Override int methodPackageWhitelist() { return -412; }
+ @Override protected int methodProtectedWhitelist() { return -413; }
+
+ @Override public int methodPublicLightGreylist() { return -421; }
+ @Override int methodPackageLightGreylist() { return -422; }
+ @Override protected int methodProtectedLightGreylist() { return -423; }
+
+ @Override public int methodPublicDarkGreylist() { return -431; }
+ @Override int methodPackageDarkGreylist() { return -432; }
+ @Override protected int methodProtectedDarkGreylist() { return -433; }
+
+ @Override public int methodPublicBlacklist() { return -441; }
+ @Override int methodPackageBlacklist() { return -442; }
+ @Override protected int methodProtectedBlacklist() { return -443; }
+
+}