From: Igor Murashkin Date: Wed, 3 Feb 2016 00:56:50 +0000 (-0800) Subject: runtime: Don't skip verification for -Xverify:soft-fail X-Git-Tag: android-x86-7.1-r1~424^2~19^2^2~1 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=df707e406877e9c0426dd051c00933ebb331673e;p=android-x86%2Fart.git runtime: Don't skip verification for -Xverify:soft-fail When forcing the interpreter into access checks mode, make sure that the regular verification is still run, giving the verifier an opportunity to throw a VerifyError. If verification would've succeeded (without -Xverify:soft-fail flag), override this and soft-fail, to force the interpreter-with-access-checks to be run instead of the normal faster interpreter. This fixes the following run-tests under the interpeter-access-checks: * 135 * 412 * 471 * 506 * 800 Bug: 22414682 Change-Id: I5cb86a8bba71c7af9361a63c0802786c852b857b --- diff --git a/compiler/driver/compiler_driver.cc b/compiler/driver/compiler_driver.cc index f1b745895..9ab728079 100644 --- a/compiler/driver/compiler_driver.cc +++ b/compiler/driver/compiler_driver.cc @@ -2306,9 +2306,9 @@ class SetVerifiedClassVisitor : public CompilationVisitor { mirror::Class::SetStatus(klass, mirror::Class::kStatusVerified, soa.Self()); // Mark methods as pre-verified. If we don't do this, the interpreter will run with // access checks. - klass->SetPreverifiedFlagOnAllMethods( + klass->SetSkipAccessChecksFlagOnAllMethods( GetInstructionSetPointerSize(manager_->GetCompiler()->GetInstructionSet())); - klass->SetPreverified(); + klass->SetVerificationAttempted(); } // Record the final class status if necessary. ClassReference ref(manager_->GetDexFile(), class_def_index); diff --git a/runtime/art_method.h b/runtime/art_method.h index 440e796f4..a020e9d17 100644 --- a/runtime/art_method.h +++ b/runtime/art_method.h @@ -173,13 +173,13 @@ class ArtMethod FINAL { bool IsProxyMethod() SHARED_REQUIRES(Locks::mutator_lock_); - bool IsPreverified() { - return (GetAccessFlags() & kAccPreverified) != 0; + bool SkipAccessChecks() { + return (GetAccessFlags() & kAccSkipAccessChecks) != 0; } - void SetPreverified() { - DCHECK(!IsPreverified()); - SetAccessFlags(GetAccessFlags() | kAccPreverified); + void SetSkipAccessChecks() { + DCHECK(!SkipAccessChecks()); + SetAccessFlags(GetAccessFlags() | kAccSkipAccessChecks); } // Returns true if this method could be overridden by a default method. diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc index 35e1ee6cb..0667e2389 100644 --- a/runtime/class_linker.cc +++ b/runtime/class_linker.cc @@ -3546,7 +3546,7 @@ void ClassLinker::VerifyClass(Thread* self, Handle klass) { // Don't attempt to re-verify if already sufficiently verified. if (klass->IsVerified()) { - EnsurePreverifiedMethods(klass); + EnsureSkipAccessChecksMethods(klass); return; } if (klass->IsCompileTimeVerified() && Runtime::Current()->IsAotCompiler()) { @@ -3569,22 +3569,10 @@ void ClassLinker::VerifyClass(Thread* self, Handle klass) { mirror::Class::SetStatus(klass, mirror::Class::kStatusVerifyingAtRuntime, self); } - // Skip verification if we are forcing a soft fail. - // This has to be before the normal verification enabled check, - // since technically verification is disabled in this mode. - if (UNLIKELY(Runtime::Current()->IsVerificationSoftFail())) { - // Force verification to be a 'soft failure'. - mirror::Class::SetStatus(klass, mirror::Class::kStatusVerified, self); - // As this is a fake verified status, make sure the methods are _not_ marked preverified - // later. - klass->SetPreverified(); - return; - } - // Skip verification if disabled. if (!Runtime::Current()->IsVerificationEnabled()) { mirror::Class::SetStatus(klass, mirror::Class::kStatusVerified, self); - EnsurePreverifiedMethods(klass); + EnsureSkipAccessChecksMethods(klass); return; } @@ -3687,9 +3675,9 @@ void ClassLinker::VerifyClass(Thread* self, Handle klass) { mirror::Class::SetStatus(klass, mirror::Class::kStatusRetryVerificationAtRuntime, self); } else { mirror::Class::SetStatus(klass, mirror::Class::kStatusVerified, self); - // As this is a fake verified status, make sure the methods are _not_ marked preverified - // later. - klass->SetPreverified(); + // As this is a fake verified status, make sure the methods are _not_ marked + // kAccSkipAccessChecks later. + klass->SetVerificationAttempted(); } } } else { @@ -3702,19 +3690,26 @@ void ClassLinker::VerifyClass(Thread* self, Handle klass) { } if (preverified || verifier_failure == verifier::MethodVerifier::kNoFailure) { // Class is verified so we don't need to do any access check on its methods. - // Let the interpreter know it by setting the kAccPreverified flag onto each + // Let the interpreter know it by setting the kAccSkipAccessChecks flag onto each // method. // Note: we're going here during compilation and at runtime. When we set the - // kAccPreverified flag when compiling image classes, the flag is recorded + // kAccSkipAccessChecks flag when compiling image classes, the flag is recorded // in the image and is set when loading the image. - EnsurePreverifiedMethods(klass); + + if (UNLIKELY(Runtime::Current()->IsVerificationSoftFail())) { + // Never skip access checks if the verification soft fail is forced. + // Mark the class as having a verification attempt to avoid re-running the verifier. + klass->SetVerificationAttempted(); + } else { + EnsureSkipAccessChecksMethods(klass); + } } } -void ClassLinker::EnsurePreverifiedMethods(Handle klass) { - if (!klass->IsPreverified()) { - klass->SetPreverifiedFlagOnAllMethods(image_pointer_size_); - klass->SetPreverified(); +void ClassLinker::EnsureSkipAccessChecksMethods(Handle klass) { + if (!klass->WasVerificationAttempted()) { + klass->SetSkipAccessChecksFlagOnAllMethods(image_pointer_size_); + klass->SetVerificationAttempted(); } } @@ -3745,7 +3740,7 @@ bool ClassLinker::VerifyClassUsingOatFile(const DexFile& dex_file, } // We may be running with a preopted oat file but without image. In this case, - // we don't skip verification of preverified classes to ensure we initialize + // we don't skip verification of skip_access_checks classes to ensure we initialize // dex caches with all types resolved during verification. // We need to trust image classes, as these might be coming out of a pre-opted, quickened boot // image (that we just failed loading), and the verifier can't be run on quickened opcodes when @@ -3853,8 +3848,9 @@ mirror::Class* ClassLinker::CreateProxyClass(ScopedObjectAccessAlreadyRunnable& } DCHECK(klass->GetClass() != nullptr); klass->SetObjectSize(sizeof(mirror::Proxy)); - // Set the class access flags incl. preverified, so we do not try to set the flag on the methods. - klass->SetAccessFlags(kAccClassIsProxy | kAccPublic | kAccFinal | kAccPreverified); + // Set the class access flags incl. VerificationAttempted, so we do not try to set the flag on + // the methods. + klass->SetAccessFlags(kAccClassIsProxy | kAccPublic | kAccFinal | kAccVerificationAttempted); klass->SetClassLoader(soa.Decode(loader)); DCHECK_EQ(klass->GetPrimitiveType(), Primitive::kPrimNot); klass->SetName(soa.Decode(name)); @@ -4663,7 +4659,7 @@ bool ClassLinker::EnsureInitialized(Thread* self, Handle c, bool bool can_init_parents) { DCHECK(c.Get() != nullptr); if (c->IsInitialized()) { - EnsurePreverifiedMethods(c); + EnsureSkipAccessChecksMethods(c); return true; } const bool success = InitializeClass(self, c, can_init_fields, can_init_parents); @@ -6358,11 +6354,11 @@ bool ClassLinker::LinkInterfaceMethods( for (ArtMethod* def_method : default_methods) { ArtMethod& new_method = *out; new_method.CopyFrom(def_method, image_pointer_size_); - // Clear the preverified flag if it is present. Since this class hasn't been verified yet it - // shouldn't have methods that are preverified. + // Clear the kAccSkipAccessChecks flag if it is present. Since this class hasn't been verified + // yet it shouldn't have methods that are skipping access checks. // TODO This is rather arbitrary. We should maybe support classes where only some of its - // methods are preverified. - new_method.SetAccessFlags((new_method.GetAccessFlags() | kAccDefault) & ~kAccPreverified); + // methods are skip_access_checks. + new_method.SetAccessFlags((new_method.GetAccessFlags() | kAccDefault) & ~kAccSkipAccessChecks); move_table.emplace(def_method, &new_method); ++out; } @@ -6370,11 +6366,11 @@ bool ClassLinker::LinkInterfaceMethods( ArtMethod& new_method = *out; new_method.CopyFrom(conf_method, image_pointer_size_); // This is a type of default method (there are default method impls, just a conflict) so mark - // this as a default, non-abstract method, since thats what it is. Also clear the preverified - // bit since this class hasn't been verified yet it shouldn't have methods that are - // preverified. + // this as a default, non-abstract method, since thats what it is. Also clear the + // kAccSkipAccessChecks bit since this class hasn't been verified yet it shouldn't have + // methods that are skipping access checks. constexpr uint32_t kSetFlags = kAccDefault | kAccDefaultConflict; - constexpr uint32_t kMaskFlags = ~(kAccAbstract | kAccPreverified); + constexpr uint32_t kMaskFlags = ~(kAccAbstract | kAccSkipAccessChecks); new_method.SetAccessFlags((new_method.GetAccessFlags() | kSetFlags) & kMaskFlags); DCHECK(new_method.IsDefaultConflicting()); // The actual method might or might not be marked abstract since we just copied it from a diff --git a/runtime/class_linker.h b/runtime/class_linker.h index 5176cbd3e..3b4e9121d 100644 --- a/runtime/class_linker.h +++ b/runtime/class_linker.h @@ -957,9 +957,10 @@ class ClassLinker { void CreateProxyMethod(Handle klass, ArtMethod* prototype, ArtMethod* out) SHARED_REQUIRES(Locks::mutator_lock_); - // Ensures that methods have the kAccPreverified bit set. We use the kAccPreverfied bit on the - // class access flags to determine whether this has been done before. - void EnsurePreverifiedMethods(Handle c) + // Ensures that methods have the kAccSkipAccessChecks bit set. We use the + // kAccVerificationAttempted bit on the class access flags to determine whether this has been done + // before. + void EnsureSkipAccessChecksMethods(Handle c) SHARED_REQUIRES(Locks::mutator_lock_); mirror::Class* LookupClassFromBootImage(const char* descriptor) diff --git a/runtime/class_linker_test.cc b/runtime/class_linker_test.cc index 40dfda931..b86da9fbb 100644 --- a/runtime/class_linker_test.cc +++ b/runtime/class_linker_test.cc @@ -1126,14 +1126,14 @@ TEST_F(ClassLinkerTest, ValidatePredefinedClassSizes) { static void CheckMethod(ArtMethod* method, bool verified) SHARED_REQUIRES(Locks::mutator_lock_) { if (!method->IsNative() && !method->IsAbstract()) { - EXPECT_EQ((method->GetAccessFlags() & kAccPreverified) != 0U, verified) + EXPECT_EQ((method->GetAccessFlags() & kAccSkipAccessChecks) != 0U, verified) << PrettyMethod(method, true); } } -static void CheckPreverified(mirror::Class* c, bool preverified) +static void CheckVerificationAttempted(mirror::Class* c, bool preverified) SHARED_REQUIRES(Locks::mutator_lock_) { - EXPECT_EQ((c->GetAccessFlags() & kAccPreverified) != 0U, preverified) + EXPECT_EQ((c->GetAccessFlags() & kAccVerificationAttempted) != 0U, preverified) << "Class " << PrettyClass(c) << " not as expected"; for (auto& m : c->GetMethods(sizeof(void*))) { CheckMethod(&m, preverified); @@ -1147,7 +1147,7 @@ TEST_F(ClassLinkerTest, Preverified_InitializedBoot) { ASSERT_TRUE(JavaLangObject != nullptr); EXPECT_TRUE(JavaLangObject->IsInitialized()) << "Not testing already initialized class from the " "core"; - CheckPreverified(JavaLangObject, true); + CheckVerificationAttempted(JavaLangObject, true); } TEST_F(ClassLinkerTest, Preverified_UninitializedBoot) { @@ -1160,10 +1160,10 @@ TEST_F(ClassLinkerTest, Preverified_UninitializedBoot) { EXPECT_FALSE(security_manager->IsInitialized()) << "Not testing uninitialized class from the " "core"; - CheckPreverified(security_manager.Get(), false); + CheckVerificationAttempted(security_manager.Get(), false); class_linker_->EnsureInitialized(soa.Self(), security_manager, true, true); - CheckPreverified(security_manager.Get(), true); + CheckVerificationAttempted(security_manager.Get(), true); } TEST_F(ClassLinkerTest, Preverified_App) { @@ -1175,10 +1175,10 @@ TEST_F(ClassLinkerTest, Preverified_App) { Handle statics( hs.NewHandle(class_linker_->FindClass(soa.Self(), "LStatics;", class_loader))); - CheckPreverified(statics.Get(), false); + CheckVerificationAttempted(statics.Get(), false); class_linker_->EnsureInitialized(soa.Self(), statics, true, true); - CheckPreverified(statics.Get(), true); + CheckVerificationAttempted(statics.Get(), true); } TEST_F(ClassLinkerTest, IsBootStrapClassLoaded) { diff --git a/runtime/interpreter/interpreter.cc b/runtime/interpreter/interpreter.cc index e93bbdbf5..2559222b6 100644 --- a/runtime/interpreter/interpreter.cc +++ b/runtime/interpreter/interpreter.cc @@ -311,7 +311,7 @@ static inline JValue Execute(Thread* self, const DexFile::CodeItem* code_item, shadow_frame.GetMethod()->GetDeclaringClass()->AssertInitializedOrInitializingInThread(self); bool transaction_active = Runtime::Current()->IsActiveTransaction(); - if (LIKELY(shadow_frame.GetMethod()->IsPreverified())) { + if (LIKELY(shadow_frame.GetMethod()->SkipAccessChecks())) { // Enter the "without access check" interpreter. if (kInterpreterImplKind == kMterpImplKind) { if (transaction_active) { diff --git a/runtime/mirror/class.cc b/runtime/mirror/class.cc index b97d99424..cdc620466 100644 --- a/runtime/mirror/class.cc +++ b/runtime/mirror/class.cc @@ -800,11 +800,11 @@ ArtField* Class::FindField(Thread* self, Handle klass, const StringPiece& return nullptr; } -void Class::SetPreverifiedFlagOnAllMethods(size_t pointer_size) { +void Class::SetSkipAccessChecksFlagOnAllMethods(size_t pointer_size) { DCHECK(IsVerified()); for (auto& m : GetMethods(pointer_size)) { if (!m.IsNative() && m.IsInvokable()) { - m.SetPreverified(); + m.SetSkipAccessChecks(); } } } diff --git a/runtime/mirror/class.h b/runtime/mirror/class.h index ea614fd2c..8092db4fb 100644 --- a/runtime/mirror/class.h +++ b/runtime/mirror/class.h @@ -287,14 +287,19 @@ class MANAGED Class FINAL : public Object { return (GetAccessFlags() & kAccSynthetic) != 0; } - // Returns true if the class can avoid access checks. - bool IsPreverified() SHARED_REQUIRES(Locks::mutator_lock_) { - return (GetAccessFlags() & kAccPreverified) != 0; + // Returns true if the class had run the verifier at least once. + // This does not necessarily mean that access checks are avoidable, + // since the class methods might still need to be run with access checks. + // If this bit returns false, then the methods are not to be trusted with skipping access checks. + bool WasVerificationAttempted() SHARED_REQUIRES(Locks::mutator_lock_) { + return (GetAccessFlags() & kAccSkipAccessChecks) != 0; } - void SetPreverified() SHARED_REQUIRES(Locks::mutator_lock_) { + // Mark the class as having gone through a verification attempt. + // Mutually exclusive from whether or not each method is allowed to skip access checks. + void SetVerificationAttempted() SHARED_REQUIRES(Locks::mutator_lock_) { uint32_t flags = GetField32(OFFSET_OF_OBJECT_MEMBER(Class, access_flags_)); - SetAccessFlags(flags | kAccPreverified); + SetAccessFlags(flags | kAccVerificationAttempted); } template @@ -1127,8 +1132,8 @@ class MANAGED Class FINAL : public Object { void VisitNativeRoots(Visitor& visitor, size_t pointer_size) SHARED_REQUIRES(Locks::mutator_lock_); - // When class is verified, set the kAccPreverified flag on each method. - void SetPreverifiedFlagOnAllMethods(size_t pointer_size) + // When class is verified, set the kAccSkipAccessChecks flag on each method. + void SetSkipAccessChecksFlagOnAllMethods(size_t pointer_size) SHARED_REQUIRES(Locks::mutator_lock_); // Get the descriptor of the class. In a few cases a std::string is required, rather than diff --git a/runtime/modifiers.h b/runtime/modifiers.h index 9946eabc8..ed4c5fc76 100644 --- a/runtime/modifiers.h +++ b/runtime/modifiers.h @@ -42,14 +42,16 @@ static constexpr uint32_t kAccEnum = 0x4000; // class, field, ic (1.5) static constexpr uint32_t kAccJavaFlagsMask = 0xffff; // bits set from Java sources (low 16) -static constexpr uint32_t kAccConstructor = 0x00010000; // method (dex only) <(cl)init> -static constexpr uint32_t kAccDeclaredSynchronized = 0x00020000; // method (dex only) -static constexpr uint32_t kAccClassIsProxy = 0x00040000; // class (dex only) -static constexpr uint32_t kAccPreverified = 0x00080000; // class (runtime), - // method (dex only) -static constexpr uint32_t kAccFastNative = 0x00080000; // method (dex only) -static constexpr uint32_t kAccMiranda = 0x00200000; // method (dex only) -static constexpr uint32_t kAccDefault = 0x00400000; // method (runtime) +static constexpr uint32_t kAccConstructor = 0x00010000; // method (dex only) <(cl)init> +static constexpr uint32_t kAccDeclaredSynchronized = 0x00020000; // method (dex only) +static constexpr uint32_t kAccClassIsProxy = 0x00040000; // class (dex only) +// Used by a method to denote that its execution does not need to go through slow path interpreter. +static constexpr uint32_t kAccSkipAccessChecks = 0x00080000; // method (dex only) +// Used by a class to denote that the verifier has attempted to check it at least once. +static constexpr uint32_t kAccVerificationAttempted = 0x00080000; // class (runtime) +static constexpr uint32_t kAccFastNative = 0x00080000; // method (dex only) +static constexpr uint32_t kAccMiranda = 0x00200000; // method (dex only) +static constexpr uint32_t kAccDefault = 0x00400000; // method (runtime) // This is set by the class linker during LinkInterfaceMethods. Prior to that point we do not know // if any particular method needs to be a default conflict. Used to figure out at runtime if // invoking this method will throw an exception. diff --git a/runtime/runtime.cc b/runtime/runtime.cc index 0c06ca672..3926f06a2 100644 --- a/runtime/runtime.cc +++ b/runtime/runtime.cc @@ -1914,7 +1914,8 @@ void Runtime::SetImtUnimplementedMethod(ArtMethod* method) { } bool Runtime::IsVerificationEnabled() const { - return verify_ == verifier::VerifyMode::kEnable; + return verify_ == verifier::VerifyMode::kEnable || + verify_ == verifier::VerifyMode::kSoftFail; } bool Runtime::IsVerificationSoftFail() const { diff --git a/test/Android.run-test.mk b/test/Android.run-test.mk index 8dafd293f..92966218c 100644 --- a/test/Android.run-test.mk +++ b/test/Android.run-test.mk @@ -302,12 +302,7 @@ TEST_ART_BROKEN_NO_RELOCATE_TESTS := # Temporarily disable some broken tests when forcing access checks in interpreter b/22414682 TEST_ART_BROKEN_INTERPRETER_ACCESS_CHECK_TESTS := \ - 135-MirandaDispatch \ - 137-cfi \ - 412-new-array \ - 471-uninitialized-locals \ - 506-verify-aput \ - 800-smali + 137-cfi ifneq (,$(filter interp-ac,$(COMPILER_TYPES))) ART_TEST_KNOWN_BROKEN += $(call all-run-test-names,$(TARGET_TYPES),$(RUN_TYPES),$(PREBUILD_TYPES), \