Fix logic around SDK visibility of interface methods.

- FindIncompatibleMethod needs to look for an accessible interface SDK method
  instead of all interface methods.
- We need to check which API list a method belongs to in FindAccessibleInterfaceMethod
  as the public API bit in the access flags can be updated for
  optimizations.

Test: 817-hiddenapi
Bug: 218589191
Change-Id: I274b234657c6dc3fe61278924774896488d4c708

10 files changed, 92 insertions, 4 deletions

diff --git a/libartbase/base/hiddenapi_flags.h b/libartbase/base/hiddenapi_flags.h
index 64d3faeef1..aef4a73bee 100644
--- a/libartbase/base/hiddenapi_flags.h
+++ b/libartbase/base/hiddenapi_flags.h
@@ -326,6 +326,10 @@ class ApiList {
     return GetValue() == Value::kBlocked;
   }
 
+  bool IsSdkApi() const {
+    return GetValue() == Value::kSdk;
+  }
+
   // Returns true if the ApiList is a test API.
   bool IsTestApi() const {
     return helper::MatchesBitMask(helper::ToBit(DomainApi::kTestApi), dex_flags_);
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index f4a13e9fcf..428136a7cc 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -9118,7 +9118,8 @@ ArtMethod* ClassLinker::FindIncompatibleMethod(ObjPtr<mirror::Class> klass,
     if (kIsDebugBuild) {
       ArtMethod* method =
           klass->FindInterfaceMethod(dex_cache, method_idx, image_pointer_size_);
-      DCHECK(CheckNoSuchMethod(method, dex_cache, class_loader));
+      CHECK(CheckNoSuchMethod(method, dex_cache, class_loader) ||
+            (klass->FindAccessibleInterfaceMethod(method, image_pointer_size_) == nullptr));
     }
     return nullptr;
   }
diff --git a/runtime/hidden_api.h b/runtime/hidden_api.h
index febe80875a..1eb5e1751f 100644
--- a/runtime/hidden_api.h
+++ b/runtime/hidden_api.h
@@ -215,7 +215,7 @@ class MemberSignature {
   void NotifyHiddenApiListener(AccessMethod access_method);
 };
 
-// Locates hiddenapi flags for `field` in the corresponding dex file.
+// Locates hiddenapi flags for `member` in the corresponding dex file.
 // NB: This is an O(N) operation, linear with the number of members in the class def.
 template<typename T>
 uint32_t GetDexFlags(T* member) REQUIRES_SHARED(Locks::mutator_lock_);
diff --git a/runtime/mirror/class.cc b/runtime/mirror/class.cc
index 40ff7c04c6..c9df1f0460 100644
--- a/runtime/mirror/class.cc
+++ b/runtime/mirror/class.cc
@@ -2120,7 +2120,13 @@ ArtMethod* Class::FindAccessibleInterfaceMethod(ArtMethod* implementation_method
         ArtMethod* interface_method = &iface->GetVirtualMethodsSlice(pointer_size)[j];
         // If the interface method is part of the public SDK, return it.
         if ((hiddenapi::GetRuntimeFlags(interface_method) & kAccPublicApi) != 0) {
-          return interface_method;
+          hiddenapi::ApiList api_list(hiddenapi::detail::GetDexFlags(interface_method));
+          // The kAccPublicApi flag is also used as an optimization to avoid
+          // other hiddenapi checks to always go on the slow path. Therefore, we
+          // need to check here if the method is in the SDK list.
+          if (api_list.IsSdkApi()) {
+            return interface_method;
+          }
         }
       }
     }
diff --git a/test/817-hiddenapi/hiddenapi-flags.csv b/test/817-hiddenapi/hiddenapi-flags.csv
index ee0ede1b7f..d3e4877ce2 100644
--- a/test/817-hiddenapi/hiddenapi-flags.csv
+++ b/test/817-hiddenapi/hiddenapi-flags.csv
@@ -1,2 +1,5 @@
 LNotInAbstractParent;->methodPublicSdkNotInAbstractParent()I,blocked
 LOtherInterface;->methodPublicSdkNotInAbstractParent()I,blocked
