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 (file)
--- 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 440e796..a020e9d 100644 (file)
--- 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 35e1ee6..0667e23 100644 (file)
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -3546,7 +3546,7 @@ void ClassLinker::VerifyClass(Thread* self, Handle<mirror::Class> 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<mirror::Class> 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<mirror::Class> 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<mirror::Class> 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<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 @@ 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<mirror::ClassLoader*>(loader));
   DCHECK_EQ(klass->GetPrimitiveType(), Primitive::kPrimNot);
   klass->SetName(soa.Decode<mirror::String*>(name));
@@ -4663,7 +4659,7 @@ bool ClassLinker::EnsureInitialized(Thread* self, Handle<mirror::Class> 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 5176cbd..3b4e912 100644 (file)
--- a/runtime/class_linker.h
+++ b/runtime/class_linker.h
@@ -957,9 +957,10 @@ class ClassLinker {
   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 (file)
--- 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<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 (file)
--- 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 b97d994..cdc6204 100644 (file)
--- a/runtime/mirror/class.cc
+++ b/runtime/mirror/class.cc
@@ -800,11 +800,11 @@ ArtField* Class::FindField(Thread* self, Handle<Class> 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 ea614fd..8092db4 100644 (file)
--- 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<VerifyObjectFlags kVerifyFlags = kDefaultVerifyFlags>
@@ -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 9946eab..ed4c5fc 100644 (file)
--- 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 0c06ca6..3926f06 100644 (file)
--- 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 8dafd29..9296621 100644 (file)
--- 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), \