Fix verifier/linker IncompatibleClassChangeError with hidden API

The verifier and class linker will attempt to find a method with
the wrong type if it could not be found with the original type,
i.e an interface method on a regular class and vice versa.

This logic did not previously take hidden API restrictions into
account and would result in bogus error messages to the user or
debug crashes.

Bug: 64382372
Bug: 77464273
Test: art/test.py -r -t 674-hiddenapi
Merged-In: If8327a70dd73b90249da3d9e505f0c6f89838f8e
Change-Id: If8327a70dd73b90249da3d9e505f0c6f89838f8e
(cherry picked from commit 54a99cfcf3d3463404fdf4152523dcc69b8648d7)
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 57c0d9d..44445ae 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -7884,6 +7884,40 @@
   return resolved;
 }
 
+// Returns true if `method` is either null or hidden.
+// Does not print any warnings if it is hidden.
+static bool CheckNoSuchMethod(ArtMethod* method,
+                              ObjPtr<mirror::DexCache> dex_cache,
+                              ObjPtr<mirror::ClassLoader> class_loader)
+      REQUIRES_SHARED(Locks::mutator_lock_) {
+  return method == nullptr ||
+         hiddenapi::GetMemberAction(method,
+                                    class_loader,
+                                    dex_cache,
+                                    hiddenapi::kNone)  // do not print warnings
+             == hiddenapi::kDeny;
+}
+
+ArtMethod* ClassLinker::FindIncompatibleMethod(ObjPtr<mirror::Class> klass,
+                                               ObjPtr<mirror::DexCache> dex_cache,
+                                               ObjPtr<mirror::ClassLoader> class_loader,
+                                               uint32_t method_idx) {
+  if (klass->IsInterface()) {
+    ArtMethod* method = klass->FindClassMethod(dex_cache, method_idx, image_pointer_size_);
+    return CheckNoSuchMethod(method, dex_cache, class_loader) ? nullptr : method;
+  } else {
+    // If there was an interface method with the same signature, we would have
+    // found it in the "copied" methods. Only DCHECK that the interface method
+    // really does not exist.
+    if (kIsDebugBuild) {
+      ArtMethod* method =
+          klass->FindInterfaceMethod(dex_cache, method_idx, image_pointer_size_);
+      DCHECK(CheckNoSuchMethod(method, dex_cache, class_loader));
+    }
+    return nullptr;
+  }
+}
+
 template <ClassLinker::ResolveMode kResolveMode>
 ArtMethod* ClassLinker::ResolveMethod(uint32_t method_idx,
                                       Handle<mirror::DexCache> dex_cache,
@@ -7959,13 +7993,7 @@
     // If we had a method, or if we can find one with another lookup type,
     // it's an incompatible-class-change error.
     if (resolved == nullptr) {
-      if (klass->IsInterface()) {
-        resolved = klass->FindClassMethod(dex_cache.Get(), method_idx, pointer_size);
-      } else {
-        // If there was an interface method with the same signature,
-        // we would have found it also in the "copied" methods.
-        DCHECK(klass->FindInterfaceMethod(dex_cache.Get(), method_idx, pointer_size) == nullptr);
-      }
+      resolved = FindIncompatibleMethod(klass, dex_cache.Get(), class_loader.Get(), method_idx);
     }
     if (resolved != nullptr) {
       ThrowIncompatibleClassChangeError(type, resolved->GetInvokeType(), resolved, referrer);
diff --git a/runtime/class_linker.h b/runtime/class_linker.h
index fa70f65..e935d1d 100644
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -329,6 +329,15 @@
                                 uint32_t method_idx)
       REQUIRES_SHARED(Locks::mutator_lock_);
 
+  // Find a method using the wrong lookup mechanism. If `klass` is an interface,
+  // search for a class method. If it is a class, search for an interface method.
+  // This is useful when throwing IncompatibleClassChangeError.
+  ArtMethod* FindIncompatibleMethod(ObjPtr<mirror::Class> klass,
+                                    ObjPtr<mirror::DexCache> dex_cache,
+                                    ObjPtr<mirror::ClassLoader> class_loader,
+                                    uint32_t method_idx)
+      REQUIRES_SHARED(Locks::mutator_lock_);
+
   // Resolve a method with a given ID from the DexFile associated with the given DexCache
   // and ClassLoader, storing the result in DexCache. The ClassLinker and ClassLoader are
   // used as in ResolveType. What is unique is the method type argument which is used to
diff --git a/runtime/hidden_api.cc b/runtime/hidden_api.cc
index 2d01fdd..fa47e3a 100644
--- a/runtime/hidden_api.cc
+++ b/runtime/hidden_api.cc
@@ -27,6 +27,9 @@
 
 static inline std::ostream& operator<<(std::ostream& os, AccessMethod value) {
   switch (value) {
+    case kNone:
+      LOG(FATAL) << "Internal access to hidden API should not be logged";
+      UNREACHABLE();
     case kReflection:
       os << "reflection";
       break;
@@ -115,6 +118,8 @@
 
 template<typename T>
 Action GetMemberActionImpl(T* member, Action action, AccessMethod access_method) {
+  DCHECK_NE(action, kAllow);
+
   // Get the signature, we need it later.
   MemberSignature member_signature(member);
 
@@ -134,10 +139,12 @@
     }
   }
 
-  // Print a log message with information about this class member access.
-  // We do this regardless of whether we block the access or not.
-  member_signature.WarnAboutAccess(access_method,
-      HiddenApiAccessFlags::DecodeFromRuntime(member->GetAccessFlags()));
+  if (access_method != kNone) {
+    // Print a log message with information about this class member access.
+    // We do this regardless of whether we block the access or not.
+    member_signature.WarnAboutAccess(access_method,
+        HiddenApiAccessFlags::DecodeFromRuntime(member->GetAccessFlags()));
+  }
 
   if (action == kDeny) {
     // Block access
@@ -147,16 +154,18 @@
   // Allow access to this member but print a warning.
   DCHECK(action == kAllowButWarn || action == kAllowButWarnAndToast);
 
-  // Depending on a runtime flag, we might move the member into whitelist and
-  // skip the warning the next time the member is accessed.
-  if (runtime->ShouldDedupeHiddenApiWarnings()) {
-    member->SetAccessFlags(HiddenApiAccessFlags::EncodeForRuntime(
-        member->GetAccessFlags(), HiddenApiAccessFlags::kWhitelist));
-  }
+  if (access_method != kNone) {
+    // Depending on a runtime flag, we might move the member into whitelist and
+    // skip the warning the next time the member is accessed.
+    if (runtime->ShouldDedupeHiddenApiWarnings()) {
+      member->SetAccessFlags(HiddenApiAccessFlags::EncodeForRuntime(
+          member->GetAccessFlags(), HiddenApiAccessFlags::kWhitelist));
+    }
 
-  // If this action requires a UI warning, set the appropriate flag.
-  if (action == kAllowButWarnAndToast || runtime->ShouldAlwaysSetHiddenApiWarningFlag()) {
-    runtime->SetPendingHiddenApiWarning(true);
+    // If this action requires a UI warning, set the appropriate flag.
+    if (action == kAllowButWarnAndToast || runtime->ShouldAlwaysSetHiddenApiWarningFlag()) {
+      runtime->SetPendingHiddenApiWarning(true);
+    }
   }
 
   return action;
diff --git a/runtime/hidden_api.h b/runtime/hidden_api.h
index 3ac0877..d2c71a7 100644
--- a/runtime/hidden_api.h
+++ b/runtime/hidden_api.h
@@ -53,6 +53,7 @@
 };
 
 enum AccessMethod {
+  kNone,  // internal test that does not correspond to an actual access by app
   kReflection,
   kJNI,
   kLinking,
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 72292c3..bc29aac 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -3801,16 +3801,8 @@
     must_fail = true;
     // Try to find the method also with the other type for better error reporting below
     // but do not store such bogus lookup result in the DexCache or VerifierDeps.
-    if (klass->IsInterface()) {
-      // NB This is normally not really allowed but we want to get any static or private object
-      // methods for error message purposes. This will never be returned.
-      // TODO We might want to change the verifier to not require this.
-      res_method = klass->FindClassMethod(dex_cache_.Get(), dex_method_idx, pointer_size);
-    } else {
-      // If there was an interface method with the same signature,
-      // we would have found it also in the "copied" methods.
-      DCHECK(klass->FindInterfaceMethod(dex_cache_.Get(), dex_method_idx, pointer_size) == nullptr);
-    }
+    res_method = class_linker->FindIncompatibleMethod(
+        klass, dex_cache_.Get(), class_loader_.Get(), dex_method_idx);
   }
 
   if (res_method == nullptr) {
diff --git a/test/674-hiddenapi/src-ex/ChildClass.java b/test/674-hiddenapi/src-ex/ChildClass.java
index ea66f16..e3d3e69 100644
--- a/test/674-hiddenapi/src-ex/ChildClass.java
+++ b/test/674-hiddenapi/src-ex/ChildClass.java
@@ -121,6 +121,7 @@
         checkLinking("LinkFieldGet" + suffix, /*takesParameter*/ false, expected);
         checkLinking("LinkFieldSet" + suffix, /*takesParameter*/ true, expected);
         checkLinking("LinkMethod" + suffix, /*takesParameter*/ false, expected);
+        checkLinking("LinkMethodInterface" + suffix, /*takesParameter*/ false, expected);
       }
 
       // Check whether Class.newInstance succeeds.
diff --git a/test/674-hiddenapi/src-ex/Linking.java b/test/674-hiddenapi/src-ex/Linking.java
index a89b92b..0fa0b19 100644
--- a/test/674-hiddenapi/src-ex/Linking.java
+++ b/test/674-hiddenapi/src-ex/Linking.java
@@ -174,6 +174,32 @@
   }
 }
 
+// INVOKE INSTANCE INTERFACE METHOD
+
+class LinkMethodInterfaceWhitelist {
+  public static int access() {
+    return DummyClass.getInterfaceInstance().methodPublicWhitelist();
+  }
+}
+
+class LinkMethodInterfaceLightGreylist {
+  public static int access() {
+    return DummyClass.getInterfaceInstance().methodPublicLightGreylist();
+  }
+}
+
+class LinkMethodInterfaceDarkGreylist {
+  public static int access() {
+    return DummyClass.getInterfaceInstance().methodPublicDarkGreylist();
+  }
+}
+
+class LinkMethodInterfaceBlacklist {
+  public static int access() {
+    return DummyClass.getInterfaceInstance().methodPublicBlacklist();
+  }
+}
+
 // INVOKE STATIC METHOD
 
 class LinkMethodStaticWhitelist {
@@ -199,3 +225,29 @@
     return ParentClass.methodPublicStaticBlacklist();
   }
 }
