Merge "Refactor enforcement of hidden API policy when linking"

10 files changed, 97 insertions, 43 deletions

diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 8769013637..dc79c7e9b1 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -7934,6 +7934,10 @@ ArtMethod* ClassLinker::FindResolvedMethod(ObjPtr<mirror::Class> klass,
     resolved = klass->FindClassMethod(dex_cache, method_idx, image_pointer_size_);
   }
   DCHECK(resolved == nullptr || resolved->GetDeclaringClassUnchecked() != nullptr);
+  if (resolved != nullptr &&
+      hiddenapi::ShouldBlockAccessToMember(resolved, class_loader, hiddenapi::kLinking)) {
+    resolved = nullptr;
+  }
   if (resolved != nullptr) {
     // In case of jmvti, the dex file gets verified before being registered, so first
     // check if it's registered before checking class tables.
@@ -8072,7 +8076,10 @@ ArtMethod* ClassLinker::ResolveMethodWithoutInvokeType(uint32_t method_idx,
   } else {
     resolved = klass->FindClassMethod(dex_cache.Get(), method_idx, image_pointer_size_);
   }
-
+  if (resolved != nullptr &&
+      hiddenapi::ShouldBlockAccessToMember(resolved, class_loader.Get(), hiddenapi::kLinking)) {
+    resolved = nullptr;
+  }
   return resolved;
 }
 
@@ -8146,11 +8153,16 @@ ArtField* ClassLinker::ResolveField(uint32_t field_idx,
     } else {
       resolved = klass->FindInstanceField(name, type);
     }
-    if (resolved == nullptr) {
-      ThrowNoSuchFieldError(is_static ? "static " : "instance ", klass, type, name);
-      return nullptr;
-    }
   }
+
+  if (resolved == nullptr ||
+      hiddenapi::ShouldBlockAccessToMember(resolved, class_loader.Get(), hiddenapi::kLinking)) {
+    const char* name = dex_file.GetFieldName(field_id);
+    const char* type = dex_file.GetFieldTypeDescriptor(field_id);
+    ThrowNoSuchFieldError(is_static ? "static " : "instance ", klass, type, name);
+    return nullptr;
+  }
+
   dex_cache->SetResolvedField(field_idx, resolved, image_pointer_size_);
   return resolved;
 }
