ART: Stash a resolved method late in the verifier
Invoke-interface should only be called on an interface method.
We cannot move the check earlier, as there are other checks
that must be done that can fail a class hard. So postpone
a push to the dex cache.
Clean up the test a bit.
Also templatize ResolveMethod with a version always checking
the invoke type, and on a cache miss check whether type target
type is an interface when an interface invoke type was given.
Bug: 21869691
Change-Id: I94cbb23339cbbb3cb6be9995775e4dcefacce7fd
diff --git a/runtime/class_linker-inl.h b/runtime/class_linker-inl.h
index 88a3996..a5d10b2 100644
--- a/runtime/class_linker-inl.h
+++ b/runtime/class_linker-inl.h
@@ -116,6 +116,7 @@
return resolved_method;
}
+template <ClassLinker::ResolveMode kResolveMode>
inline ArtMethod* ClassLinker::ResolveMethod(Thread* self,
uint32_t method_idx,
ArtMethod* referrer,
@@ -127,12 +128,12 @@
Handle<mirror::DexCache> h_dex_cache(hs.NewHandle(declaring_class->GetDexCache()));
Handle<mirror::ClassLoader> h_class_loader(hs.NewHandle(declaring_class->GetClassLoader()));
const DexFile* dex_file = h_dex_cache->GetDexFile();
- resolved_method = ResolveMethod(*dex_file,
- method_idx,
- h_dex_cache,
- h_class_loader,
- referrer,
- type);
+ resolved_method = ResolveMethod<kResolveMode>(*dex_file,
+ method_idx,
+ h_dex_cache,
+ h_class_loader,
+ referrer,
+ type);
}
// Note: We cannot check here to see whether we added the method to the cache. It
// might be an erroneous class, which results in it being hidden from us.
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 0d10b4e..f5085ed 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -6162,6 +6162,7 @@
return resolved;
}
+template <ClassLinker::ResolveMode kResolveMode>
ArtMethod* ClassLinker::ResolveMethod(const DexFile& dex_file,
uint32_t method_idx,
Handle<mirror::DexCache> dex_cache,
@@ -6173,6 +6174,12 @@
ArtMethod* resolved = dex_cache->GetResolvedMethod(method_idx, image_pointer_size_);
if (resolved != nullptr && !resolved->IsRuntimeMethod()) {
DCHECK(resolved->GetDeclaringClassUnchecked() != nullptr) << resolved->GetDexMethodIndex();
+ if (kResolveMode == ClassLinker::kForceICCECheck) {
+ if (resolved->CheckIncompatibleClassChange(type)) {
+ ThrowIncompatibleClassChangeError(type, resolved->GetInvokeType(), resolved, referrer);
+ return nullptr;
+ }
+ }
return resolved;
}
// Fail, get the declaring class.
@@ -6191,8 +6198,36 @@
DCHECK(resolved == nullptr || resolved->GetDeclaringClassUnchecked() != nullptr);
break;
case kInterface:
- resolved = klass->FindInterfaceMethod(dex_cache.Get(), method_idx, image_pointer_size_);
- DCHECK(resolved == nullptr || resolved->GetDeclaringClass()->IsInterface());
+ // We have to check whether the method id really belongs to an interface (dex static bytecode
+ // constraint A15). Otherwise you must not invoke-interface on it.
+ //
+ // This is not symmetric to A12-A14 (direct, static, virtual), as using FindInterfaceMethod
+ // assumes that the given type is an interface, and will check the interface table if the
+ // method isn't declared in the class. So it may find an interface method (usually by name
+ // in the handling below, but we do the constraint check early). In that case,
+ // CheckIncompatibleClassChange will succeed (as it is called on an interface method)
+ // unexpectedly.
+ // Example:
+ // interface I {
+ // foo()
+ // }
+ // class A implements I {
+ // ...
+ // }
+ // class B extends A {
+ // ...
+ // }
+ // invoke-interface B.foo
+ // -> FindInterfaceMethod finds I.foo (interface method), not A.foo (miranda method)
+ if (UNLIKELY(!klass->IsInterface())) {
+ ThrowIncompatibleClassChangeError(klass,
+ "Found class %s, but interface was expected",
+ PrettyDescriptor(klass).c_str());
+ return nullptr;
+ } else {
+ resolved = klass->FindInterfaceMethod(dex_cache.Get(), method_idx, image_pointer_size_);
+ DCHECK(resolved == nullptr || resolved->GetDeclaringClass()->IsInterface());
+ }
break;
case kSuper: // Fall-through.
case kVirtual:
@@ -6794,4 +6829,20 @@
}
}
+// Instantiate ResolveMethod.
+template ArtMethod* ClassLinker::ResolveMethod<ClassLinker::kForceICCECheck>(
+ const DexFile& dex_file,
+ uint32_t method_idx,
+ Handle<mirror::DexCache> dex_cache,
+ Handle<mirror::ClassLoader> class_loader,
+ ArtMethod* referrer,
+ InvokeType type);
+template ArtMethod* ClassLinker::ResolveMethod<ClassLinker::kNoICCECheckForCache>(
+ const DexFile& dex_file,
+ uint32_t method_idx,
+ Handle<mirror::DexCache> dex_cache,
+ Handle<mirror::ClassLoader> class_loader,
+ ArtMethod* referrer,
+ InvokeType type);
+
} // namespace art
diff --git a/runtime/class_linker.h b/runtime/class_linker.h
index 29aac31..0d3bc1e 100644
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -246,11 +246,19 @@
SHARED_REQUIRES(Locks::mutator_lock_)
REQUIRES(!dex_lock_, !Roles::uninterruptible_);
+ // Determine whether a dex cache result should be trusted, or an IncompatibleClassChangeError
+ // check should be performed even after a hit.
+ enum ResolveMode { // private.
+ kNoICCECheckForCache,
+ kForceICCECheck
+ };
+
// Resolve a method with a given ID from the DexFile, 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 determine if this method is a direct, static, or
// virtual method.
+ template <ResolveMode kResolveMode>
ArtMethod* ResolveMethod(const DexFile& dex_file,
uint32_t method_idx,
Handle<mirror::DexCache> dex_cache,
@@ -262,6 +270,7 @@
ArtMethod* GetResolvedMethod(uint32_t method_idx, ArtMethod* referrer)
SHARED_REQUIRES(Locks::mutator_lock_);
+ template <ResolveMode kResolveMode>
ArtMethod* ResolveMethod(Thread* self, uint32_t method_idx, ArtMethod* referrer, InvokeType type)
SHARED_REQUIRES(Locks::mutator_lock_)
REQUIRES(!dex_lock_, !Roles::uninterruptible_);
diff --git a/runtime/entrypoints/entrypoint_utils-inl.h b/runtime/entrypoints/entrypoint_utils-inl.h
index dccb1da..ba2fb94 100644
--- a/runtime/entrypoints/entrypoint_utils-inl.h
+++ b/runtime/entrypoints/entrypoint_utils-inl.h
@@ -68,7 +68,7 @@
class_loader.Assign(caller->GetClassLoader());
}
- return class_linker->ResolveMethod(
+ return class_linker->ResolveMethod<ClassLinker::kNoICCECheckForCache>(
*outer_method->GetDexFile(), method_index, dex_cache, class_loader, nullptr, invoke_type);
}
@@ -401,7 +401,10 @@
mirror::Object* null_this = nullptr;
HandleWrapper<mirror::Object> h_this(
hs.NewHandleWrapper(type == kStatic ? &null_this : this_object));
- resolved_method = class_linker->ResolveMethod(self, method_idx, referrer, type);
+ constexpr ClassLinker::ResolveMode resolve_mode =
+ access_check ? ClassLinker::kForceICCECheck
+ : ClassLinker::kNoICCECheckForCache;
+ resolved_method = class_linker->ResolveMethod<resolve_mode>(self, method_idx, referrer, type);
}
if (UNLIKELY(resolved_method == nullptr)) {
DCHECK(self->IsExceptionPending()); // Throw exception and unwind.
diff --git a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
index 2c8ed88..08c9b49 100644
--- a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
+++ b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
@@ -1015,7 +1015,8 @@
HandleWrapper<mirror::Object> h_receiver(
hs.NewHandleWrapper(virtual_or_interface ? &receiver : &dummy));
DCHECK_EQ(caller->GetDexFile(), called_method.dex_file);
- called = linker->ResolveMethod(self, called_method.dex_method_index, caller, invoke_type);
+ called = linker->ResolveMethod<ClassLinker::kForceICCECheck>(
+ self, called_method.dex_method_index, caller, invoke_type);
}
const void* code = nullptr;
if (LIKELY(!self->IsExceptionPending())) {
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 9ca805d..d75587b 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -195,7 +195,7 @@
}
previous_method_idx = method_idx;
InvokeType type = it->GetMethodInvokeType(*class_def);
- ArtMethod* method = linker->ResolveMethod(
+ ArtMethod* method = linker->ResolveMethod<ClassLinker::kNoICCECheckForCache>(
*dex_file, method_idx, dex_cache, class_loader, nullptr, type);
if (method == nullptr) {
DCHECK(self->IsExceptionPending());
@@ -3656,7 +3656,9 @@
const RegType& referrer = GetDeclaringClass();
auto* cl = Runtime::Current()->GetClassLinker();
auto pointer_size = cl->GetImagePointerSize();
+
ArtMethod* res_method = dex_cache_->GetResolvedMethod(dex_method_idx, pointer_size);
+ bool stash_method = false;
if (res_method == nullptr) {
const char* name = dex_file_->GetMethodName(method_id);
const Signature signature = dex_file_->GetMethodSignature(method_id);
@@ -3669,7 +3671,7 @@
res_method = klass->FindVirtualMethod(name, signature, pointer_size);
}
if (res_method != nullptr) {
- dex_cache_->SetResolvedMethod(dex_method_idx, res_method, pointer_size);
+ stash_method = true;
} else {
// If a virtual or interface method wasn't found with the expected type, look in
// the direct methods. This can happen when the wrong invoke type is used or when
@@ -3698,6 +3700,38 @@
<< PrettyMethod(res_method);
return nullptr;
}
+
+ // Check that interface methods are static or match interface classes.
+ // We only allow statics if we don't have default methods enabled.
+ //
+ // Note: this check must be after the initializer check, as those are required to fail a class,
+ // while this check implies an IncompatibleClassChangeError.
+ if (klass->IsInterface()) {
+ Runtime* runtime = Runtime::Current();
+ const bool default_methods_supported =
+ runtime == nullptr ||
+ runtime->AreExperimentalFlagsEnabled(ExperimentalFlags::kDefaultMethods);
+ if (method_type != METHOD_INTERFACE &&
+ (!default_methods_supported || method_type != METHOD_STATIC)) {
+ Fail(VERIFY_ERROR_CLASS_CHANGE)
+ << "non-interface method " << PrettyMethod(dex_method_idx, *dex_file_)
+ << " is in an interface class " << PrettyClass(klass);
+ return nullptr;
+ }
+ } else {
+ if (method_type == METHOD_INTERFACE) {
+ Fail(VERIFY_ERROR_CLASS_CHANGE)
+ << "interface method " << PrettyMethod(dex_method_idx, *dex_file_)
+ << " is in a non-interface class " << PrettyClass(klass);
+ return nullptr;
+ }
+ }
+
+ // Only stash after the above passed. Otherwise the method wasn't guaranteed to be correct.
+ if (stash_method) {
+ dex_cache_->SetResolvedMethod(dex_method_idx, res_method, pointer_size);
+ }
+
// Check if access is allowed.
if (!referrer.CanAccessMember(res_method->GetDeclaringClass(), res_method->GetAccessFlags())) {
Fail(VERIFY_ERROR_ACCESS_METHOD) << "illegal method access (call " << PrettyMethod(res_method)
@@ -3710,23 +3744,6 @@
<< PrettyMethod(res_method);
return nullptr;
}
- // Check that interface methods are static or match interface classes.
- // We only allow statics if we don't have default methods enabled.
- Runtime* runtime = Runtime::Current();
- const bool default_methods_supported =
- runtime == nullptr ||
- runtime->AreExperimentalFlagsEnabled(ExperimentalFlags::kDefaultMethods);
- if (klass->IsInterface() &&
- method_type != METHOD_INTERFACE &&
- (!default_methods_supported || method_type != METHOD_STATIC)) {
- Fail(VERIFY_ERROR_CLASS_CHANGE) << "non-interface method " << PrettyMethod(res_method)
- << " is in an interface class " << PrettyClass(klass);
- return nullptr;
- } else if (!klass->IsInterface() && method_type == METHOD_INTERFACE) {
- Fail(VERIFY_ERROR_CLASS_CHANGE) << "interface method " << PrettyMethod(res_method)
- << " is in a non-interface class " << PrettyClass(klass);
- return nullptr;
- }
// See if the method type implied by the invoke instruction matches the access flags for the
// target method.
if ((method_type == METHOD_DIRECT && (!res_method->IsDirect() || res_method->IsStatic())) ||