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 f1b7458..9ab7280 100644
--- a/compiler/driver/compiler_driver.cc
+++ b/compiler/driver/compiler_driver.cc
@@ -2306,9 +2306,9 @@
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 440e796..a020e9d 100644
--- a/runtime/art_method.h
+++ b/runtime/art_method.h
@@ -173,13 +173,13 @@
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 35e1ee6..0667e23 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -3546,7 +3546,7 @@
// 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 @@
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 @@
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 @@
}
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<mirror::Class> klass) {
- if (!klass->IsPreverified()) {
- klass->SetPreverifiedFlagOnAllMethods(image_pointer_size_);
- klass->SetPreverified();
+void ClassLinker::EnsureSkipAccessChecksMethods(Handle<mirror::Class> klass) {
+ if (!klass->WasVerificationAttempted()) {
+ klass->SetSkipAccessChecksFlagOnAllMethods(image_pointer_size_);
+ klass->SetVerificationAttempted();
}
}
@@ -3745,7 +3740,7 @@
}
// 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 @@
}
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<mirror::ClassLoader*>(loader));
DCHECK_EQ(klass->GetPrimitiveType(), Primitive::kPrimNot);
klass->SetName(soa.Decode<mirror::String*>(name));
@@ -4663,7 +4659,7 @@
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 @@
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 @@
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 5176cbd..3b4e912 100644
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -957,9 +957,10 @@
void CreateProxyMethod(Handle<mirror::Class> 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<mirror::Class> 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<mirror::Class> 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 40dfda9..b86da9f 100644
--- a/runtime/class_linker_test.cc
+++ b/runtime/class_linker_test.cc
@@ -1126,14 +1126,14 @@
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 @@
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 @@
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 @@
Handle<mirror::Class> 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 e93bbdb..2559222 100644
--- a/runtime/interpreter/interpreter.cc
+++ b/runtime/interpreter/interpreter.cc
@@ -311,7 +311,7 @@
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 b97d994..cdc6204 100644
--- a/runtime/mirror/class.cc
+++ b/runtime/mirror/class.cc
@@ -800,11 +800,11 @@
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 ea614fd..8092db4 100644
--- a/runtime/mirror/class.h
+++ b/runtime/mirror/class.h
@@ -287,14 +287,19 @@
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<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags>
@@ -1127,8 +1132,8 @@
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 9946eab..ed4c5fc 100644
--- a/runtime/modifiers.h
+++ b/runtime/modifiers.h
@@ -42,14 +42,16 @@
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 0c06ca6..3926f06 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -1914,7 +1914,8 @@
}
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 8dafd29..9296621 100644
--- a/test/Android.run-test.mk
+++ b/test/Android.run-test.mk
@@ -302,12 +302,7 @@
# 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), \