Revert "ART: Check invoke-interface earlier in verifier"
This reverts commit dae24142127c64551142a50423085aabdb0a6060.
It is important to check the name of the method being called.
Bug: 21869691
diff --git a/compiler/driver/compiler_driver-inl.h b/compiler/driver/compiler_driver-inl.h
index 0eb3e43..10841e6 100644
--- a/compiler/driver/compiler_driver-inl.h
+++ b/compiler/driver/compiler_driver-inl.h
@@ -264,16 +264,18 @@
Handle<mirror::ClassLoader> class_loader, const DexCompilationUnit* mUnit,
uint32_t method_idx, InvokeType invoke_type, bool check_incompatible_class_change) {
DCHECK_EQ(class_loader.Get(), soa.Decode<mirror::ClassLoader*>(mUnit->GetClassLoader()));
- ArtMethod* resolved_method =
- check_incompatible_class_change
- ? mUnit->GetClassLinker()->ResolveMethod<ClassLinker::kForceICCECheck>(
- *dex_cache->GetDexFile(), method_idx, dex_cache, class_loader, nullptr, invoke_type)
- : mUnit->GetClassLinker()->ResolveMethod<ClassLinker::kNoICCECheckForCache>(
- *dex_cache->GetDexFile(), method_idx, dex_cache, class_loader, nullptr, invoke_type);
+ ArtMethod* resolved_method = mUnit->GetClassLinker()->ResolveMethod(
+ *dex_cache->GetDexFile(), method_idx, dex_cache, class_loader, nullptr, invoke_type);
+ DCHECK_EQ(resolved_method == nullptr, soa.Self()->IsExceptionPending());
if (UNLIKELY(resolved_method == nullptr)) {
- DCHECK(soa.Self()->IsExceptionPending());
// Clean up any exception left by type resolution.
soa.Self()->ClearException();
+ return nullptr;
+ }
+ if (check_incompatible_class_change &&
+ UNLIKELY(resolved_method->CheckIncompatibleClassChange(invoke_type))) {
+ // Silently return null on incompatible class change.
+ return nullptr;
}
return resolved_method;
}
@@ -359,7 +361,7 @@
ArtMethod* called_method;
ClassLinker* class_linker = mUnit->GetClassLinker();
if (LIKELY(devirt_target->dex_file == mUnit->GetDexFile())) {
- called_method = class_linker->ResolveMethod<ClassLinker::kNoICCECheckForCache>(
+ called_method = class_linker->ResolveMethod(
*devirt_target->dex_file, devirt_target->dex_method_index, dex_cache, class_loader,
nullptr, kVirtual);
} else {
@@ -367,7 +369,7 @@
auto target_dex_cache(hs.NewHandle(class_linker->RegisterDexFile(
*devirt_target->dex_file,
class_linker->GetOrCreateAllocatorForClassLoader(class_loader.Get()))));
- called_method = class_linker->ResolveMethod<ClassLinker::kNoICCECheckForCache>(
+ called_method = class_linker->ResolveMethod(
*devirt_target->dex_file, devirt_target->dex_method_index, target_dex_cache,
class_loader, nullptr, kVirtual);
}
diff --git a/compiler/driver/compiler_driver.cc b/compiler/driver/compiler_driver.cc
index 82af541..0cad643 100644
--- a/compiler/driver/compiler_driver.cc
+++ b/compiler/driver/compiler_driver.cc
@@ -1902,7 +1902,7 @@
}
if (resolve_fields_and_methods) {
while (it.HasNextDirectMethod()) {
- ArtMethod* method = class_linker->ResolveMethod<ClassLinker::kNoICCECheckForCache>(
+ ArtMethod* method = class_linker->ResolveMethod(
dex_file, it.GetMemberIndex(), dex_cache, class_loader, nullptr,
it.GetMethodInvokeType(class_def));
if (method == nullptr) {
@@ -1911,7 +1911,7 @@
it.Next();
}
while (it.HasNextVirtualMethod()) {
- ArtMethod* method = class_linker->ResolveMethod<ClassLinker::kNoICCECheckForCache>(
+ ArtMethod* method = class_linker->ResolveMethod(
dex_file, it.GetMemberIndex(), dex_cache, class_loader, nullptr,
it.GetMethodInvokeType(class_def));
if (method == nullptr) {
diff --git a/compiler/oat_writer.cc b/compiler/oat_writer.cc
index 2b2f0e8..59d59fc 100644
--- a/compiler/oat_writer.cc
+++ b/compiler/oat_writer.cc
@@ -733,12 +733,8 @@
StackHandleScope<1> hs(soa.Self());
Handle<mirror::DexCache> dex_cache(hs.NewHandle(linker->FindDexCache(
Thread::Current(), *dex_file_)));
- ArtMethod* method = linker->ResolveMethod<ClassLinker::kNoICCECheckForCache>(
- *dex_file_,
- it.GetMemberIndex(),
- dex_cache,
- NullHandle<mirror::ClassLoader>(),
- nullptr,
+ ArtMethod* method = linker->ResolveMethod(
+ *dex_file_, it.GetMemberIndex(), dex_cache, NullHandle<mirror::ClassLoader>(), nullptr,
invoke_type);
if (method == nullptr) {
LOG(INTERNAL_FATAL) << "Unexpected failure to resolve a method: "
diff --git a/compiler/optimizing/builder.cc b/compiler/optimizing/builder.cc
index e1404ce..b156d13 100644
--- a/compiler/optimizing/builder.cc
+++ b/compiler/optimizing/builder.cc
@@ -744,7 +744,7 @@
soa.Decode<mirror::ClassLoader*>(dex_compilation_unit_->GetClassLoader())));
Handle<mirror::Class> compiling_class(hs.NewHandle(GetCompilingClass()));
- ArtMethod* resolved_method = class_linker->ResolveMethod<ClassLinker::kForceICCECheck>(
+ ArtMethod* resolved_method = class_linker->ResolveMethod(
*dex_compilation_unit_->GetDexFile(),
method_idx,
dex_compilation_unit_->GetDexCache(),
diff --git a/compiler/optimizing/reference_type_propagation.cc b/compiler/optimizing/reference_type_propagation.cc
index fea903d..dd34924 100644
--- a/compiler/optimizing/reference_type_propagation.cc
+++ b/compiler/optimizing/reference_type_propagation.cc
@@ -469,7 +469,7 @@
// but then we would need to pass it to RTPVisitor just for this debug check. Since
// the method is from the String class, the null loader is good enough.
Handle<mirror::ClassLoader> loader;
- ArtMethod* method = cl->ResolveMethod<ClassLinker::kNoICCECheckForCache>(
+ ArtMethod* method = cl->ResolveMethod(
invoke->GetDexFile(), invoke->GetDexMethodIndex(), dex_cache, loader, nullptr, kDirect);
DCHECK(method != nullptr);
mirror::Class* declaring_class = method->GetDeclaringClass();
diff --git a/runtime/class_linker-inl.h b/runtime/class_linker-inl.h
index a5d10b2..88a3996 100644
--- a/runtime/class_linker-inl.h
+++ b/runtime/class_linker-inl.h
@@ -116,7 +116,6 @@
return resolved_method;
}
-template <ClassLinker::ResolveMode kResolveMode>
inline ArtMethod* ClassLinker::ResolveMethod(Thread* self,
uint32_t method_idx,
ArtMethod* referrer,
@@ -128,12 +127,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<kResolveMode>(*dex_file,
- method_idx,
- h_dex_cache,
- h_class_loader,
- referrer,
- type);
+ resolved_method = ResolveMethod(*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 f5085ed..0d10b4e 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -6162,7 +6162,6 @@
return resolved;
}
-template <ClassLinker::ResolveMode kResolveMode>
ArtMethod* ClassLinker::ResolveMethod(const DexFile& dex_file,
uint32_t method_idx,
Handle<mirror::DexCache> dex_cache,
@@ -6174,12 +6173,6 @@
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.
@@ -6198,36 +6191,8 @@
DCHECK(resolved == nullptr || resolved->GetDeclaringClassUnchecked() != nullptr);
break;
case kInterface:
- // 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());
- }
+ resolved = klass->FindInterfaceMethod(dex_cache.Get(), method_idx, image_pointer_size_);
+ DCHECK(resolved == nullptr || resolved->GetDeclaringClass()->IsInterface());
break;
case kSuper: // Fall-through.
case kVirtual:
@@ -6829,20 +6794,4 @@
}
}
-// 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 0d3bc1e..29aac31 100644
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -246,19 +246,11 @@
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,
@@ -270,7 +262,6 @@
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 ba2fb94..dccb1da 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<ClassLinker::kNoICCECheckForCache>(
+ return class_linker->ResolveMethod(
*outer_method->GetDexFile(), method_index, dex_cache, class_loader, nullptr, invoke_type);
}
@@ -401,10 +401,7 @@
mirror::Object* null_this = nullptr;
HandleWrapper<mirror::Object> h_this(
hs.NewHandleWrapper(type == kStatic ? &null_this : this_object));
- constexpr ClassLinker::ResolveMode resolve_mode =
- access_check ? ClassLinker::kForceICCECheck
- : ClassLinker::kNoICCECheckForCache;
- resolved_method = class_linker->ResolveMethod<resolve_mode>(self, method_idx, referrer, type);
+ resolved_method = class_linker->ResolveMethod(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 08c9b49..2c8ed88 100644
--- a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
+++ b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc
@@ -1015,8 +1015,7 @@
HandleWrapper<mirror::Object> h_receiver(
hs.NewHandleWrapper(virtual_or_interface ? &receiver : &dummy));
DCHECK_EQ(caller->GetDexFile(), called_method.dex_file);
- called = linker->ResolveMethod<ClassLinker::kForceICCECheck>(
- self, called_method.dex_method_index, caller, invoke_type);
+ called = linker->ResolveMethod(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 3b0d7c4..9ca805d 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<ClassLinker::kNoICCECheckForCache>(
+ ArtMethod* method = linker->ResolveMethod(
*dex_file, method_idx, dex_cache, class_loader, nullptr, type);
if (method == nullptr) {
DCHECK(self->IsExceptionPending());
@@ -3656,30 +3656,6 @@
const RegType& referrer = GetDeclaringClass();
auto* cl = Runtime::Current()->GetClassLinker();
auto pointer_size = cl->GetImagePointerSize();
-
- // Check that interface methods are static or match interface classes.
- // We only allow statics if we don't have default methods enabled.
- 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;
- }
- }
-
ArtMethod* res_method = dex_cache_->GetResolvedMethod(dex_method_idx, pointer_size);
if (res_method == nullptr) {
const char* name = dex_file_->GetMethodName(method_id);
@@ -3734,6 +3710,23 @@
<< 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())) ||
diff --git a/test/800-smali/expected.txt b/test/800-smali/expected.txt
index ebefeea..a590cf1 100644
--- a/test/800-smali/expected.txt
+++ b/test/800-smali/expected.txt
@@ -47,5 +47,4 @@
b/23502994 (if-eqz)
b/23502994 (check-cast)
b/25494456
-b/21869691
Done!
diff --git a/test/800-smali/smali/b_21869691A.smali b/test/800-smali/smali/b_21869691A.smali
deleted file mode 100644
index a7a6ef4..0000000
--- a/test/800-smali/smali/b_21869691A.smali
+++ /dev/null
@@ -1,47 +0,0 @@
-# Test that the verifier does not stash methods incorrectly because they are being invoked with
-# the wrong opcode.
-#
-# When using invoke-interface on a method id that is not from an interface class, we should throw
-# an IncompatibleClassChangeError. FindInterfaceMethod assumes that the given type is an interface,
-# so we can construct a class hierarchy that would have a surprising result:
-#
-# interface I {
-# void a();
-# }
-#
-# class B implements I {
-# // miranda method for a, or a implemented.
-# }
-#
-# class C extends B {
-# }
-#
-# Then calling invoke-interface C.a() will go wrong if there is no explicit check: a can't be found
-# in C, but in the interface table, so we will find an interface method and pass ICCE checks.
-#
-# If we do this before a correct invoke-virtual C.a(), we poison the dex cache with an incorrect
-# method. In this test, this is done in A (A < B, so processed first). The "real" call is in B.
-
-.class public LB21869691A;
-
-.super Ljava/lang/Object;
-
-.method public constructor <init>()V
- .registers 1
- invoke-direct {p0}, Ljava/lang/Object;-><init>()V
- return-void
-.end method
-
-.method public run()V
- .registers 3
- new-instance v0, LB21869691C;
- invoke-direct {v0}, LB21869691C;-><init>()V
- invoke-virtual {v2, v0}, LB21869691A;->callinf(LB21869691C;)V
- return-void
-.end method
-
-.method public callinf(LB21869691C;)V
- .registers 2
- invoke-interface {p1}, LB21869691C;->a()V
- return-void
-.end method
diff --git a/test/800-smali/smali/b_21869691B.smali b/test/800-smali/smali/b_21869691B.smali
deleted file mode 100644
index 1172bdb..0000000
--- a/test/800-smali/smali/b_21869691B.smali
+++ /dev/null
@@ -1,33 +0,0 @@
-# Test that the verifier does not stash methods incorrectly because they are being invoked with
-# the wrong opcode. See b_21869691A.smali for explanation.
-
-.class public abstract LB21869691B;
-
-.super Ljava/lang/Object;
-.implements LB21869691I;
-
-.method protected constructor <init>()V
- .registers 1
- invoke-direct {p0}, Ljava/lang/Object;-><init>()V
- return-void
-.end method
-
-# Have an implementation for the interface method.
-.method public a()V
- .registers 1
- return-void
-.end method
-
-# Call ourself with invoke-virtual.
-.method public callB()V
- .registers 1
- invoke-virtual {p0}, LB21869691B;->a()V
- return-void
-.end method
-
-# Call C with invoke-virtual.
-.method public callB(LB21869691C;)V
- .registers 2
- invoke-virtual {p1}, LB21869691C;->a()V
- return-void
-.end method
diff --git a/test/800-smali/smali/b_21869691C.smali b/test/800-smali/smali/b_21869691C.smali
deleted file mode 100644
index 4f89a04..0000000
--- a/test/800-smali/smali/b_21869691C.smali
+++ /dev/null
@@ -1,12 +0,0 @@
-# Test that the verifier does not stash methods incorrectly because they are being invoked with
-# the wrong opcode. See b_21869691A.smali for explanation.
-
-.class public LB21869691C;
-
-.super LB21869691B;
-
-.method public constructor <init>()V
- .registers 1
- invoke-direct {p0}, LB21869691B;-><init>()V
- return-void
-.end method
diff --git a/test/800-smali/smali/b_21869691I.smali b/test/800-smali/smali/b_21869691I.smali
deleted file mode 100644
index 72a27dd..0000000
--- a/test/800-smali/smali/b_21869691I.smali
+++ /dev/null
@@ -1,11 +0,0 @@
-# Test that the verifier does not stash methods incorrectly because they are being invoked with
-# the wrong opcode.
-#
-# This is the interface class that has an "a" method.
-
-.class public abstract interface LB21869691I;
-
-.super Ljava/lang/Object;
-
-.method public abstract a()V
-.end method
diff --git a/test/800-smali/src/Main.java b/test/800-smali/src/Main.java
index 3b62a46..4844848 100644
--- a/test/800-smali/src/Main.java
+++ b/test/800-smali/src/Main.java
@@ -139,8 +139,6 @@
new Object[] { "abc" }, null, null));
testCases.add(new TestCase("b/25494456", "B25494456", "run", null, new VerifyError(),
null));
- testCases.add(new TestCase("b/21869691", "B21869691A", "run", null,
- new IncompatibleClassChangeError(), null));
}
public void runTests() {
@@ -210,7 +208,7 @@
tc.expectedException.getClass().getName() +
", but got " + exc.getClass(), exc);
} else {
- // Expected exception, do nothing.
+ // Expected exception, do nothing.
}
} finally {
if (errorReturn != null) {