+LNotInAbstractParent;->methodUnsupportedNotInAbstractParent()I,blocked
+LOtherInterface;->methodUnsupportedNotInAbstractParent()I,blocked
+LNotInAbstractInterface;->methodUnsupportedNotInAbstractParent()I,unsupported
diff --git a/test/817-hiddenapi/src-ex/TestCase.java b/test/817-hiddenapi/src-ex/TestCase.java
index ed3ecc8211..b9ea1790dc 100644
--- a/test/817-hiddenapi/src-ex/TestCase.java
+++ b/test/817-hiddenapi/src-ex/TestCase.java
@@ -17,6 +17,11 @@
 public class TestCase {
 
   public static void test() {
+    testPublicSdk();
+    testUnsupportedAppUsage();
+  }
+
+  public static void testPublicSdk() {
     // This call should be successful as the method is accessible through the interface.
     int value = new InheritAbstract().methodPublicSdkNotInAbstractParent();
     if (value != 42) {
@@ -24,13 +29,54 @@ public class TestCase {
     }
   }
 
+  public static void testUnsupportedAppUsage() {
+    // This call should throw an exception as the only accessible method is unsupportedappusage.
+    try {
+      new InheritAbstract().methodUnsupportedNotInAbstractParent();
+      throw new Error("Expected NoSuchMethodError");
+    } catch (NoSuchMethodError e) {
+      // Expected.
+    }
+  }
+
   public static void testNative(String library) {
     System.load(library);
     int value = testNativeInternal();
     if (value != 42) {
       throw new Error("Expected 42, got " + value);
     }
+
+    // Test that we consistently return we cannot access a hidden method.
+
+    // Dedupe hidden api warnings to trigger the optimization described below.
+    dedupeHiddenApiWarnings();
+    assertFalse(testAccessInternal(
+        InheritAbstract.class, "methodUnsupportedNotInAbstractParent", "()I"));
+    // Access the accessible method through reflection. This will do an optimization pretending the
+    // method is public API.
+    try {
+      NotInAbstractInterface.class.getDeclaredMethod("methodUnsupportedNotInAbstractParent").
+          invoke(new InheritAbstract());
+    } catch (Exception e) {
+      throw new Error(e);
+    }
+    assertFalse(testAccessInternal(
+        InheritAbstract.class, "methodUnsupportedNotInAbstractParent", "()I"));
+  }
+
+  public static void assertTrue(boolean value) {
+    if (!value) {
+      throw new Error("Expected true value");
+    }
+  }
+
+  public static void assertFalse(boolean value) {
+    if (value) {
+      throw new Error("Expected false value");
+    }
   }
 
   public static native int testNativeInternal();
+  public static native void dedupeHiddenApiWarnings();
+  public static native boolean testAccessInternal(Class<?> cls, String method, String signature);
 }
diff --git a/test/817-hiddenapi/src/NotInAbstractInterface.java b/test/817-hiddenapi/src/NotInAbstractInterface.java
index 1400e21e7e..e55369c5f4 100644
--- a/test/817-hiddenapi/src/NotInAbstractInterface.java
+++ b/test/817-hiddenapi/src/NotInAbstractInterface.java
@@ -17,4 +17,7 @@
 public interface NotInAbstractInterface {
   // This method will be part of the public SDK.
   int methodPublicSdkNotInAbstractParent();
+
+  // This method will be unsupportedappusage.
+  int methodUnsupportedNotInAbstractParent();
 }
diff --git a/test/817-hiddenapi/src/NotInAbstractParent.java b/test/817-hiddenapi/src/NotInAbstractParent.java
index 464cffddb6..a82e5df344 100644
--- a/test/817-hiddenapi/src/NotInAbstractParent.java
+++ b/test/817-hiddenapi/src/NotInAbstractParent.java
@@ -19,4 +19,8 @@ public abstract class NotInAbstractParent {
   public int methodPublicSdkNotInAbstractParent() {
     return 42;
   }
+  // This method will be in the blocklist.
+  public int methodUnsupportedNotInAbstractParent() {
+    return 43;
+  }
 }
diff --git a/test/817-hiddenapi/src/OtherInterface.java b/test/817-hiddenapi/src/OtherInterface.java
index fd9c87e1da..a5f6b80f23 100644
--- a/test/817-hiddenapi/src/OtherInterface.java
+++ b/test/817-hiddenapi/src/OtherInterface.java
@@ -14,6 +14,6 @@
  * limitations under the License.
  */
 interface OtherInterface {
-  // This method will not be part of the public SDK.
+  // This method will be blocked.
   int methodPublicSdkNotInAbstractParent();
 }
diff --git a/test/817-hiddenapi/test_native.cc b/test/817-hiddenapi/test_native.cc
index 1c4dd8e51b..d99fb06220 100644
--- a/test/817-hiddenapi/test_native.cc
+++ b/test/817-hiddenapi/test_native.cc
@@ -18,6 +18,9 @@
 
 #include <android-base/logging.h>
 
+#include "nativehelper/ScopedUtfChars.h"
+#include "runtime.h"
+
 namespace art {
 
 extern "C" JNIEXPORT jint JNICALL Java_TestCase_testNativeInternal(JNIEnv* env,
@@ -34,4 +37,22 @@ extern "C" JNIEXPORT jint JNICALL Java_TestCase_testNativeInternal(JNIEnv* env,
   return env->CallIntMethod(obj, method_id);
 }
 
+extern "C" JNIEXPORT jboolean JNICALL Java_TestCase_testAccessInternal(JNIEnv* env,
+                                                                       jclass,
+                                                                       jclass cls,
+                                                                       jstring method_name,
+                                                                       jstring signature) {
+  ScopedUtfChars chars_method(env, method_name);
+  ScopedUtfChars chars_signature(env, signature);
+  if (env->GetMethodID(cls, chars_method.c_str(), chars_signature.c_str()) != nullptr) {
+    return true;
+  }
+  env->ExceptionClear();
+  return false;
+}
+
+extern "C" JNIEXPORT void JNICALL Java_TestCase_dedupeHiddenApiWarnings(JNIEnv*, jclass) {
+  Runtime::Current()->SetDedupeHiddenApiWarnings(true);
+}
+
 }  // namespace art