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

5 files changed, 92 insertions, 0 deletions

diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 9990a373c9..f720558e51 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -5836,6 +5836,14 @@ bool ClassLinker::LinkVirtualMethods(
       // 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 7ca2378a07..f2ea2fdaaa 100644
--- a/runtime/hidden_api.h
+++ b/runtime/hidden_api.h
@@ -38,6 +38,7 @@ enum AccessMethod {
   kReflection,
   kJNI,
   kLinking,
+  kOverride,
 };
 
 inline std::ostream& operator<<(std::ostream& os, AccessMethod value) {
@@ -51,6 +52,9 @@ inline std::ostream& operator<<(std::ostream& os, AccessMethod value) {
     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 582e907ca3..8cd237ab6f 100644
--- a/test/674-hiddenapi/src-ex/ChildClass.java
+++ b/test/674-hiddenapi/src-ex/ChildClass.java
@@ -123,6 +123,9 @@ public class ChildClass {
           // 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 @@ public class ChildClass {
     }
   }
 
+  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 a89b92b2b9..b416250953 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 class Linking {
       }
     }
   }
+
+  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 0000000000..1f1f4d6aac
--- /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; }
+
+}