Fix missing logging of core platform API violations in just-warn mode.

In one case a hiddenapi access check was done with
hiddenapi::AccessMethod::kNone for an app access, with the intention of
following it up with another "real" check that does log and applies the
enforcement policy.

However, hiddenapi::AccessMethod::kNone applied the current enforcement
policy, so the second check would always get skipped if the policy
isn't hiddenapi::EnforcementPolicy::kEnabled. That's a problem when
it's kJustWarn, because the first check won't log, and the second -
which would - is skipped.

Fix that by returning an access failure for the first check, regardless
of the policy, so that the second doesn't get skipped. The behaviour
doesn't change if the policy is kEnabled, and the second check won't do
anything if it's kDisabled, so this change only has effect when it's
kJustWarn.

Also rename kNone to the more apt kCheck.

For compatibility in other places where access checks with kNone aren't
used like above, introduce a new access method kCheckWithPolicy, which
keeps the behaviour of kNone (but with a more descriptive name). Some
of those code paths should perhaps be changed to kCheck as well, but
that's for another time.

Test: Boot and check logcat with planted API violations in system_server.
Test: 674-hiddenapi 690-hiddenapi-same-name-methods 691-hiddenapi-proxy
      817-hiddenapi 822-hiddenapi-future 999-redefine-hiddenapi
      2038-hiddenapi-jvmti-ext 2270-mh-internal-hiddenapi-use
      on host and target
Bug: 377676642
Change-Id: I4ca70c229bd6261a78dcd68990708a318b0e7588

7 files changed, 46 insertions, 26 deletions

diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 0ca970aa2d..73c90f58ac 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -5796,7 +5796,7 @@ bool ClassLinker::InitializeClass(Thread* self,
         DCHECK(!hiddenapi::ShouldDenyAccessToMember(
             field,
             hiddenapi::AccessContext(class_loader.Get(), dex_cache.Get()),
-            hiddenapi::AccessMethod::kNone));
+            hiddenapi::AccessMethod::kCheckWithPolicy));
         dex_cache->SetResolvedField(field_idx, field);
       } else {
         DCHECK_EQ(field, resolved_field);
@@ -10003,12 +10003,12 @@ ArtMethod* ClassLinker::FindResolvedMethod(ObjPtr<mirror::Class> klass,
   }
   DCHECK(resolved == nullptr || resolved->GetDeclaringClassUnchecked() != nullptr);
   if (resolved != nullptr &&
-      // We pass AccessMethod::kNone instead of kLinking to not warn yet on the
+      // We pass AccessMethod::kCheck instead of kLinking to not warn yet on the
       // access, as we'll be looking if the method can be accessed through an
       // interface.
       hiddenapi::ShouldDenyAccessToMember(resolved,
                                           hiddenapi::AccessContext(class_loader, dex_cache),
-                                          hiddenapi::AccessMethod::kNone)) {
+                                          hiddenapi::AccessMethod::kCheck)) {
     // The resolved method that we have found cannot be accessed due to
     // hiddenapi (typically it is declared up the hierarchy and is not an SDK
     // method). Try to find an interface method from the implemented interfaces which is
@@ -10017,11 +10017,12 @@ ArtMethod* ClassLinker::FindResolvedMethod(ObjPtr<mirror::Class> klass,
     if (itf_method == nullptr) {
       // No interface method. Call ShouldDenyAccessToMember again but this time
       // with AccessMethod::kLinking to ensure that an appropriate warning is
-      // logged.
-      hiddenapi::ShouldDenyAccessToMember(resolved,
-                                          hiddenapi::AccessContext(class_loader, dex_cache),
-                                          hiddenapi::AccessMethod::kLinking);
-      resolved = nullptr;
+      // logged and the enforcement policy is applied.
+      if (hiddenapi::ShouldDenyAccessToMember(resolved,
+                                              hiddenapi::AccessContext(class_loader, dex_cache),
+                                              hiddenapi::AccessMethod::kLinking)) {
+        resolved = nullptr;
+      }
     } else {
       // We found an interface method that is accessible, continue with the resolved method.
     }
@@ -10054,10 +10055,10 @@ static bool CheckNoSuchMethod(ArtMethod* method,
                               ObjPtr<mirror::ClassLoader> class_loader)
       REQUIRES_SHARED(Locks::mutator_lock_) {
   DCHECK(dex_cache->GetClassLoader().Ptr() == class_loader.Ptr());
-  return method == nullptr ||
-         hiddenapi::ShouldDenyAccessToMember(method,
-                                             hiddenapi::AccessContext(class_loader, dex_cache),
-                                             hiddenapi::AccessMethod::kNone);  // no warnings
+  return method == nullptr || hiddenapi::ShouldDenyAccessToMember(
+                                  method,
+                                  hiddenapi::AccessContext(class_loader, dex_cache),
+                                  hiddenapi::AccessMethod::kCheckWithPolicy);  // no warnings
 }
 
 ArtMethod* ClassLinker::FindIncompatibleMethod(ObjPtr<mirror::Class> klass,
diff --git a/runtime/hidden_api.cc b/runtime/hidden_api.cc
index 53010fc0f7..38bd7671e1 100644
--- a/runtime/hidden_api.cc
+++ b/runtime/hidden_api.cc
@@ -70,7 +70,8 @@ static const std::vector<std::string> kWarningExemptions = {
 
 static inline std::ostream& operator<<(std::ostream& os, AccessMethod value) {
   switch (value) {
-    case AccessMethod::kNone:
+    case AccessMethod::kCheck:
+    case AccessMethod::kCheckWithPolicy:
       LOG(FATAL) << "Internal access to hidden API should not be logged";
       UNREACHABLE();
     case AccessMethod::kReflection:
@@ -394,11 +395,12 @@ void MemberSignature::LogAccessToEventLog(uint32_t sampled_value,
                                           AccessMethod access_method,
                                           bool access_denied) {
 #ifdef ART_TARGET_ANDROID
-  if (access_method == AccessMethod::kLinking || access_method == AccessMethod::kNone) {
+  if (access_method == AccessMethod::kCheck || access_method == AccessMethod::kCheckWithPolicy ||
+      access_method == AccessMethod::kLinking) {
+    // Checks do not correspond to actual accesses, so should be ignored.
     // Linking warnings come from static analysis/compilation of the bytecode
-    // and can contain false positives (i.e. code that is never run). We choose
-    // not to log these in the event log.
-    // None does not correspond to actual access, so should also be ignored.
+    // and can contain false positives (i.e. code that is never run). Hence we
+    // choose to not log those either in the event log.
     return;
   }
   Runtime* runtime = Runtime::Current();
@@ -595,7 +597,13 @@ bool HandleCorePlatformApiViolation(T* member,
   DCHECK(policy != EnforcementPolicy::kDisabled)
       << "Should never enter this function when access checks are completely disabled";
 
-  if (access_method != AccessMethod::kNone) {
+  if (access_method == AccessMethod::kCheck) {
+    // Always return true for internal checks, so the current enforcement policy
+    // won't affect the caller.
+    return true;
+  }
+
+  if (access_method != AccessMethod::kCheckWithPolicy) {
     LOG(policy == EnforcementPolicy::kEnabled ? ERROR : WARNING)
         << "hiddenapi: Core platform API violation: "
         << Dumpable<MemberSignature>(MemberSignature(member))
@@ -664,7 +672,7 @@ bool ShouldDenyAccessToMemberImpl(T* member,
     }
   }
 
-  if (access_method != AccessMethod::kNone) {
+  if (access_method != AccessMethod::kCheck && access_method != AccessMethod::kCheckWithPolicy) {
     // Warn if blocked signature is being accessed or it is not exempted.
     if (deny_access || !member_signature.DoesPrefixMatchAny(kWarningExemptions)) {
       // Print a log message with information about this class member access.
diff --git a/runtime/hidden_api.h b/runtime/hidden_api.h
index f503767ed5..9c17f6641d 100644
--- a/runtime/hidden_api.h
+++ b/runtime/hidden_api.h
@@ -51,9 +51,20 @@ inline EnforcementPolicy EnforcementPolicyFromInt(int api_policy_int) {
 }
 
 // Hidden API access method
-// Thist must be kept in sync with VMRuntime.HiddenApiUsageLogger.ACCESS_METHOD_*
+// This must be kept in sync with VMRuntime.HiddenApiUsageLogger.ACCESS_METHOD_*
+// for the access methods that are logged.
 enum class AccessMethod {
-  kNone = 0,  // internal test that does not correspond to an actual access by app
+  // An internal check that does not correspond to an actual access by an app.
+  // It's not logged and the current EnforcementPolicy is not applied. The check
+  // can also be one that, if denied, will be followed by another check with one
+  // of the other methods below (except kCheckWithPolicy), which will then log
+  // and apply the policy (if that one is denied too).
+  kCheck = 0,
+
+  // Like kCheck, except the current EnforcementPolicy is applied (but it still
+  // doesn't log).
+  kCheckWithPolicy = 4,
+
   kReflection = 1,
   kJNI = 2,
   kLinking = 3,
diff --git a/runtime/hidden_api_test.cc b/runtime/hidden_api_test.cc
index 28f1d28c1f..5a2ee55723 100644
--- a/runtime/hidden_api_test.cc
+++ b/runtime/hidden_api_test.cc
@@ -191,7 +191,7 @@ class HiddenApiTest : public CommonRuntimeTest {
     // This is only used for log messages, so its state doesn't matter.
     const hiddenapi::AccessContext placeholder_context(/* is_trusted= */ false);
 
-    // Choose parameters such that there are no side effects (AccessMethod::kNone)
+    // Choose parameters such that there are no side effects (AccessMethod::kCheck)
     // and that the member is not on the exemptions list (here we choose one which
     // is not even in boot class path).
     return ShouldDenyAccessToMemberImpl(/* member= */ class1_field1_,
@@ -199,7 +199,7 @@ class HiddenApiTest : public CommonRuntimeTest {
                                         /* runtime_flags= */ 0,
                                         /* caller_context= */ placeholder_context,
                                         /* callee_context= */ placeholder_context,
-                                        /* access_method= */ hiddenapi::AccessMethod::kNone);
+                                        hiddenapi::AccessMethod::kCheck);
   }
 
   void TestLocation(const std::string& location, hiddenapi::Domain expected_domain) {
diff --git a/runtime/jni/jni_internal.cc b/runtime/jni/jni_internal.cc
index f1ea88da4e..588c2cd04d 100644
--- a/runtime/jni/jni_internal.cc
+++ b/runtime/jni/jni_internal.cc
@@ -494,7 +494,7 @@ ArtMethod* FindMethodJNI(const ScopedObjectAccess& soa,
     method = c->FindClassMethod(name, sig, pointer_size);
   }
   if (method != nullptr &&
-      ShouldDenyAccessToMember(method, soa.Self(), hiddenapi::AccessMethod::kNone)) {
+      ShouldDenyAccessToMember(method, soa.Self(), hiddenapi::AccessMethod::kCheckWithPolicy)) {
     // The resolved method that we have found cannot be accessed due to
     // hiddenapi (typically it is declared up the hierarchy and is not an SDK
     // method). Try to find an interface method from the implemented interfaces which is
diff --git a/runtime/mirror/class-inl.h b/runtime/mirror/class-inl.h
index 82e1462add..40f119e3c4 100644
--- a/runtime/mirror/class-inl.h
+++ b/runtime/mirror/class-inl.h
@@ -422,7 +422,7 @@ inline bool Class::IsDiscoverable(bool public_only,
   }
 
   return !hiddenapi::ShouldDenyAccessToMember(
-      member, access_context, hiddenapi::AccessMethod::kNone);
+      member, access_context, hiddenapi::AccessMethod::kCheckWithPolicy);
 }
 
 // Determine whether "this" is assignable from "src", where both of these
diff --git a/runtime/mirror/class.cc b/runtime/mirror/class.cc
index 327d2b78f1..a6d760a188 100644
--- a/runtime/mirror/class.cc
+++ b/runtime/mirror/class.cc
@@ -1962,7 +1962,7 @@ ObjPtr<Method> Class::GetDeclaredMethodInternal(
   }
   auto h_args = hs.NewHandle(args);
   Handle<Class> h_klass = hs.NewHandle(klass);
-  constexpr hiddenapi::AccessMethod access_method = hiddenapi::AccessMethod::kNone;
+  constexpr hiddenapi::AccessMethod access_method = hiddenapi::AccessMethod::kCheckWithPolicy;
   ArtMethod* result = nullptr;
   bool result_hidden = false;
   for (auto& m : h_klass->GetDeclaredVirtualMethods(kPointerSize)) {