+
+// INVOKE INTERFACE STATIC METHOD
+
+class LinkMethodInterfaceStaticWhitelist {
+  public static int access() {
+    return ParentInterface.methodPublicStaticWhitelist();
+  }
+}
+
+class LinkMethodInterfaceStaticLightGreylist {
+  public static int access() {
+    return ParentInterface.methodPublicStaticLightGreylist();
+  }
+}
+
+class LinkMethodInterfaceStaticDarkGreylist {
+  public static int access() {
+    return ParentInterface.methodPublicStaticDarkGreylist();
+  }
+}
+
+class LinkMethodInterfaceStaticBlacklist {
+  public static int access() {
+    return ParentInterface.methodPublicStaticBlacklist();
+  }
+}
diff --git a/test/674-hiddenapi/src/DummyClass.java b/test/674-hiddenapi/src/DummyClass.java
new file mode 100644
index 0000000..51281a2
--- /dev/null
+++ b/test/674-hiddenapi/src/DummyClass.java
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2017 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 DummyClass implements ParentInterface {
+  public int methodPublicWhitelist() { return 1; }
+  public int methodPublicLightGreylist() { return 2; }
+  public int methodPublicDarkGreylist() { return 3; }
+  public int methodPublicBlacklist() { return 4; }
+
+  public static ParentInterface getInterfaceInstance() {
+    return new DummyClass();
+  }
+}
diff --git a/test/674-hiddenapi/src/ParentInterface.java b/test/674-hiddenapi/src/ParentInterface.java
index e36fe0e..f79ac9d 100644
--- a/test/674-hiddenapi/src/ParentInterface.java
+++ b/test/674-hiddenapi/src/ParentInterface.java
@@ -23,9 +23,9 @@
 
   // INSTANCE METHOD
   int methodPublicWhitelist();
-  int methodPublicBlacklist();
   int methodPublicLightGreylist();
   int methodPublicDarkGreylist();
+  int methodPublicBlacklist();
 
   // STATIC METHOD
   static int methodPublicStaticWhitelist() { return 21; }