@@ -8176,6 +8188,10 @@ ArtField* ClassLinker::ResolveFieldJLS(uint32_t field_idx,
   StringPiece name(dex_file.GetFieldName(field_id));
   StringPiece type(dex_file.GetFieldTypeDescriptor(field_id));
   resolved = mirror::Class::FindField(self, klass, name, type);
+  if (resolved != nullptr &&
+      hiddenapi::ShouldBlockAccessToMember(resolved, class_loader.Get(), hiddenapi::kLinking)) {
+    resolved = nullptr;
+  }
   if (resolved != nullptr) {
     dex_cache->SetResolvedField(field_idx, resolved, image_pointer_size_);
   } else {
diff --git a/runtime/hidden_api.h b/runtime/hidden_api.h
index e0519a07da..7ca2378a07 100644
--- a/runtime/hidden_api.h
+++ b/runtime/hidden_api.h
@@ -17,7 +17,10 @@
 #ifndef ART_RUNTIME_HIDDEN_API_H_
 #define ART_RUNTIME_HIDDEN_API_H_
 
+#include "art_field-inl.h"
+#include "art_method-inl.h"
 #include "dex/hidden_api_access_flags.h"
+#include "mirror/class-inl.h"
 #include "reflection.h"
 #include "runtime.h"
 
@@ -33,7 +36,8 @@ enum Action {
 
 enum AccessMethod {
   kReflection,
-  kJNI
+  kJNI,
+  kLinking,
 };
 
 inline std::ostream& operator<<(std::ostream& os, AccessMethod value) {
@@ -44,6 +48,9 @@ inline std::ostream& operator<<(std::ostream& os, AccessMethod value) {
     case kJNI:
       os << "JNI";
       break;
+    case kLinking:
+      os << "linking";
+      break;
   }
   return os;
 }
@@ -88,7 +95,7 @@ inline void WarnAboutMemberAccess(ArtMethod* method, AccessMethod access_method)
 // class path or not. Because different users of this function determine this
 // in a different way, `fn_caller_in_boot(self)` is called and should return
 // true if the caller is in boot class path.
-// This function might print warnings into the log if the member is greylisted.
+// This function might print warnings into the log if the member is hidden.
 template<typename T>
 inline bool ShouldBlockAccessToMember(T* member,
                                       Thread* self,
@@ -145,27 +152,19 @@ inline bool ShouldBlockAccessToMember(T* member,
   return false;
 }
 
-// Returns true if access to member with `access_flags` should be denied to `caller`.
-// This function should be called on statically linked uses of hidden API.
-inline bool ShouldBlockAccessToMember(uint32_t access_flags, mirror::Class* caller)
+// Returns true if access to `member` should be denied to a caller loaded with
+// `caller_class_loader`.
+// This function might print warnings into the log if the member is hidden.
+template<typename T>
+inline bool ShouldBlockAccessToMember(T* member,
+                                      ObjPtr<mirror::ClassLoader> caller_class_loader,
+                                      AccessMethod access_method)
     REQUIRES_SHARED(Locks::mutator_lock_) {
-  if (!Runtime::Current()->AreHiddenApiChecksEnabled()) {
-    // Exit early. Nothing to enforce.
-    return false;
-  }
-
-  // Only continue if we want to deny access. Warnings are *not* printed.
-  if (GetMemberAction(access_flags) != kDeny) {
-    return false;
-  }
-
-  // Member is hidden. Check if the caller is in boot class path.
-  if (caller == nullptr) {
-    // The caller is unknown. We assume that this is *not* boot class path.
-    return true;
-  }
-
-  return !caller->IsBootStrapClassLoaded();
+  bool caller_in_boot = (caller_class_loader.IsNull());
+  return ShouldBlockAccessToMember(member,
+                                   /* thread */ nullptr,
+                                   [caller_in_boot] (Thread*) { return caller_in_boot; },
+                                   access_method);
 }
 
 }  // namespace hiddenapi
diff --git a/runtime/interpreter/unstarted_runtime.cc b/runtime/interpreter/unstarted_runtime.cc
index 600561b85c..76df65f730 100644
--- a/runtime/interpreter/unstarted_runtime.cc
+++ b/runtime/interpreter/unstarted_runtime.cc
@@ -182,7 +182,9 @@ template<typename T>
 static ALWAYS_INLINE bool ShouldBlockAccessToMember(T* member, ShadowFrame* frame)
     REQUIRES_SHARED(Locks::mutator_lock_) {
   return hiddenapi::ShouldBlockAccessToMember(
-      member->GetAccessFlags(), frame->GetMethod()->GetDeclaringClass());
+      member,
+      frame->GetMethod()->GetDeclaringClass()->GetClassLoader(),
+      hiddenapi::kReflection);  // all uses in this file are from reflection
 }
 
 void UnstartedRuntime::UnstartedClassForNameCommon(Thread* self,
diff --git a/runtime/mirror/class-inl.h b/runtime/mirror/class-inl.h
index f63f105c3a..f0898f49d3 100644
--- a/runtime/mirror/class-inl.h
+++ b/runtime/mirror/class-inl.h
@@ -1151,10 +1151,6 @@ inline bool Class::CanAccessMember(ObjPtr<Class> access_to, uint32_t member_flag
   if (this == access_to) {
     return true;
   }
-  // Do not allow non-boot class path classes access hidden APIs.
-  if (hiddenapi::ShouldBlockAccessToMember(member_flags, this)) {
-    return false;
-  }
   // Public members are trivially accessible
   if (member_flags & kAccPublic) {
     return true;
diff --git a/test/674-hiddenapi/api-blacklist.txt b/test/674-hiddenapi/api-blacklist.txt
index d43360c62f..4a67fb8ebf 100644
--- a/test/674-hiddenapi/api-blacklist.txt
+++ b/test/674-hiddenapi/api-blacklist.txt
@@ -1,9 +1,11 @@
 LNullaryConstructorBlacklist;-><init>()V
 LParentClass;->fieldPublicBlacklist:I
+LParentClass;->fieldPublicBlacklistB:I
 LParentClass;->fieldPackageBlacklist:I
 LParentClass;->fieldProtectedBlacklist:I
 LParentClass;->fieldPrivateBlacklist:I
 LParentClass;->fieldPublicStaticBlacklist:I
+LParentClass;->fieldPublicStaticBlacklistB:I
 LParentClass;->fieldPackageStaticBlacklist:I
 LParentClass;->fieldProtectedStaticBlacklist:I
 LParentClass;->fieldPrivateStaticBlacklist:I
diff --git a/test/674-hiddenapi/api-dark-greylist.txt b/test/674-hiddenapi/api-dark-greylist.txt
index d0f35f64bc..e010a0a07f 100644
--- a/test/674-hiddenapi/api-dark-greylist.txt
+++ b/test/674-hiddenapi/api-dark-greylist.txt
@@ -1,9 +1,11 @@
 LNullaryConstructorDarkGreylist;-><init>()V
 LParentClass;->fieldPublicDarkGreylist:I
+LParentClass;->fieldPublicDarkGreylistB:I
 LParentClass;->fieldPackageDarkGreylist:I
 LParentClass;->fieldProtectedDarkGreylist:I
 LParentClass;->fieldPrivateDarkGreylist:I
 LParentClass;->fieldPublicStaticDarkGreylist:I
+LParentClass;->fieldPublicStaticDarkGreylistB:I
 LParentClass;->fieldPackageStaticDarkGreylist:I
 LParentClass;->fieldProtectedStaticDarkGreylist:I
 LParentClass;->fieldPrivateStaticDarkGreylist:I
diff --git a/test/674-hiddenapi/api-light-greylist.txt b/test/674-hiddenapi/api-light-greylist.txt
index 2809025cfd..4be793fd0c 100644
--- a/test/674-hiddenapi/api-light-greylist.txt
+++ b/test/674-hiddenapi/api-light-greylist.txt
@@ -1,9 +1,11 @@
 LNullaryConstructorLightGreylist;-><init>()V
 LParentClass;->fieldPublicLightGreylist:I
+LParentClass;->fieldPublicLightGreylistB:I
 LParentClass;->fieldPackageLightGreylist:I
 LParentClass;->fieldProtectedLightGreylist:I
 LParentClass;->fieldPrivateLightGreylist:I
 LParentClass;->fieldPublicStaticLightGreylist:I
+LParentClass;->fieldPublicStaticLightGreylistB:I
 LParentClass;->fieldPackageStaticLightGreylist:I
 LParentClass;->fieldProtectedStaticLightGreylist:I
 LParentClass;->fieldPrivateStaticLightGreylist:I
diff --git a/test/674-hiddenapi/src-ex/ChildClass.java b/test/674-hiddenapi/src-ex/ChildClass.java
index babd88359b..582e907ca3 100644
--- a/test/674-hiddenapi/src-ex/ChildClass.java
+++ b/test/674-hiddenapi/src-ex/ChildClass.java
@@ -120,7 +120,7 @@ public class ChildClass {
           // Check whether one can use a class constructor.
           checkConstructor(ParentClass.class, visibility, hiddenness, expected);
 
-          // Check whether you can use an interface default method.
+          // 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);
         }
@@ -389,7 +389,7 @@ public class ChildClass {
   private static void checkLinking(String className, boolean takesParameter, Behaviour behaviour)
       throws Exception {
     boolean canAccess = (behaviour != Behaviour.Denied);
-    boolean setsWarning = false;  // we do not set the flag in verifier or at runtime
+    boolean setsWarning = (behaviour == Behaviour.Warning);
 
     clearWarning();
     if (Linking.canAccess(className, takesParameter) != canAccess) {
diff --git a/test/674-hiddenapi/src-ex/Linking.java b/test/674-hiddenapi/src-ex/Linking.java
index c6735d85fe..a89b92b2b9 100644
--- a/test/674-hiddenapi/src-ex/Linking.java
+++ b/test/674-hiddenapi/src-ex/Linking.java
@@ -27,7 +27,7 @@ public class Linking {
       }
       return true;
     } catch (InvocationTargetException ex) {
-      if (ex.getCause() instanceof IllegalAccessError) {
+      if (ex.getCause() instanceof NoSuchFieldError || ex.getCause() instanceof NoSuchMethodError) {
         return false;
       } else {
         throw ex;
@@ -66,25 +66,29 @@ class LinkFieldGetBlacklist {
 
 class LinkFieldSetWhitelist {
   public static void access(int x) {
-    new ParentClass().fieldPublicWhitelist = x;
+    // Need to use a different field from the getter to bypass DexCache.
+    new ParentClass().fieldPublicWhitelistB = x;
   }
 }
 
 class LinkFieldSetLightGreylist {
   public static void access(int x) {
-    new ParentClass().fieldPublicLightGreylist = x;
+    // Need to use a different field from the getter to bypass DexCache.
+    new ParentClass().fieldPublicLightGreylistB = x;
   }
 }
 
 class LinkFieldSetDarkGreylist {
   public static void access(int x) {
-    new ParentClass().fieldPublicDarkGreylist = x;
+    // Need to use a different field from the getter to bypass DexCache.
+    new ParentClass().fieldPublicDarkGreylistB = x;
   }
 }
 
 class LinkFieldSetBlacklist {
   public static void access(int x) {
-    new ParentClass().fieldPublicBlacklist = x;
+    // Need to use a different field from the getter to bypass DexCache.
+    new ParentClass().fieldPublicBlacklistB = x;
   }
 }
 
@@ -118,25 +122,29 @@ class LinkFieldGetStaticBlacklist {
 
 class LinkFieldSetStaticWhitelist {
   public static void access(int x) {
-    ParentClass.fieldPublicStaticWhitelist = x;
+    // Need to use a different field from the getter to bypass DexCache.
+    ParentClass.fieldPublicStaticWhitelistB = x;
   }
 }
 
 class LinkFieldSetStaticLightGreylist {
   public static void access(int x) {
-    ParentClass.fieldPublicStaticLightGreylist = x;
+    // Need to use a different field from the getter to bypass DexCache.
+    ParentClass.fieldPublicStaticLightGreylistB = x;
   }
 }
 
 class LinkFieldSetStaticDarkGreylist {
   public static void access(int x) {
-    ParentClass.fieldPublicStaticDarkGreylist = x;
+    // Need to use a different field from the getter to bypass DexCache.
+    ParentClass.fieldPublicStaticDarkGreylistB = x;
   }
 }
 
 class LinkFieldSetStaticBlacklist {
   public static void access(int x) {
-    ParentClass.fieldPublicStaticBlacklist = x;
+    // Need to use a different field from the getter to bypass DexCache.
+    ParentClass.fieldPublicStaticBlacklistB = x;
   }
 }
 
diff --git a/test/674-hiddenapi/src/ParentClass.java b/test/674-hiddenapi/src/ParentClass.java
index edad02dc2c..07e84ccad5 100644
--- a/test/674-hiddenapi/src/ParentClass.java
+++ b/test/674-hiddenapi/src/ParentClass.java
@@ -23,21 +23,25 @@ public class ParentClass {
   int fieldPackageWhitelist = 212;
   protected int fieldProtectedWhitelist = 213;
   private int fieldPrivateWhitelist = 214;
+  public int fieldPublicWhitelistB = 215;
 
   public int fieldPublicLightGreylist = 221;
   int fieldPackageLightGreylist = 222;
   protected int fieldProtectedLightGreylist = 223;
   private int fieldPrivateLightGreylist = 224;
+  public int fieldPublicLightGreylistB = 225;
 
   public int fieldPublicDarkGreylist = 231;
   int fieldPackageDarkGreylist = 232;
   protected int fieldProtectedDarkGreylist = 233;
   private int fieldPrivateDarkGreylist = 234;
+  public int fieldPublicDarkGreylistB = 235;
 
   public int fieldPublicBlacklist = 241;
   int fieldPackageBlacklist = 242;
   protected int fieldProtectedBlacklist = 243;
   private int fieldPrivateBlacklist = 244;
+  public int fieldPublicBlacklistB = 245;
 
   // STATIC FIELD
 
@@ -45,21 +49,25 @@ public class ParentClass {
   static int fieldPackageStaticWhitelist = 112;
   protected static int fieldProtectedStaticWhitelist = 113;
   private static int fieldPrivateStaticWhitelist = 114;
+  public static int fieldPublicStaticWhitelistB = 115;
 
   public static int fieldPublicStaticLightGreylist = 121;
   static int fieldPackageStaticLightGreylist = 122;
   protected static int fieldProtectedStaticLightGreylist = 123;
   private static int fieldPrivateStaticLightGreylist = 124;
+  public static int fieldPublicStaticLightGreylistB = 125;
 
   public static int fieldPublicStaticDarkGreylist = 131;
   static int fieldPackageStaticDarkGreylist = 132;
   protected static int fieldProtectedStaticDarkGreylist = 133;
   private static int fieldPrivateStaticDarkGreylist = 134;
+  public static int fieldPublicStaticDarkGreylistB = 135;
 
   public static int fieldPublicStaticBlacklist = 141;
   static int fieldPackageStaticBlacklist = 142;
   protected static int fieldProtectedStaticBlacklist = 143;
   private static int fieldPrivateStaticBlacklist = 144;
+  public static int fieldPublicStaticBlacklistB = 145;
 
   // INSTANCE METHOD
 
@@ -130,4 +138,23 @@ public class ParentClass {
   ParentClass(float x, char y) {}
   protected ParentClass(long x, char y) {}
   private ParentClass(double x, char y) {}
+
+  // HELPERS
+
+  public int callMethodPublicWhitelist() { return methodPublicWhitelist(); }
+  public int callMethodPackageWhitelist() { return methodPackageWhitelist(); }
+  public int callMethodProtectedWhitelist() { return methodProtectedWhitelist(); }
+
+  public int callMethodPublicLightGreylist() { return methodPublicLightGreylist(); }
+  public int callMethodPackageLightGreylist() { return methodPackageLightGreylist(); }
+  public int callMethodProtectedLightGreylist() { return methodProtectedLightGreylist(); }
+
+  public int callMethodPublicDarkGreylist() { return methodPublicDarkGreylist(); }
+  public int callMethodPackageDarkGreylist() { return methodPackageDarkGreylist(); }
+  public int callMethodProtectedDarkGreylist() { return methodProtectedDarkGreylist(); }
+
+  public int callMethodPublicBlacklist() { return methodPublicBlacklist(); }
+  public int callMethodPackageBlacklist() { return methodPackageBlacklist(); }
+  public int callMethodProtectedBlacklist() { return methodProtectedBlacklist(); }
+
 }