Refactor enforcement of hidden API policy when linking
Hidden API enforcement when linking was sub-optimal for two reasons:
a) it was based on Class::CanAccessMember and would throw
IllegalAccessError instead of NoSuchMethodError/NoSuchFieldError, and
b) no warnings were printed in the code path.
This patch moves the checks into ClassLinker's resolution routines and
uses a code path which prints warning.
Bug: 64382372
Test: art/test.py -r -t 674-hiddenapi
Change-Id: I8a0fbdca58ce5803c1588b644a3e627a0a9a6504
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 72c110a..9990a37 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -7907,6 +7907,10 @@
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.
@@ -8045,7 +8049,10 @@
} 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;
}
@@ -8119,11 +8126,16 @@
} 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;
}
@@ -8149,6 +8161,10 @@
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 e0519a0..7ca2378 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 AccessMethod {
kReflection,
- kJNI
+ kJNI,
+ kLinking,
};
inline std::ostream& operator<<(std::ostream& os, AccessMethod value) {
@@ -44,6 +48,9 @@
case kJNI:
os << "JNI";
break;
+ case kLinking:
+ os << "linking";
+ break;
}
return os;
}
@@ -88,7 +95,7 @@
// 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 @@
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 600561b..76df65f 100644
--- a/runtime/interpreter/unstarted_runtime.cc
+++ b/runtime/interpreter/unstarted_runtime.cc
@@ -182,7 +182,9 @@
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 ee7d217..60062c8 100644
--- a/runtime/mirror/class-inl.h
+++ b/runtime/mirror/class-inl.h
@@ -1144,10 +1144,6 @@
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 d43360c..4a67fb8 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 d0f35f6..e010a0a 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 2809025..4be793f 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 babd883..582e907 100644
--- a/test/674-hiddenapi/src-ex/ChildClass.java
+++ b/test/674-hiddenapi/src-ex/ChildClass.java
@@ -120,7 +120,7 @@
// 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 @@
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 c6735d8..a89b92b 100644
--- a/test/674-hiddenapi/src-ex/Linking.java
+++ b/test/674-hiddenapi/src-ex/Linking.java
@@ -27,7 +27,7 @@
}
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 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 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 edad02d..07e84cc 100644
--- a/test/674-hiddenapi/src/ParentClass.java
+++ b/test/674-hiddenapi/src/ParentClass.java
@@ -23,21 +23,25 @@
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 @@
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 @@
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(); }
+
}