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; }