From: Andreas Gampe Date: Fri, 4 Dec 2015 01:27:32 +0000 (-0800) Subject: ART: Check invoke-interface earlier in verifier X-Git-Tag: android-x86-7.1-r1~952^2 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=dae24142127c64551142a50423085aabdb0a6060;p=android-x86%2Fart.git ART: Check invoke-interface earlier in verifier Invoke-interface should only be called on an interface method. Move the check earlier, as otherwise we'll try to resolve and potentially inject a method into the dex cache. 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: Ica27158f675b5aa223d9229248189612f4706832 --- diff --git a/compiler/driver/compiler_driver-inl.h b/compiler/driver/compiler_driver-inl.h index 10841e670..0eb3e439a 100644 --- a/compiler/driver/compiler_driver-inl.h +++ b/compiler/driver/compiler_driver-inl.h @@ -264,18 +264,16 @@ inline ArtMethod* CompilerDriver::ResolveMethod( Handle class_loader, const DexCompilationUnit* mUnit, uint32_t method_idx, InvokeType invoke_type, bool check_incompatible_class_change) { DCHECK_EQ(class_loader.Get(), soa.Decode(mUnit->GetClassLoader())); - 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()); + ArtMethod* resolved_method = + check_incompatible_class_change + ? mUnit->GetClassLinker()->ResolveMethod( + *dex_cache->GetDexFile(), method_idx, dex_cache, class_loader, nullptr, invoke_type) + : mUnit->GetClassLinker()->ResolveMethod( + *dex_cache->GetDexFile(), method_idx, dex_cache, class_loader, nullptr, invoke_type); 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; } @@ -361,7 +359,7 @@ inline int CompilerDriver::IsFastInvoke( ArtMethod* called_method; ClassLinker* class_linker = mUnit->GetClassLinker(); if (LIKELY(devirt_target->dex_file == mUnit->GetDexFile())) { - called_method = class_linker->ResolveMethod( + called_method = class_linker->ResolveMethod( *devirt_target->dex_file, devirt_target->dex_method_index, dex_cache, class_loader, nullptr, kVirtual); } else { @@ -369,7 +367,7 @@ inline int CompilerDriver::IsFastInvoke( auto target_dex_cache(hs.NewHandle(class_linker->RegisterDexFile( *devirt_target->dex_file, class_linker->GetOrCreateAllocatorForClassLoader(class_loader.Get())))); - called_method = class_linker->ResolveMethod( + 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 9d3af1681..a05105b84 100644 --- a/compiler/driver/compiler_driver.cc +++ b/compiler/driver/compiler_driver.cc @@ -1902,7 +1902,7 @@ class ResolveClassFieldsAndMethodsVisitor : public CompilationVisitor { } if (resolve_fields_and_methods) { while (it.HasNextDirectMethod()) { - ArtMethod* method = class_linker->ResolveMethod( + 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 @@ class ResolveClassFieldsAndMethodsVisitor : public CompilationVisitor { it.Next(); } while (it.HasNextVirtualMethod()) { - ArtMethod* method = class_linker->ResolveMethod( + 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 a6a49f901..901580079 100644 --- a/compiler/oat_writer.cc +++ b/compiler/oat_writer.cc @@ -641,8 +641,12 @@ class OatWriter::InitImageMethodVisitor : public OatDexMethodVisitor { StackHandleScope<1> hs(soa.Self()); Handle dex_cache(hs.NewHandle(linker->FindDexCache( Thread::Current(), *dex_file_))); - ArtMethod* method = linker->ResolveMethod( - *dex_file_, it.GetMemberIndex(), dex_cache, NullHandle(), nullptr, + ArtMethod* method = linker->ResolveMethod( + *dex_file_, + it.GetMemberIndex(), + dex_cache, + NullHandle(), + 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 8e75bdcdc..2bbf500a9 100644 --- a/compiler/optimizing/builder.cc +++ b/compiler/optimizing/builder.cc @@ -744,7 +744,7 @@ ArtMethod* HGraphBuilder::ResolveMethod(uint16_t method_idx, InvokeType invoke_t soa.Decode(dex_compilation_unit_->GetClassLoader()))); Handle compiling_class(hs.NewHandle(GetCompilingClass())); - ArtMethod* resolved_method = class_linker->ResolveMethod( + 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 dd349240f..fea903d9c 100644 --- a/compiler/optimizing/reference_type_propagation.cc +++ b/compiler/optimizing/reference_type_propagation.cc @@ -469,7 +469,7 @@ void RTPVisitor::SetClassAsTypeInfo(HInstruction* instr, // 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 loader; - ArtMethod* method = cl->ResolveMethod( + 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 88a399657..a5d10b265 100644 --- a/runtime/class_linker-inl.h +++ b/runtime/class_linker-inl.h @@ -116,6 +116,7 @@ inline ArtMethod* ClassLinker::GetResolvedMethod(uint32_t method_idx, ArtMethod* return resolved_method; } +template inline ArtMethod* ClassLinker::ResolveMethod(Thread* self, uint32_t method_idx, ArtMethod* referrer, @@ -127,12 +128,12 @@ inline ArtMethod* ClassLinker::ResolveMethod(Thread* self, Handle h_dex_cache(hs.NewHandle(declaring_class->GetDexCache())); Handle 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(*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 2dd2a8388..3f31fcd15 100644 --- a/runtime/class_linker.cc +++ b/runtime/class_linker.cc @@ -6149,6 +6149,7 @@ mirror::Class* ClassLinker::ResolveType(const DexFile& dex_file, return resolved; } +template ArtMethod* ClassLinker::ResolveMethod(const DexFile& dex_file, uint32_t method_idx, Handle dex_cache, @@ -6160,6 +6161,12 @@ ArtMethod* ClassLinker::ResolveMethod(const DexFile& dex_file, 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. @@ -6178,8 +6185,36 @@ ArtMethod* ClassLinker::ResolveMethod(const DexFile& dex_file, 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: @@ -6781,4 +6816,20 @@ void ClassLinker::CleanupClassLoaders() { } } +// Instantiate ResolveMethod. +template ArtMethod* ClassLinker::ResolveMethod( + const DexFile& dex_file, + uint32_t method_idx, + Handle dex_cache, + Handle class_loader, + ArtMethod* referrer, + InvokeType type); +template ArtMethod* ClassLinker::ResolveMethod( + const DexFile& dex_file, + uint32_t method_idx, + Handle dex_cache, + Handle class_loader, + ArtMethod* referrer, + InvokeType type); + } // namespace art diff --git a/runtime/class_linker.h b/runtime/class_linker.h index 29aac312c..0d3bc1e2c 100644 --- a/runtime/class_linker.h +++ b/runtime/class_linker.h @@ -246,11 +246,19 @@ class ClassLinker { 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 ArtMethod* ResolveMethod(const DexFile& dex_file, uint32_t method_idx, Handle dex_cache, @@ -262,6 +270,7 @@ class ClassLinker { ArtMethod* GetResolvedMethod(uint32_t method_idx, ArtMethod* referrer) SHARED_REQUIRES(Locks::mutator_lock_); + template 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 dccb1dad3..ba2fb9493 100644 --- a/runtime/entrypoints/entrypoint_utils-inl.h +++ b/runtime/entrypoints/entrypoint_utils-inl.h @@ -68,7 +68,7 @@ inline ArtMethod* GetResolvedMethod(ArtMethod* outer_method, class_loader.Assign(caller->GetClassLoader()); } - return class_linker->ResolveMethod( + return class_linker->ResolveMethod( *outer_method->GetDexFile(), method_index, dex_cache, class_loader, nullptr, invoke_type); } @@ -401,7 +401,10 @@ inline ArtMethod* FindMethodFromCode(uint32_t method_idx, mirror::Object** this_ mirror::Object* null_this = nullptr; HandleWrapper 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(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 c41ee45e6..6361144db 100644 --- a/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc +++ b/runtime/entrypoints/quick/quick_trampoline_entrypoints.cc @@ -1012,7 +1012,8 @@ extern "C" const void* artQuickResolutionTrampoline( HandleWrapper 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( + 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 364b8cefb..cf27ff2f6 100644 --- a/runtime/verifier/method_verifier.cc +++ b/runtime/verifier/method_verifier.cc @@ -192,7 +192,7 @@ void MethodVerifier::VerifyMethods(Thread* self, } previous_method_idx = method_idx; InvokeType type = it->GetMethodInvokeType(*class_def); - ArtMethod* method = linker->ResolveMethod( + ArtMethod* method = linker->ResolveMethod( *dex_file, method_idx, dex_cache, class_loader, nullptr, type); if (method == nullptr) { DCHECK(self->IsExceptionPending()); @@ -3638,6 +3638,30 @@ ArtMethod* MethodVerifier::ResolveMethodAndCheckAccess( 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); @@ -3692,23 +3716,6 @@ ArtMethod* MethodVerifier::ResolveMethodAndCheckAccess( << 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 a590cf1e0..ebefeea40 100644 --- a/test/800-smali/expected.txt +++ b/test/800-smali/expected.txt @@ -47,4 +47,5 @@ b/23300986 (2) 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 new file mode 100644 index 000000000..a7a6ef4bc --- /dev/null +++ b/test/800-smali/smali/b_21869691A.smali @@ -0,0 +1,47 @@ +# 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 ()V + .registers 1 + invoke-direct {p0}, Ljava/lang/Object;->()V + return-void +.end method + +.method public run()V + .registers 3 + new-instance v0, LB21869691C; + invoke-direct {v0}, LB21869691C;->()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 new file mode 100644 index 000000000..1172bdba5 --- /dev/null +++ b/test/800-smali/smali/b_21869691B.smali @@ -0,0 +1,33 @@ +# 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 ()V + .registers 1 + invoke-direct {p0}, Ljava/lang/Object;->()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 new file mode 100644 index 000000000..4f89a046c --- /dev/null +++ b/test/800-smali/smali/b_21869691C.smali @@ -0,0 +1,12 @@ +# 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 ()V + .registers 1 + invoke-direct {p0}, LB21869691B;->()V + return-void +.end method diff --git a/test/800-smali/smali/b_21869691I.smali b/test/800-smali/smali/b_21869691I.smali new file mode 100644 index 000000000..72a27ddd2 --- /dev/null +++ b/test/800-smali/smali/b_21869691I.smali @@ -0,0 +1,11 @@ +# 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 484484833..3b62a46fd 100644 --- a/test/800-smali/src/Main.java +++ b/test/800-smali/src/Main.java @@ -139,6 +139,8 @@ public class Main { 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() { @@ -208,7 +210,7 @@ public class Main { tc.expectedException.getClass().getName() + ", but got " + exc.getClass(), exc); } else { - // Expected exception, do nothing. + // Expected exception, do nothing. } } finally { if (errorReturn != null) {