Merge "ART: Move access flags checking to dex file verifier"

8 files changed, 1717 insertions, 167 deletions

diff --git a/runtime/dex_file.h b/runtime/dex_file.h
index 98d4e59ec8..47e5c124ff 100644
--- a/runtime/dex_file.h
+++ b/runtime/dex_file.h
@@ -1275,6 +1275,8 @@ class DexFile {
   // pointer to the OatDexFile it was loaded from. Otherwise oat_dex_file_ is
   // null.
   const OatDexFile* oat_dex_file_;
+
+  friend class DexFileVerifierTest;
 };
 
 struct DexFileReference {
@@ -1459,6 +1461,9 @@ class ClassDataItemIterator {
   uint32_t GetMethodCodeItemOffset() const {
     return method_.code_off_;
   }
+  const uint8_t* DataPointer() const {
+    return ptr_pos_;
+  }
   const uint8_t* EndDataPointer() const {
     CHECK(!HasNext());
     return ptr_pos_;
diff --git a/runtime/dex_file_verifier.cc b/runtime/dex_file_verifier.cc
index eec49839ef..2dd96aac8c 100644
--- a/runtime/dex_file_verifier.cc
+++ b/runtime/dex_file_verifier.cc
@@ -16,7 +16,9 @@
 
 #include "dex_file_verifier.h"
 
+#include <inttypes.h>
 #include <zlib.h>
+
 #include <memory>
 
 #include "base/stringprintf.h"
@@ -444,66 +446,86 @@ bool DexFileVerifier::CheckAndGetHandlerOffsets(const DexFile::CodeItem* code_it
   return true;
 }
 
-bool DexFileVerifier::CheckClassDataItemField(uint32_t idx, uint32_t access_flags,
+bool DexFileVerifier::CheckClassDataItemField(uint32_t idx,
+                                              uint32_t access_flags,
+                                              uint32_t class_access_flags,
+                                              uint32_t class_type_index,
                                               bool expect_static) {
+  // Check for overflow.
   if (!CheckIndex(idx, header_->field_ids_size_, "class_data_item field_idx")) {
     return false;
   }
 
+  // Check that it's the right class.
+  uint16_t my_class_index =
+      (reinterpret_cast<const DexFile::FieldId*>(begin_ + header_->field_ids_off_) + idx)->
+          class_idx_;
+  if (class_type_index != my_class_index) {
+    ErrorStringPrintf("Field's class index unexpected, %" PRIu16 "vs %" PRIu16,
+                      my_class_index,
+                      class_type_index);
+    return false;
+  }
+
+  // Check that it falls into the right class-data list.
   bool is_static = (access_flags & kAccStatic) != 0;
   if (UNLIKELY(is_static != expect_static)) {
     ErrorStringPrintf("Static/instance field not in expected list");
     return false;
   }
 
-  if (UNLIKELY((access_flags & ~kAccJavaFlagsMask) != 0)) {
-    ErrorStringPrintf("Bad class_data_item field access_flags %x", access_flags);
+  // Check field access flags.
+  std::string error_msg;
+  if (!CheckFieldAccessFlags(access_flags, class_access_flags, &error_msg)) {
+    ErrorStringPrintf("%s", error_msg.c_str());
     return false;
   }
 
   return true;
 }
 
-bool DexFileVerifier::CheckClassDataItemMethod(uint32_t idx, uint32_t access_flags,
+bool DexFileVerifier::CheckClassDataItemMethod(uint32_t idx,
+                                               uint32_t access_flags,
+                                               uint32_t class_access_flags,
+                                               uint32_t class_type_index,
                                                uint32_t code_offset,
-                                               std::unordered_set<uint32_t>& direct_method_indexes,
+                                               std::unordered_set<uint32_t>* direct_method_indexes,
                                                bool expect_direct) {
+  DCHECK(direct_method_indexes != nullptr);
+  // Check for overflow.
   if (!CheckIndex(idx, header_->method_ids_size_, "class_data_item method_idx")) {
     return false;
   }
 
-  bool is_direct = (access_flags & (kAccStatic | kAccPrivate | kAccConstructor)) != 0;
-  bool expect_code = (access_flags & (kAccNative | kAccAbstract)) == 0;
-  bool is_synchronized = (access_flags & kAccSynchronized) != 0;
-  bool allow_synchronized = (access_flags & kAccNative) != 0;
-
-  if (UNLIKELY(is_direct != expect_direct)) {
-    ErrorStringPrintf("Direct/virtual method not in expected list");
+  // Check that it's the right class.
+  uint16_t my_class_index =
+      (reinterpret_cast<const DexFile::MethodId*>(begin_ + header_->method_ids_off_) + idx)->
+          class_idx_;
+  if (class_type_index != my_class_index) {
+    ErrorStringPrintf("Method's class index unexpected, %" PRIu16 "vs %" PRIu16,
+                      my_class_index,
+                      class_type_index);
     return false;
   }
 
+  // Check that it's not defined as both direct and virtual.
   if (expect_direct) {
-    direct_method_indexes.insert(idx);
-  } else if (direct_method_indexes.find(idx) != direct_method_indexes.end()) {
+    direct_method_indexes->insert(idx);
+  } else if (direct_method_indexes->find(idx) != direct_method_indexes->end()) {
     ErrorStringPrintf("Found virtual method with same index as direct method: %d", idx);
     return false;
   }
 
-  constexpr uint32_t access_method_mask = kAccJavaFlagsMask | kAccConstructor |
-      kAccDeclaredSynchronized;
-  if (UNLIKELY(((access_flags & ~access_method_mask) != 0) ||
-               (is_synchronized && !allow_synchronized))) {
-    ErrorStringPrintf("Bad class_data_item method access_flags %x", access_flags);
-    return false;
-  }
-
-  if (UNLIKELY(expect_code && (code_offset == 0))) {
-    ErrorStringPrintf("Unexpected zero value for class_data_item method code_off with access "
-                      "flags %x", access_flags);
-    return false;
-  } else if (UNLIKELY(!expect_code && (code_offset != 0))) {
-    ErrorStringPrintf("Unexpected non-zero value %x for class_data_item method code_off"
-                      " with access flags %x", code_offset, access_flags);
+  // Check method access flags.
+  bool has_code = (code_offset != 0);
+  std::string error_msg;
+  if (!CheckMethodAccessFlags(idx,
+                              access_flags,
+                              class_access_flags,
+                              has_code,
+                              expect_direct,
+                              &error_msg)) {
+    ErrorStringPrintf("%s", error_msg.c_str());
     return false;
   }
 
@@ -689,62 +711,187 @@ bool DexFileVerifier::CheckEncodedAnnotation() {
   return true;
 }
 
-bool DexFileVerifier::CheckIntraClassDataItem() {
-  ClassDataItemIterator it(*dex_file_, ptr_);
-  std::unordered_set<uint32_t> direct_method_indexes;
+bool DexFileVerifier::FindClassFlags(uint32_t index,
+                                     bool is_field,
+                                     uint16_t* class_type_index,
+                                     uint32_t* class_access_flags) {
+  DCHECK(class_type_index != nullptr);
+  DCHECK(class_access_flags != nullptr);
 
-  // These calls use the raw access flags to check whether the whole dex field is valid.
-  uint32_t prev_index = 0;
-  for (; it.HasNextStaticField(); it.Next()) {
-    uint32_t curr_index = it.GetMemberIndex();
-    if (curr_index < prev_index) {
-      ErrorStringPrintf("out-of-order static field indexes %d and %d", prev_index, curr_index);
-      return false;
-    }
-    prev_index = curr_index;
-    if (!CheckClassDataItemField(curr_index, it.GetRawMemberAccessFlags(), true)) {
-      return false;
-    }
+  // First check if the index is valid.
+  if (index >= (is_field ? header_->field_ids_size_ : header_->method_ids_size_)) {
+    return false;
   }
-  prev_index = 0;
-  for (; it.HasNextInstanceField(); it.Next()) {
-    uint32_t curr_index = it.GetMemberIndex();
-    if (curr_index < prev_index) {
-      ErrorStringPrintf("out-of-order instance field indexes %d and %d", prev_index, curr_index);
-      return false;
+
+  // Next get the type index.
+  if (is_field) {
+    *class_type_index =
+        (reinterpret_cast<const DexFile::FieldId*>(begin_ + header_->field_ids_off_) + index)->
+            class_idx_;
+  } else {
+    *class_type_index =
+        (reinterpret_cast<const DexFile::MethodId*>(begin_ + header_->method_ids_off_) + index)->
+            class_idx_;
+  }
+
+  // Check if that is valid.
+  if (*class_type_index >= header_->type_ids_size_) {
+    return false;
+  }
+
+  // Now search for the class def. This is basically a specialized version of the DexFile code, as
+  // we should not trust that this is a valid DexFile just yet.
+  const DexFile::ClassDef* class_def_begin =
+      reinterpret_cast<const DexFile::ClassDef*>(begin_ + header_->class_defs_off_);
+  for (size_t i = 0; i < header_->class_defs_size_; ++i) {
+    const DexFile::ClassDef* class_def = class_def_begin + i;
+    if (class_def->class_idx_ == *class_type_index) {
+      *class_access_flags = class_def->access_flags_;
+      return true;
     }
-    prev_index = curr_index;
-    if (!CheckClassDataItemField(curr_index, it.GetRawMemberAccessFlags(), false)) {
+  }
+
+  // Didn't find the class-def, not defined here...
+  return false;
+}
+
+bool DexFileVerifier::CheckOrderAndGetClassFlags(bool is_field,
+                                                 const char* type_descr,
+                                                 uint32_t curr_index,
+                                                 uint32_t prev_index,
+                                                 bool* have_class,
+                                                 uint16_t* class_type_index,
+                                                 uint32_t* class_access_flags) {
+  if (curr_index < prev_index) {
+    ErrorStringPrintf("out-of-order %s indexes %" PRIu32 " and %" PRIu32,
+                      type_descr,
+                      prev_index,
+                      curr_index);
+    return false;
+  }
+
+  if (!*have_class) {
+    *have_class = FindClassFlags(curr_index, is_field, class_type_index, class_access_flags);
+    if (!*have_class) {
+      // Should have really found one.
+      ErrorStringPrintf("could not find declaring class for %s index %" PRIu32,
+                        type_descr,
+                        curr_index);
       return false;
     }
   }
-  prev_index = 0;
-  for (; it.HasNextDirectMethod(); it.Next()) {
-    uint32_t curr_index = it.GetMemberIndex();
-    if (curr_index < prev_index) {
-      ErrorStringPrintf("out-of-order direct method indexes %d and %d", prev_index, curr_index);
+  return true;
+}
+
+template <bool kStatic>
+bool DexFileVerifier::CheckIntraClassDataItemFields(ClassDataItemIterator* it,
+                                                    bool* have_class,
+                                                    uint16_t* class_type_index,
+                                                    uint32_t* class_access_flags) {
+  DCHECK(it != nullptr);
+  // These calls use the raw access flags to check whether the whole dex field is valid.
+  uint32_t prev_index = 0;
+  for (; kStatic ? it->HasNextStaticField() : it->HasNextInstanceField(); it->Next()) {
+    uint32_t curr_index = it->GetMemberIndex();
+    if (!CheckOrderAndGetClassFlags(true,
+                                    kStatic ? "static field" : "instance field",
+                                    curr_index,
+                                    prev_index,
+                                    have_class,
+                                    class_type_index,
+                                    class_access_flags)) {
       return false;
     }
     prev_index = curr_index;
-    if (!CheckClassDataItemMethod(curr_index, it.GetRawMemberAccessFlags(),
-        it.GetMethodCodeItemOffset(), direct_method_indexes, true)) {
+
+    if (!CheckClassDataItemField(curr_index,
+                                 it->GetRawMemberAccessFlags(),
+                                 *class_access_flags,
+                                 *class_type_index,
+                                 kStatic)) {
       return false;
     }
   }
-  prev_index = 0;
-  for (; it.HasNextVirtualMethod(); it.Next()) {
-    uint32_t curr_index = it.GetMemberIndex();
-    if (curr_index < prev_index) {
-      ErrorStringPrintf("out-of-order virtual method indexes %d and %d", prev_index, curr_index);
+
+  return true;
+}
+
+template <bool kDirect>
+bool DexFileVerifier::CheckIntraClassDataItemMethods(
+    ClassDataItemIterator* it,
+    std::unordered_set<uint32_t>* direct_method_indexes,
+    bool* have_class,
+    uint16_t* class_type_index,
+    uint32_t* class_access_flags) {
+  uint32_t prev_index = 0;
+  for (; kDirect ? it->HasNextDirectMethod() : it->HasNextVirtualMethod(); it->Next()) {
+    uint32_t curr_index = it->GetMemberIndex();
+    if (!CheckOrderAndGetClassFlags(false,
+                                    kDirect ? "direct method" : "virtual method",
+                                    curr_index,
+                                    prev_index,
+                                    have_class,
+                                    class_type_index,
+                                    class_access_flags)) {
       return false;
     }
     prev_index = curr_index;
-    if (!CheckClassDataItemMethod(curr_index, it.GetRawMemberAccessFlags(),
-        it.GetMethodCodeItemOffset(), direct_method_indexes, false)) {
+
+    if (!CheckClassDataItemMethod(curr_index,
+                                  it->GetRawMemberAccessFlags(),
+                                  *class_access_flags,
+                                  *class_type_index,
+                                  it->GetMethodCodeItemOffset(),
+                                  direct_method_indexes,
+                                  kDirect)) {
       return false;
     }
   }
 
+  return true;
+}
+
+bool DexFileVerifier::CheckIntraClassDataItem() {
+  ClassDataItemIterator it(*dex_file_, ptr_);
+  std::unordered_set<uint32_t> direct_method_indexes;
+
+  // This code is complicated by the fact that we don't directly know which class this belongs to.
+  // So we need to explicitly search with the first item we find (either field or method), and then,
+  // as the lookup is expensive, cache the result.
+  bool have_class = false;
+  uint16_t class_type_index;
+  uint32_t class_access_flags;
+
+  // Check fields.
+  if (!CheckIntraClassDataItemFields<true>(&it,
+                                           &have_class,
+                                           &class_type_index,
+                                           &class_access_flags)) {
+    return false;
+  }
+  if (!CheckIntraClassDataItemFields<false>(&it,
+                                            &have_class,
+                                            &class_type_index,
+                                            &class_access_flags)) {
+    return false;
+  }
+
+  // Check methods.
+  if (!CheckIntraClassDataItemMethods<true>(&it,
+                                            &direct_method_indexes,
+                                            &have_class,
+                                            &class_type_index,
+                                            &class_access_flags)) {
+    return false;
+  }
+  if (!CheckIntraClassDataItemMethods<false>(&it,
+                                             &direct_method_indexes,
+                                             &have_class,
+                                             &class_type_index,
+                                             &class_access_flags)) {
+    return false;
+  }
+
   ptr_ = it.EndDataPointer();
   return true;
 }
@@ -2149,4 +2296,259 @@ void DexFileVerifier::ErrorStringPrintf(const char* fmt, ...) {
   va_end(ap);
 }
 
+// Fields and methods may have only one of public/protected/private.
+static bool CheckAtMostOneOfPublicProtectedPrivate(uint32_t flags) {
+  size_t count = (((flags & kAccPublic) == 0) ? 0 : 1) +
+                 (((flags & kAccProtected) == 0) ? 0 : 1) +
+                 (((flags & kAccPrivate) == 0) ? 0 : 1);
+  return count <= 1;
+}
+
+bool DexFileVerifier::CheckFieldAccessFlags(uint32_t field_access_flags,
+                                            uint32_t class_access_flags,
+                                            std::string* error_msg) {
+  // Generally sort out >16-bit flags.
+  if ((field_access_flags & ~kAccJavaFlagsMask) != 0) {
+    *error_msg = StringPrintf("Bad class_data_item field access_flags %x", field_access_flags);
+    return false;
+  }
+
+  // Flags allowed on fields, in general. Other lower-16-bit flags are to be ignored.
+  constexpr uint32_t kFieldAccessFlags = kAccPublic |
+                                         kAccPrivate |
+                                         kAccProtected |
+                                         kAccStatic |
+                                         kAccFinal |
+                                         kAccVolatile |
+                                         kAccTransient |
+                                         kAccSynthetic |
+                                         kAccEnum;
+
+  // Fields may have only one of public/protected/final.
+  if (!CheckAtMostOneOfPublicProtectedPrivate(field_access_flags)) {
+    *error_msg = StringPrintf("Field may have only one of public/protected/private, %x",
+                              field_access_flags);
+    return false;
+  }
+
+  // Interfaces have a pretty restricted list.
+  if ((class_access_flags & kAccInterface) != 0) {
+    // Interface fields must be public final static.
+    constexpr uint32_t kPublicFinalStatic = kAccPublic | kAccFinal | kAccStatic;
+    if ((field_access_flags & kPublicFinalStatic) != kPublicFinalStatic) {
+      *error_msg = StringPrintf("Interface field is not public final static: %x",
+                                field_access_flags);
+      return false;
+    }
+    // Interface fields may be synthetic, but may not have other flags.
+    constexpr uint32_t kDisallowed = ~(kPublicFinalStatic | kAccSynthetic);
+    if ((field_access_flags & kFieldAccessFlags & kDisallowed) != 0) {
+      *error_msg = StringPrintf("Interface field has disallowed flag: %x", field_access_flags);
+      return false;
+    }
+    return true;
+  }
+
+  // Volatile fields may not be final.
+  constexpr uint32_t kVolatileFinal = kAccVolatile | kAccFinal;
+  if ((field_access_flags & kVolatileFinal) == kVolatileFinal) {
+    *error_msg = "Fields may not be volatile and final";
+    return false;
+  }
+
+  return true;
+}
+
+// Try to find the name of the method with the given index. We do not want to rely on DexFile
+// infrastructure at this point, so do it all by hand. begin and header correspond to begin_ and
+// header_ of the DexFileVerifier. str will contain the pointer to the method name on success
+// (flagged by the return value), otherwise error_msg will contain an error string.
+static bool FindMethodName(uint32_t method_index,
+                           const uint8_t* begin,
+                           const DexFile::Header* header,
+                           const char** str,
+                           std::string* error_msg) {
+  if (method_index >= header->method_ids_size_) {
+    *error_msg = "Method index not available for method flags verification";
+    return false;
+  }
+  uint32_t string_idx =
+      (reinterpret_cast<const DexFile::MethodId*>(begin + header->method_ids_off_) +
+          method_index)->name_idx_;
+  if (string_idx >= header->string_ids_size_) {
+    *error_msg = "String index not available for method flags verification";
+    return false;
+  }
+  uint32_t string_off =
+      (reinterpret_cast<const DexFile::StringId*>(begin + header->string_ids_off_) + string_idx)->
+          string_data_off_;
+  if (string_off >= header->file_size_) {
+    *error_msg = "String offset out of bounds for method flags verification";
+    return false;
+  }
+  const uint8_t* str_data_ptr = begin + string_off;
+  DecodeUnsignedLeb128(&str_data_ptr);
+  *str = reinterpret_cast<const char*>(str_data_ptr);
+  return true;
+}
+
+bool DexFileVerifier::CheckMethodAccessFlags(uint32_t method_index,
+                                             uint32_t method_access_flags,
+                                             uint32_t class_access_flags,
+                                             bool has_code,
+                                             bool expect_direct,
+                                             std::string* error_msg) {
+  // Generally sort out >16-bit flags, except dex knows Constructor and DeclaredSynchronized.
+  constexpr uint32_t kAllMethodFlags =
+      kAccJavaFlagsMask | kAccConstructor | kAccDeclaredSynchronized;
+  if ((method_access_flags & ~kAllMethodFlags) != 0) {
+    *error_msg = StringPrintf("Bad class_data_item method access_flags %x", method_access_flags);
+    return false;
+  }
+
+  // Flags allowed on fields, in general. Other lower-16-bit flags are to be ignored.
+  constexpr uint32_t kMethodAccessFlags = kAccPublic |
+                                          kAccPrivate |
+                                          kAccProtected |
+                                          kAccStatic |
+                                          kAccFinal |
+                                          kAccSynthetic |
+                                          kAccSynchronized |
+                                          kAccBridge |
+                                          kAccVarargs |
+                                          kAccNative |
+                                          kAccAbstract |
+                                          kAccStrict;
+
+  // Methods may have only one of public/protected/final.
+  if (!CheckAtMostOneOfPublicProtectedPrivate(method_access_flags)) {
+    *error_msg = StringPrintf("Method may have only one of public/protected/private, %x",
+                              method_access_flags);
+    return false;
+  }
+
+  // Try to find the name, to check for constructor properties.
+  const char* str;
+  if (!FindMethodName(method_index, begin_, header_, &str, error_msg)) {
+    return false;
+  }
+  bool is_init_by_name = false;
+  constexpr const char* kInitName = "<init>";
+  size_t str_offset = (reinterpret_cast<const uint8_t*>(str) - begin_);
+  if (header_->file_size_ - str_offset >= sizeof(kInitName)) {
+    is_init_by_name = strcmp(kInitName, str) == 0;
+  }
+  bool is_clinit_by_name = false;
+  constexpr const char* kClinitName = "<clinit>";
+  if (header_->file_size_ - str_offset >= sizeof(kClinitName)) {
+    is_clinit_by_name = strcmp(kClinitName, str) == 0;
+  }
+  bool is_constructor = is_init_by_name || is_clinit_by_name;
+
+  // Only methods named "<clinit>" or "<init>" may be marked constructor. Note: we cannot enforce
+  // the reverse for backwards compatibility reasons.
+  if (((method_access_flags & kAccConstructor) != 0) && !is_constructor) {
+    *error_msg = StringPrintf("Method %" PRIu32 " is marked constructor, but doesn't match name",
+                              method_index);
+    return false;
+  }
+  // Check that the static constructor (= static initializer) is named "<clinit>" and that the
+  // instance constructor is called "<init>".
+  if (is_constructor) {
+    bool is_static = (method_access_flags & kAccStatic) != 0;
+    if (is_static ^ is_clinit_by_name) {
+      *error_msg = StringPrintf("Constructor %" PRIu32 " is not flagged correctly wrt/ static.",
+                                method_index);
+      return false;
+    }
+  }
+  // Check that static and private methods, as well as constructors, are in the direct methods list,
+  // and other methods in the virtual methods list.
+  bool is_direct = (method_access_flags & (kAccStatic | kAccPrivate)) != 0 || is_constructor;
+  if (is_direct != expect_direct) {
+    *error_msg = StringPrintf("Direct/virtual method %" PRIu32 " not in expected list %d",
+                              method_index,
+                              expect_direct);
+    return false;
+  }
+
+
+  // From here on out it is easier to mask out the bits we're supposed to ignore.
+  method_access_flags &= kMethodAccessFlags;
+
+  // If there aren't any instructions, make sure that's expected.
+  if (!has_code) {
+    // Only native or abstract methods may not have code.
+    if ((method_access_flags & (kAccNative | kAccAbstract)) == 0) {
+      *error_msg = StringPrintf("Method %" PRIu32 " has no code, but is not marked native or "
+                                "abstract",
+                                method_index);
+      return false;
+    }
+    // Constructors must always have code.
+    if (is_constructor) {
+      *error_msg = StringPrintf("Constructor %u must not be abstract or native", method_index);
+      return false;
+    }
+    if ((method_access_flags & kAccAbstract) != 0) {
+      // Abstract methods are not allowed to have the following flags.
+      constexpr uint32_t kForbidden =
+          kAccPrivate | kAccStatic | kAccFinal | kAccNative | kAccStrict | kAccSynchronized;
+      if ((method_access_flags & kForbidden) != 0) {
+        *error_msg = StringPrintf("Abstract method %" PRIu32 " has disallowed access flags %x",
+                                  method_index,
+                                  method_access_flags);
+        return false;
+      }
+      // Abstract methods must be in an abstract class or interface.
+      if ((class_access_flags & (kAccInterface | kAccAbstract)) == 0) {
+        *error_msg = StringPrintf("Method %" PRIu32 " is abstract, but the declaring class "
+                                  "is neither abstract nor an interface", method_index);
+        return false;
+      }
+    }
+    // Interfaces are special.
+    if ((class_access_flags & kAccInterface) != 0) {
+      // Interface methods must be public and abstract.
+      if ((method_access_flags & (kAccPublic | kAccAbstract)) != (kAccPublic | kAccAbstract)) {
+        *error_msg = StringPrintf("Interface method %" PRIu32 " is not public and abstract",
+                                  method_index);
+        return false;
+      }
+      // At this point, we know the method is public and abstract. This means that all the checks
+      // for invalid combinations above applies. In addition, interface methods must not be
+      // protected. This is caught by the check for only-one-of-public-protected-private.
+    }
+    return true;
+  }
+
+  // When there's code, the method must not be native or abstract.
+  if ((method_access_flags & (kAccNative | kAccAbstract)) != 0) {
+    *error_msg = StringPrintf("Method %" PRIu32 " has code, but is marked native or abstract",
+                              method_index);
+    return false;
+  }
+
+  // Only the static initializer may have code in an interface.
+  if (((class_access_flags & kAccInterface) != 0) && !is_clinit_by_name) {
+    *error_msg = StringPrintf("Non-clinit interface method %" PRIu32 " should not have code",
+                              method_index);
+    return false;
+  }
+
+  // Instance constructors must not be synchronized and a few other flags.
+  if (is_init_by_name) {
+    static constexpr uint32_t kInitAllowed =
+        kAccPrivate | kAccProtected | kAccPublic | kAccStrict | kAccVarargs | kAccSynthetic;
+    if ((method_access_flags & ~kInitAllowed) != 0) {
+      *error_msg = StringPrintf("Constructor %" PRIu32 " flagged inappropriately %x",
+                                method_index,
+                                method_access_flags);
+      return false;
+    }
+  }
+
+  return true;
+}
+
 }  // namespace art
diff --git a/runtime/dex_file_verifier.h b/runtime/dex_file_verifier.h
index ccc40d4442..c964b79439 100644
--- a/runtime/dex_file_verifier.h
+++ b/runtime/dex_file_verifier.h
@@ -57,16 +57,48 @@ class DexFileVerifier {
   uint32_t ReadUnsignedLittleEndian(uint32_t size);
   bool CheckAndGetHandlerOffsets(const DexFile::CodeItem* code_item,
                                  uint32_t* handler_offsets, uint32_t handlers_size);
-  bool CheckClassDataItemField(uint32_t idx, uint32_t access_flags, bool expect_static);
-  bool CheckClassDataItemMethod(uint32_t idx, uint32_t access_flags, uint32_t code_offset,
-                                std::unordered_set<uint32_t>& direct_method_indexes,
+  bool CheckClassDataItemField(uint32_t idx,
+                               uint32_t access_flags,
+                               uint32_t class_access_flags,
+                               uint32_t class_type_index,
+                               bool expect_static);
+  bool CheckClassDataItemMethod(uint32_t idx,
+                                uint32_t access_flags,
+                                uint32_t class_access_flags,
+                                uint32_t class_type_index,
+                                uint32_t code_offset,
+                                std::unordered_set<uint32_t>* direct_method_indexes,
                                 bool expect_direct);
+  bool CheckOrderAndGetClassFlags(bool is_field,
+                                  const char* type_descr,
+                                  uint32_t curr_index,
+                                  uint32_t prev_index,
+                                  bool* have_class,
+                                  uint16_t* class_type_index,
+                                  uint32_t* class_access_flags);
+
   bool CheckPadding(size_t offset, uint32_t aligned_offset);
   bool CheckEncodedValue();
   bool CheckEncodedArray();
   bool CheckEncodedAnnotation();
 
   bool CheckIntraClassDataItem();
+  // Check all fields of the given type from the given iterator. Load the class data from the first
+  // field, if necessary (and return it), or use the given values.
+  template <bool kStatic>
+  bool CheckIntraClassDataItemFields(ClassDataItemIterator* it,
+                                     bool* have_class,
+                                     uint16_t* class_type_index,
+                                     uint32_t* class_access_flags);
+  // Check all methods of the given type from the given iterator. Load the class data from the first
+  // method, if necessary (and return it), or use the given values.
+  template <bool kDirect>
+  bool CheckIntraClassDataItemMethods(ClassDataItemIterator* it,
+                                      std::unordered_set<uint32_t>* direct_method_indexes,
+                                      bool* have_class,
+                                      uint16_t* class_type_index,
+                                      uint32_t* class_access_flags);
+
   bool CheckIntraCodeItem();
   bool CheckIntraStringDataItem();
   bool CheckIntraDebugInfoItem();
@@ -112,6 +144,31 @@ class DexFileVerifier {
   void ErrorStringPrintf(const char* fmt, ...)
       __attribute__((__format__(__printf__, 2, 3))) COLD_ATTR;
 
+  // Retrieve class index and class access flag from the given member. index is the member index,
+  // which is taken as either a field or a method index (as designated by is_field). The result,
+  // if the member and declaring class could be found, is stored in class_type_index and
+  // class_access_flags.
+  // This is an expensive lookup, as we have to find the class-def by type index, which is a
+  // linear search. The output values should thus be cached by the caller.
+  bool FindClassFlags(uint32_t index,
+                      bool is_field,
+                      uint16_t* class_type_index,
+                      uint32_t* class_access_flags);
+
+  // Check validity of the given access flags, interpreted for a field in the context of a class
+  // with the given second access flags.
+  static bool CheckFieldAccessFlags(uint32_t field_access_flags,
+                                    uint32_t class_access_flags,
+                                    std::string* error_msg);
+  // Check validity of the given method and access flags, in the context of a class with the given
+  // second access flags.
+  bool CheckMethodAccessFlags(uint32_t method_index,
+                              uint32_t method_access_flags,
+                              uint32_t class_access_flags,
+                              bool has_code,
+                              bool expect_direct,
+                              std::string* error_msg);
+
   const DexFile* const dex_file_;
   const uint8_t* const begin_;
   const size_t size_;
diff --git a/runtime/dex_file_verifier_test.cc b/runtime/dex_file_verifier_test.cc
index 9f1ffec35f..1b529c9240 100644
--- a/runtime/dex_file_verifier_test.cc
+++ b/runtime/dex_file_verifier_test.cc
@@ -18,18 +18,20 @@
 
 #include "sys/mman.h"
 #include "zlib.h"
+#include <functional>
 #include <memory>
 
 #include "base/unix_file/fd_file.h"
+#include "base/bit_utils.h"
 #include "base/macros.h"
 #include "common_runtime_test.h"
+#include "dex_file-inl.h"
+#include "leb128.h"
 #include "scoped_thread_state_change.h"
 #include "thread-inl.h"
 
 namespace art {
 
-class DexFileVerifierTest : public CommonRuntimeTest {};
-
 static const uint8_t kBase64Map[256] = {
   255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
   255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
@@ -101,6 +103,64 @@ static inline uint8_t* DecodeBase64(const char* src, size_t* dst_size) {
   return dst.release();
 }
 
+static void FixUpChecksum(uint8_t* dex_file) {
+  DexFile::Header* header = reinterpret_cast<DexFile::Header*>(dex_file);
+  uint32_t expected_size = header->file_size_;
+  uint32_t adler_checksum = adler32(0L, Z_NULL, 0);
+  const uint32_t non_sum = sizeof(DexFile::Header::magic_) + sizeof(DexFile::Header::checksum_);
+  const uint8_t* non_sum_ptr = dex_file + non_sum;
+  adler_checksum = adler32(adler_checksum, non_sum_ptr, expected_size - non_sum);
+  header->checksum_ = adler_checksum;
+}
+
+// Custom deleter. Necessary to clean up the memory we use (to be able to mutate).
+struct DexFileDeleter {
+  void operator()(DexFile* in) {
+    if (in != nullptr) {
+      delete in->Begin();
+      delete in;
+    }
+  }
+};
+
+using DexFileUniquePtr = std::unique_ptr<DexFile, DexFileDeleter>;
+
+class DexFileVerifierTest : public CommonRuntimeTest {
+ protected:
+  void VerifyModification(const char* dex_file_base64_content,
+                          const char* location,
+                          std::function<void(DexFile*)> f,
+                          const char* expected_error) {
+    DexFileUniquePtr dex_file(WrapAsDexFile(dex_file_base64_content));
+    f(dex_file.get());
+    FixUpChecksum(const_cast<uint8_t*>(dex_file->Begin()));
+
+    std::string error_msg;
+    bool success = DexFileVerifier::Verify(dex_file.get(),
+                                           dex_file->Begin(),
+                                           dex_file->Size(),
+                                           location,
+                                           &error_msg);
+    if (expected_error == nullptr) {
+      EXPECT_TRUE(success) << error_msg;
+    } else {
+      EXPECT_FALSE(success) << "Expected " << expected_error;
+      if (!success) {
+        EXPECT_NE(error_msg.find(expected_error), std::string::npos) << error_msg;
+      }
+    }
+  }
+
+ private:
+  static DexFile* WrapAsDexFile(const char* dex_file_content_in_base_64) {
+    // Decode base64.
+    size_t length;
+    uint8_t* dex_bytes = DecodeBase64(dex_file_content_in_base_64, &length);
+    CHECK(dex_bytes != nullptr);
+    return new DexFile(dex_bytes, length, "tmp", 0, nullptr, nullptr);
+  }
+};
+
 static std::unique_ptr<const DexFile> OpenDexFileBase64(const char* base64,
                                                         const char* location,
                                                         std::string* error_msg) {
@@ -133,7 +193,6 @@ static std::unique_ptr<const DexFile> OpenDexFileBase64(const char* base64,
   return dex_file;
 }
 
-
 // For reference.
 static const char kGoodTestDex[] =
     "ZGV4CjAzNQDrVbyVkxX1HljTznNf95AglkUAhQuFtmKkAgAAcAAAAHhWNBIAAAAAAAAAAAQCAAAN"
@@ -157,94 +216,1005 @@ TEST_F(DexFileVerifierTest, GoodDex) {
   ASSERT_TRUE(raw.get() != nullptr) << error_msg;
 }
 
-static void FixUpChecksum(uint8_t* dex_file) {
-  DexFile::Header* header = reinterpret_cast<DexFile::Header*>(dex_file);
-  uint32_t expected_size = header->file_size_;
-  uint32_t adler_checksum = adler32(0L, Z_NULL, 0);
-  const uint32_t non_sum = sizeof(DexFile::Header::magic_) + sizeof(DexFile::Header::checksum_);
-  const uint8_t* non_sum_ptr = dex_file + non_sum;
-  adler_checksum = adler32(adler_checksum, non_sum_ptr, expected_size - non_sum);
-  header->checksum_ = adler_checksum;
+TEST_F(DexFileVerifierTest, MethodId) {
+  // Class idx error.
+  VerifyModification(
+      kGoodTestDex,
+      "method_id_class_idx",
+      [](DexFile* dex_file) {
+        DexFile::MethodId* method_id = const_cast<DexFile::MethodId*>(&dex_file->GetMethodId(0));
+        method_id->class_idx_ = 0xFF;
+      },
+      "could not find declaring class for direct method index 0");
+
+  // Proto idx error.
+  VerifyModification(
+      kGoodTestDex,
+      "method_id_proto_idx",
+      [](DexFile* dex_file) {
+        DexFile::MethodId* method_id = const_cast<DexFile::MethodId*>(&dex_file->GetMethodId(0));
+        method_id->proto_idx_ = 0xFF;
+      },
+      "inter_method_id_item proto_idx");
+
+  // Name idx error.
+  VerifyModification(
+      kGoodTestDex,
+      "method_id_name_idx",
+      [](DexFile* dex_file) {
+        DexFile::MethodId* method_id = const_cast<DexFile::MethodId*>(&dex_file->GetMethodId(0));
+        method_id->name_idx_ = 0xFF;
+      },
+      "String index not available for method flags verification");
 }
 
-static std::unique_ptr<const DexFile> FixChecksumAndOpen(uint8_t* bytes, size_t length,
-                                                         const char* location,
-                                                         std::string* error_msg) {
-  // Check data.
-  CHECK(bytes != nullptr);
+// Method flags test class generated from the following smali code. The declared-synchronized
+// flags are there to enforce a 3-byte uLEB128 encoding so we don't have to relayout
+// the code, but we need to remove them before doing tests.
+//
+// .class public LMethodFlags;
+// .super Ljava/lang/Object;
+//
+// .method public static constructor <clinit>()V
+// .registers 1
+//     return-void
+// .end method
+//
+// .method public constructor <init>()V
+// .registers 1
+//     return-void
+// .end method
+//
+// .method private declared-synchronized foo()V
+// .registers 1
+//     return-void
+// .end method
+//
+// .method public declared-synchronized bar()V
+// .registers 1
+//     return-void
+// .end method
 
-  // Fixup of checksum.
-  FixUpChecksum(bytes);
+static const char kMethodFlagsTestDex[] =
+    "ZGV4CjAzNQCyOQrJaDBwiIWv5MIuYKXhxlLLsQcx5SwgAgAAcAAAAHhWNBIAAAAAAAAAAJgBAAAH"
+    "AAAAcAAAAAMAAACMAAAAAQAAAJgAAAAAAAAAAAAAAAQAAACkAAAAAQAAAMQAAAA8AQAA5AAAAOQA"
+    "AADuAAAA9gAAAAUBAAAZAQAAHAEAACEBAAACAAAAAwAAAAQAAAAEAAAAAgAAAAAAAAAAAAAAAAAA"
+    "AAAAAAABAAAAAAAAAAUAAAAAAAAABgAAAAAAAAABAAAAAQAAAAAAAAD/////AAAAAHoBAAAAAAAA"
+    "CDxjbGluaXQ+AAY8aW5pdD4ADUxNZXRob2RGbGFnczsAEkxqYXZhL2xhbmcvT2JqZWN0OwABVgAD"
+    "YmFyAANmb28AAAAAAAAAAQAAAAAAAAAAAAAAAQAAAA4AAAABAAEAAAAAAAAAAAABAAAADgAAAAEA"
+    "AQAAAAAAAAAAAAEAAAAOAAAAAQABAAAAAAAAAAAAAQAAAA4AAAADAQCJgASsAgGBgATAAgKCgAjU"
+    "AgKBgAjoAgAACwAAAAAAAAABAAAAAAAAAAEAAAAHAAAAcAAAAAIAAAADAAAAjAAAAAMAAAABAAAA"
+    "mAAAAAUAAAAEAAAApAAAAAYAAAABAAAAxAAAAAIgAAAHAAAA5AAAAAMQAAABAAAAKAEAAAEgAAAE"
+    "AAAALAEAAAAgAAABAAAAegEAAAAQAAABAAAAmAEAAA==";
 
-  // write to provided file
-  std::unique_ptr<File> file(OS::CreateEmptyFile(location));
-  CHECK(file.get() != nullptr);
-  if (!file->WriteFully(bytes, length)) {
-    PLOG(FATAL) << "Failed to write base64 as dex file";
+// Find the method data for the first method with the given name (from class 0). Note: the pointer
+// is to the access flags, so that the caller doesn't have to handle the leb128-encoded method-index
+// delta.
+static const uint8_t* FindMethodData(const DexFile* dex_file, const char* name) {
+  const DexFile::ClassDef& class_def = dex_file->GetClassDef(0);
+  const uint8_t* class_data = dex_file->GetClassData(class_def);
+
+  ClassDataItemIterator it(*dex_file, class_data);
+
+  const uint8_t* trailing = class_data;
+  // Need to manually decode the four entries. DataPointer() doesn't work for this, as the first
+  // element has already been loaded into the iterator.
+  DecodeUnsignedLeb128(&trailing);
+  DecodeUnsignedLeb128(&trailing);
+  DecodeUnsignedLeb128(&trailing);
+  DecodeUnsignedLeb128(&trailing);
+
+  // Skip all fields.
+  while (it.HasNextStaticField() || it.HasNextInstanceField()) {
+    trailing = it.DataPointer();
+    it.Next();
   }
-  if (file->FlushCloseOrErase() != 0) {
-    PLOG(FATAL) << "Could not flush and close test file.";
+
+  while (it.HasNextDirectMethod() || it.HasNextVirtualMethod()) {
+    uint32_t method_index = it.GetMemberIndex();
+    uint32_t name_index = dex_file->GetMethodId(method_index).name_idx_;
+    const DexFile::StringId& string_id = dex_file->GetStringId(name_index);
+    const char* str = dex_file->GetStringData(string_id);
+    if (strcmp(name, str) == 0) {
+      DecodeUnsignedLeb128(&trailing);
+      return trailing;
+    }
+
+    trailing = it.DataPointer();
+    it.Next();
   }
-  file.reset();
 
-  // read dex file
-  ScopedObjectAccess soa(Thread::Current());
-  std::vector<std::unique_ptr<const DexFile>> tmp;
-  if (!DexFile::Open(location, location, error_msg, &tmp)) {
-    return nullptr;
+  return nullptr;
+}
+
+// Set the method flags to the given value.
+static void SetMethodFlags(DexFile* dex_file, const char* method, uint32_t mask) {
+  uint8_t* method_flags_ptr = const_cast<uint8_t*>(FindMethodData(dex_file, method));
+  CHECK(method_flags_ptr != nullptr) << method;
+
+    // Unroll this, as we only have three bytes, anyways.
+  uint8_t base1 = static_cast<uint8_t>(mask & 0x7F);
+  *(method_flags_ptr++) = (base1 | 0x80);
+  mask >>= 7;
+
+  uint8_t base2 = static_cast<uint8_t>(mask & 0x7F);
+  *(method_flags_ptr++) = (base2 | 0x80);
+  mask >>= 7;
+
+  uint8_t base3 = static_cast<uint8_t>(mask & 0x7F);
+  *method_flags_ptr = base3;
+}
+
+static uint32_t GetMethodFlags(DexFile* dex_file, const char* method) {
+  const uint8_t* method_flags_ptr = const_cast<uint8_t*>(FindMethodData(dex_file, method));
+  CHECK(method_flags_ptr != nullptr) << method;
+  return DecodeUnsignedLeb128(&method_flags_ptr);
+}
+
+// Apply the given mask to method flags.
+static void ApplyMaskToMethodFlags(DexFile* dex_file, const char* method, uint32_t mask) {
+  uint32_t value = GetMethodFlags(dex_file, method);
+  value &= mask;
+  SetMethodFlags(dex_file, method, value);
+}
+
+// Apply the given mask to method flags.
+static void OrMaskToMethodFlags(DexFile* dex_file, const char* method, uint32_t mask) {
+  uint32_t value = GetMethodFlags(dex_file, method);
+  value |= mask;
+  SetMethodFlags(dex_file, method, value);
+}
+
+// Set code_off to 0 for the method.
+static void RemoveCode(DexFile* dex_file, const char* method) {
+  const uint8_t* ptr = FindMethodData(dex_file, method);
+  // Next is flags, pass.
+  DecodeUnsignedLeb128(&ptr);
+
+  // Figure out how many bytes the code_off is.
+  const uint8_t* tmp = ptr;
+  DecodeUnsignedLeb128(&tmp);
+  size_t bytes = tmp - ptr;
+
+  uint8_t* mod = const_cast<uint8_t*>(ptr);
+  for (size_t i = 1; i < bytes; ++i) {
+    *(mod++) = 0x80;
   }
-  EXPECT_EQ(1U, tmp.size());
-  std::unique_ptr<const DexFile> dex_file = std::move(tmp[0]);
-  EXPECT_EQ(PROT_READ, dex_file->GetPermissions());
-  EXPECT_TRUE(dex_file->IsReadOnly());
-  return dex_file;
+  *mod = 0x00;
 }
 
-static bool ModifyAndLoad(const char* dex_file_content, const char* location, size_t offset,
-                          uint8_t new_val, std::string* error_msg) {
-  // Decode base64.
-  size_t length;
-  std::unique_ptr<uint8_t[]> dex_bytes(DecodeBase64(dex_file_content, &length));
-  CHECK(dex_bytes.get() != nullptr);
+TEST_F(DexFileVerifierTest, MethodAccessFlagsBase) {
+  // Check that it's OK when the wrong declared-synchronized flag is removed from "foo."
+  VerifyModification(
+      kMethodFlagsTestDex,
+      "method_flags_ok",
+      [](DexFile* dex_file) {
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+        ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+      },
+      nullptr);
+}
+
+TEST_F(DexFileVerifierTest, MethodAccessFlagsConstructors) {
+  // Make sure we still accept constructors without their flags.
+  VerifyModification(
+      kMethodFlagsTestDex,
+      "method_flags_missing_constructor_tag_ok",
+      [](DexFile* dex_file) {
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+        ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToMethodFlags(dex_file, "<init>", ~kAccConstructor);
+        ApplyMaskToMethodFlags(dex_file, "<clinit>", ~kAccConstructor);
+      },
+      nullptr);
 
-  // Make modifications.
-  dex_bytes.get()[offset] = new_val;
+  constexpr const char* kConstructors[] = { "<clinit>", "<init>"};
+  for (size_t i = 0; i < 2; ++i) {
+    // Constructor with code marked native.
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "method_flags_constructor_native",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
 
-  // Fixup and load.
-  std::unique_ptr<const DexFile> file(FixChecksumAndOpen(dex_bytes.get(), length, location,
-                                                         error_msg));
-  return file.get() != nullptr;
+          OrMaskToMethodFlags(dex_file, kConstructors[i], kAccNative);
+        },
+        "has code, but is marked native or abstract");
+    // Constructor with code marked abstract.
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "method_flags_constructor_abstract",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          OrMaskToMethodFlags(dex_file, kConstructors[i], kAccAbstract);
+        },
+        "has code, but is marked native or abstract");
+    // Constructor as-is without code.
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "method_flags_constructor_nocode",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          RemoveCode(dex_file, kConstructors[i]);
+        },
+        "has no code, but is not marked native or abstract");
+    // Constructor without code marked native.
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "method_flags_constructor_native_nocode",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          OrMaskToMethodFlags(dex_file, kConstructors[i], kAccNative);
+          RemoveCode(dex_file, kConstructors[i]);
+        },
+        "must not be abstract or native");
+    // Constructor without code marked abstract.
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "method_flags_constructor_abstract_nocode",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          OrMaskToMethodFlags(dex_file, kConstructors[i], kAccAbstract);
+          RemoveCode(dex_file, kConstructors[i]);
+        },
+        "must not be abstract or native");
+  }
+  // <init> may only have (modulo ignored):
+  // kAccPrivate | kAccProtected | kAccPublic | kAccStrict | kAccVarargs | kAccSynthetic
+  static constexpr uint32_t kInitAllowed[] = {
+      0,
+      kAccPrivate,
+      kAccProtected,
+      kAccPublic,
+      kAccStrict,
+      kAccVarargs,
+      kAccSynthetic
+  };
+  for (size_t i = 0; i < arraysize(kInitAllowed); ++i) {
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "init_allowed_flags",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          ApplyMaskToMethodFlags(dex_file, "<init>", ~kAccPublic);
+          OrMaskToMethodFlags(dex_file, "<init>", kInitAllowed[i]);
+        },
+        nullptr);
+  }
+  // Only one of public-private-protected.
+  for (size_t i = 1; i < 8; ++i) {
+    if (POPCOUNT(i) < 2) {
+      continue;
+    }
+    // Technically the flags match, but just be defensive here.
+    uint32_t mask = ((i & 1) != 0 ? kAccPrivate : 0) |
+                    ((i & 2) != 0 ? kAccProtected : 0) |
+                    ((i & 4) != 0 ? kAccPublic : 0);
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "init_one_of_ppp",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          ApplyMaskToMethodFlags(dex_file, "<init>", ~kAccPublic);
+          OrMaskToMethodFlags(dex_file, "<init>", mask);
+        },
+        "Method may have only one of public/protected/private");
+  }
+  // <init> doesn't allow
+  // kAccStatic | kAccFinal | kAccSynchronized | kAccBridge
+  // Need to handle static separately as it has its own error message.
+  VerifyModification(
+      kMethodFlagsTestDex,
+      "init_not_allowed_flags",
+      [&](DexFile* dex_file) {
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+        ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToMethodFlags(dex_file, "<init>", ~kAccPublic);
+        OrMaskToMethodFlags(dex_file, "<init>", kAccStatic);
+      },
+      "Constructor 1 is not flagged correctly wrt/ static");
+  static constexpr uint32_t kInitNotAllowed[] = {
+      kAccFinal,
+      kAccSynchronized,
+      kAccBridge
+  };
+  for (size_t i = 0; i < arraysize(kInitNotAllowed); ++i) {
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "init_not_allowed_flags",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          ApplyMaskToMethodFlags(dex_file, "<init>", ~kAccPublic);
+          OrMaskToMethodFlags(dex_file, "<init>", kInitNotAllowed[i]);
+        },
+        "Constructor 1 flagged inappropriately");
+  }
 }
 
-TEST_F(DexFileVerifierTest, MethodId) {
-  {
-    // Class error.
-    ScratchFile tmp;
-    std::string error_msg;
-    bool success = !ModifyAndLoad(kGoodTestDex, tmp.GetFilename().c_str(), 220, 0xFFU, &error_msg);
-    ASSERT_TRUE(success);
-    ASSERT_NE(error_msg.find("inter_method_id_item class_idx"), std::string::npos) << error_msg;
+TEST_F(DexFileVerifierTest, MethodAccessFlagsMethods) {
+  constexpr const char* kMethods[] = { "foo", "bar"};
+  for (size_t i = 0; i < arraysize(kMethods); ++i) {
+    // Make sure we reject non-constructors marked as constructors.
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "method_flags_non_constructor",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          OrMaskToMethodFlags(dex_file, kMethods[i], kAccConstructor);
+        },
+        "is marked constructor, but doesn't match name");
+
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "method_flags_native_with_code",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          OrMaskToMethodFlags(dex_file, kMethods[i], kAccNative);
+        },
+        "has code, but is marked native or abstract");
+
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "method_flags_abstract_with_code",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          OrMaskToMethodFlags(dex_file, kMethods[i], kAccAbstract);
+        },
+        "has code, but is marked native or abstract");
+
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "method_flags_non_abstract_native_no_code",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          RemoveCode(dex_file, kMethods[i]);
+        },
+        "has no code, but is not marked native or abstract");
+
+    // Abstract methods may not have the following flags.
+    constexpr uint32_t kAbstractDisallowed[] = {
+        kAccPrivate,
+        kAccStatic,
+        kAccFinal,
+        kAccNative,
+        kAccStrict,
+        kAccSynchronized,
+    };
+    for (size_t j = 0; j < arraysize(kAbstractDisallowed); ++j) {
+      VerifyModification(
+          kMethodFlagsTestDex,
+          "method_flags_abstract_and_disallowed_no_code",
+          [&](DexFile* dex_file) {
+            ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+            ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+            RemoveCode(dex_file, kMethods[i]);
+
+            // Can't check private and static with foo, as it's in the virtual list and gives a
+            // different error.
+            if (((GetMethodFlags(dex_file, kMethods[i]) & kAccPublic) != 0) &&
+                ((kAbstractDisallowed[j] & (kAccPrivate | kAccStatic)) != 0)) {
+              // Use another breaking flag.
+              OrMaskToMethodFlags(dex_file, kMethods[i], kAccAbstract | kAccFinal);
+            } else {
+              OrMaskToMethodFlags(dex_file, kMethods[i], kAccAbstract | kAbstractDisallowed[j]);
+            }
+          },
+          "has disallowed access flags");
+    }
+
+    // Only one of public-private-protected.
+    for (size_t j = 1; j < 8; ++j) {
+      if (POPCOUNT(j) < 2) {
+        continue;
+      }
+      // Technically the flags match, but just be defensive here.
+      uint32_t mask = ((j & 1) != 0 ? kAccPrivate : 0) |
+                      ((j & 2) != 0 ? kAccProtected : 0) |
+                      ((j & 4) != 0 ? kAccPublic : 0);
+      VerifyModification(
+          kMethodFlagsTestDex,
+          "method_flags_one_of_ppp",
+          [&](DexFile* dex_file) {
+            ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+            ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+            ApplyMaskToMethodFlags(dex_file, kMethods[i], ~kAccPublic);
+            OrMaskToMethodFlags(dex_file, kMethods[i], mask);
+          },
+          "Method may have only one of public/protected/private");
+    }
   }
+}
 
-  {
-    // Proto error.
-    ScratchFile tmp;
-    std::string error_msg;
-    bool success = !ModifyAndLoad(kGoodTestDex, tmp.GetFilename().c_str(), 222, 0xFFU, &error_msg);
-    ASSERT_TRUE(success);
-    ASSERT_NE(error_msg.find("inter_method_id_item proto_idx"), std::string::npos) << error_msg;
+TEST_F(DexFileVerifierTest, MethodAccessFlagsIgnoredOK) {
+  constexpr const char* kMethods[] = { "<clinit>", "<init>", "foo", "bar"};
+  for (size_t i = 0; i < arraysize(kMethods); ++i) {
+    // All interesting method flags, other flags are to be ignored.
+    constexpr uint32_t kAllMethodFlags =
+        kAccPublic |
+        kAccPrivate |
+        kAccProtected |
+        kAccStatic |
+        kAccFinal |
+        kAccSynchronized |
+        kAccBridge |
+        kAccVarargs |
+        kAccNative |
+        kAccAbstract |
+        kAccStrict |
+        kAccSynthetic;
+    constexpr uint32_t kIgnoredMask = ~kAllMethodFlags & 0xFFFF;
+    VerifyModification(
+        kMethodFlagsTestDex,
+        "method_flags_ignored",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToMethodFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          OrMaskToMethodFlags(dex_file, kMethods[i], kIgnoredMask);
+        },
+        nullptr);
   }
+}
 
-  {
-    // Name error.
-    ScratchFile tmp;
-    std::string error_msg;
-    bool success = !ModifyAndLoad(kGoodTestDex, tmp.GetFilename().c_str(), 224, 0xFFU, &error_msg);
-    ASSERT_TRUE(success);
-    ASSERT_NE(error_msg.find("inter_method_id_item name_idx"), std::string::npos) << error_msg;
+// Set of dex files for interface method tests. As it's not as easy to mutate method names, it's
+// just easier to break up bad cases.
+
+// Interface with an instance constructor.
+//
+// .class public interface LInterfaceMethodFlags;
+// .super Ljava/lang/Object;
+//
+// .method public static constructor <clinit>()V
+// .registers 1
+//     return-void
+// .end method
+//
+// .method public constructor <init>()V
+// .registers 1
+//     return-void
+// .end method
+static const char kMethodFlagsInterfaceWithInit[] =
+    "ZGV4CjAzNQDRNt+hZ6X3I+xe66iVlCW7h9I38HmN4SvUAQAAcAAAAHhWNBIAAAAAAAAAAEwBAAAF"
+    "AAAAcAAAAAMAAACEAAAAAQAAAJAAAAAAAAAAAAAAAAIAAACcAAAAAQAAAKwAAAAIAQAAzAAAAMwA"
+    "AADWAAAA3gAAAPYAAAAKAQAAAgAAAAMAAAAEAAAABAAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAQAA"
+    "AAAAAAABAgAAAQAAAAAAAAD/////AAAAADoBAAAAAAAACDxjbGluaXQ+AAY8aW5pdD4AFkxJbnRl"
+    "cmZhY2VNZXRob2RGbGFnczsAEkxqYXZhL2xhbmcvT2JqZWN0OwABVgAAAAAAAAAAAQAAAAAAAAAA"
+    "AAAAAQAAAA4AAAABAAEAAAAAAAAAAAABAAAADgAAAAIAAImABJQCAYGABKgCAAALAAAAAAAAAAEA"
+    "AAAAAAAAAQAAAAUAAABwAAAAAgAAAAMAAACEAAAAAwAAAAEAAACQAAAABQAAAAIAAACcAAAABgAA"
+    "AAEAAACsAAAAAiAAAAUAAADMAAAAAxAAAAEAAAAQAQAAASAAAAIAAAAUAQAAACAAAAEAAAA6AQAA"
+    "ABAAAAEAAABMAQAA";
+
+// Standard interface. Use declared-synchronized again for 3B encoding.
+//
+// .class public interface LInterfaceMethodFlags;
+// .super Ljava/lang/Object;
+//
+// .method public static constructor <clinit>()V
+// .registers 1
+//     return-void
+// .end method
+//
+// .method public abstract declared-synchronized foo()V
+// .end method
+static const char kMethodFlagsInterface[] =
+    "ZGV4CjAzNQCOM0odZ5bws1d9GSmumXaK5iE/7XxFpOm8AQAAcAAAAHhWNBIAAAAAAAAAADQBAAAF"
+    "AAAAcAAAAAMAAACEAAAAAQAAAJAAAAAAAAAAAAAAAAIAAACcAAAAAQAAAKwAAADwAAAAzAAAAMwA"
+    "AADWAAAA7gAAAAIBAAAFAQAAAQAAAAIAAAADAAAAAwAAAAIAAAAAAAAAAAAAAAAAAAAAAAAABAAA"
+    "AAAAAAABAgAAAQAAAAAAAAD/////AAAAACIBAAAAAAAACDxjbGluaXQ+ABZMSW50ZXJmYWNlTWV0"
+    "aG9kRmxhZ3M7ABJMamF2YS9sYW5nL09iamVjdDsAAVYAA2ZvbwAAAAAAAAABAAAAAAAAAAAAAAAB"
+    "AAAADgAAAAEBAImABJACAYGICAAAAAALAAAAAAAAAAEAAAAAAAAAAQAAAAUAAABwAAAAAgAAAAMA"
+    "AACEAAAAAwAAAAEAAACQAAAABQAAAAIAAACcAAAABgAAAAEAAACsAAAAAiAAAAUAAADMAAAAAxAA"
+    "AAEAAAAMAQAAASAAAAEAAAAQAQAAACAAAAEAAAAiAQAAABAAAAEAAAA0AQAA";
+
+// To simplify generation of interesting "sub-states" of src_value, allow a "simple" mask to apply
+// to a src_value, such that mask bit 0 applies to the lowest set bit in src_value, and so on.
+static uint32_t ApplyMaskShifted(uint32_t src_value, uint32_t mask) {
+  uint32_t result = 0;
+  uint32_t mask_index = 0;
+  while (src_value != 0) {
+    uint32_t index = CTZ(src_value);
+    if (((src_value & (1 << index)) != 0) &&
+        ((mask & (1 << mask_index)) != 0)) {
+      result |= (1 << index);
+    }
+    src_value &= ~(1 << index);
+    mask_index++;
+  }
+  return result;
+}
+
+TEST_F(DexFileVerifierTest, MethodAccessFlagsInterfaces) {
+  // Reject interface with <init>.
+  VerifyModification(
+      kMethodFlagsInterfaceWithInit,
+      "method_flags_interface_with_init",
+      [](DexFile* dex_file ATTRIBUTE_UNUSED) {},
+      "Non-clinit interface method 1 should not have code");
+
+  VerifyModification(
+      kMethodFlagsInterface,
+      "method_flags_interface_ok",
+      [](DexFile* dex_file) {
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+      },
+      nullptr);
+
+  VerifyModification(
+      kMethodFlagsInterface,
+      "method_flags_interface_non_public",
+      [](DexFile* dex_file) {
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccPublic);
+      },
+      "Interface method 1 is not public and abstract");
+  VerifyModification(
+      kMethodFlagsInterface,
+      "method_flags_interface_non_abstract",
+      [](DexFile* dex_file) {
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccAbstract);
+      },
+      "Method 1 has no code, but is not marked native or abstract");
+
+  VerifyModification(
+      kMethodFlagsInterface,
+      "method_flags_interface_static",
+      [](DexFile* dex_file) {
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+        OrMaskToMethodFlags(dex_file, "foo", kAccStatic);
+      },
+      "Direct/virtual method 1 not in expected list 0");
+  VerifyModification(
+      kMethodFlagsInterface,
+      "method_flags_interface_private",
+      [](DexFile* dex_file) {
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccPublic);
+        OrMaskToMethodFlags(dex_file, "foo", kAccPrivate);
+      },
+      "Direct/virtual method 1 not in expected list 0");
+
+  VerifyModification(
+      kMethodFlagsInterface,
+      "method_flags_interface_non_public",
+      [](DexFile* dex_file) {
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccPublic);
+      },
+      "Interface method 1 is not public and abstract");
+  VerifyModification(
+      kMethodFlagsInterface,
+      "method_flags_interface_protected",
+      [](DexFile* dex_file) {
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToMethodFlags(dex_file, "foo", ~kAccPublic);
+        OrMaskToMethodFlags(dex_file, "foo", kAccProtected);
+      },
+      "Interface method 1 is not public and abstract");
+
+  constexpr uint32_t kAllMethodFlags =
+      kAccPublic |
+      kAccPrivate |
+      kAccProtected |
+      kAccStatic |
+      kAccFinal |
+      kAccSynchronized |
+      kAccBridge |
+      kAccVarargs |
+      kAccNative |
+      kAccAbstract |
+      kAccStrict |
+      kAccSynthetic;
+  constexpr uint32_t kInterfaceMethodFlags =
+      kAccPublic | kAccAbstract | kAccVarargs | kAccBridge | kAccSynthetic;
+  constexpr uint32_t kInterfaceDisallowed = kAllMethodFlags &
+                                            ~kInterfaceMethodFlags &
+                                            // Already tested, needed to be separate.
+                                            ~kAccStatic &
+                                            ~kAccPrivate &
+                                            ~kAccProtected;
+  static_assert(kInterfaceDisallowed != 0, "There should be disallowed flags.");
+
+  uint32_t bits = POPCOUNT(kInterfaceDisallowed);
+  for (uint32_t i = 1; i < (1u << bits); ++i) {
+    VerifyModification(
+        kMethodFlagsInterface,
+        "method_flags_interface_non_abstract",
+        [&](DexFile* dex_file) {
+          ApplyMaskToMethodFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+          uint32_t mask = ApplyMaskShifted(kInterfaceDisallowed, i);
+          if ((mask & kAccProtected) != 0) {
+            mask &= ~kAccProtected;
+            ApplyMaskToMethodFlags(dex_file, "foo", ~kAccPublic);
+          }
+          OrMaskToMethodFlags(dex_file, "foo", mask);
+        },
+        "Abstract method 1 has disallowed access flags");
+  }
+}
+
+///////////////////////////////////////////////////////////////////
+
+// Field flags.
+
+// Find the method data for the first method with the given name (from class 0). Note: the pointer
+// is to the access flags, so that the caller doesn't have to handle the leb128-encoded method-index
+// delta.
+static const uint8_t* FindFieldData(const DexFile* dex_file, const char* name) {
+  const DexFile::ClassDef& class_def = dex_file->GetClassDef(0);
+  const uint8_t* class_data = dex_file->GetClassData(class_def);
+
+  ClassDataItemIterator it(*dex_file, class_data);
+
+  const uint8_t* trailing = class_data;
+  // Need to manually decode the four entries. DataPointer() doesn't work for this, as the first
+  // element has already been loaded into the iterator.
+  DecodeUnsignedLeb128(&trailing);
+  DecodeUnsignedLeb128(&trailing);
+  DecodeUnsignedLeb128(&trailing);
+  DecodeUnsignedLeb128(&trailing);
+
+  while (it.HasNextStaticField() || it.HasNextInstanceField()) {
+    uint32_t field_index = it.GetMemberIndex();
+    uint32_t name_index = dex_file->GetFieldId(field_index).name_idx_;
+    const DexFile::StringId& string_id = dex_file->GetStringId(name_index);
+    const char* str = dex_file->GetStringData(string_id);
+    if (strcmp(name, str) == 0) {
+      DecodeUnsignedLeb128(&trailing);
+      return trailing;
+    }
+
+    trailing = it.DataPointer();
+    it.Next();
+  }
+
+  return nullptr;
+}
+
+// Set the method flags to the given value.
+static void SetFieldFlags(DexFile* dex_file, const char* field, uint32_t mask) {
+  uint8_t* field_flags_ptr = const_cast<uint8_t*>(FindFieldData(dex_file, field));
+  CHECK(field_flags_ptr != nullptr) << field;
+
+    // Unroll this, as we only have three bytes, anyways.
+  uint8_t base1 = static_cast<uint8_t>(mask & 0x7F);
+  *(field_flags_ptr++) = (base1 | 0x80);
+  mask >>= 7;
+
+  uint8_t base2 = static_cast<uint8_t>(mask & 0x7F);
+  *(field_flags_ptr++) = (base2 | 0x80);
+  mask >>= 7;
+
+  uint8_t base3 = static_cast<uint8_t>(mask & 0x7F);
+  *field_flags_ptr = base3;
+}
+
+static uint32_t GetFieldFlags(DexFile* dex_file, const char* field) {
+  const uint8_t* field_flags_ptr = const_cast<uint8_t*>(FindFieldData(dex_file, field));
+  CHECK(field_flags_ptr != nullptr) << field;
+  return DecodeUnsignedLeb128(&field_flags_ptr);
+}
+
+// Apply the given mask to method flags.
+static void ApplyMaskToFieldFlags(DexFile* dex_file, const char* field, uint32_t mask) {
+  uint32_t value = GetFieldFlags(dex_file, field);
+  value &= mask;
+  SetFieldFlags(dex_file, field, value);
+}
+
+// Apply the given mask to method flags.
+static void OrMaskToFieldFlags(DexFile* dex_file, const char* field, uint32_t mask) {
+  uint32_t value = GetFieldFlags(dex_file, field);
+  value |= mask;
+  SetFieldFlags(dex_file, field, value);
+}
+
+// Standard class. Use declared-synchronized again for 3B encoding.
+//
+// .class public LFieldFlags;
+// .super Ljava/lang/Object;
+//
+// .field declared-synchronized public foo:I
+//
+// .field declared-synchronized public static bar:I
+
+static const char kFieldFlagsTestDex[] =
+    "ZGV4CjAzNQBtLw7hydbfv4TdXidZyzAB70W7w3vnYJRwAQAAcAAAAHhWNBIAAAAAAAAAAAABAAAF"
+    "AAAAcAAAAAMAAACEAAAAAAAAAAAAAAACAAAAkAAAAAAAAAAAAAAAAQAAAKAAAACwAAAAwAAAAMAA"
+    "AADDAAAA0QAAAOUAAADqAAAAAAAAAAEAAAACAAAAAQAAAAMAAAABAAAABAAAAAEAAAABAAAAAgAA"
+    "AAAAAAD/////AAAAAPQAAAAAAAAAAUkADExGaWVsZEZsYWdzOwASTGphdmEvbGFuZy9PYmplY3Q7"
+    "AANiYXIAA2ZvbwAAAAAAAAEBAAAAiYAIAYGACAkAAAAAAAAAAQAAAAAAAAABAAAABQAAAHAAAAAC"
+    "AAAAAwAAAIQAAAAEAAAAAgAAAJAAAAAGAAAAAQAAAKAAAAACIAAABQAAAMAAAAADEAAAAQAAAPAA"
+    "AAAAIAAAAQAAAPQAAAAAEAAAAQAAAAABAAA=";
+
+TEST_F(DexFileVerifierTest, FieldAccessFlagsBase) {
+  // Check that it's OK when the wrong declared-synchronized flag is removed from "foo."
+  VerifyModification(
+      kFieldFlagsTestDex,
+      "field_flags_ok",
+      [](DexFile* dex_file) {
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+        ApplyMaskToFieldFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+      },
+      nullptr);
+}
+
+TEST_F(DexFileVerifierTest, FieldAccessFlagsWrongList) {
+  // Mark the field so that it should appear in the opposite list (instance vs static).
+  VerifyModification(
+      kFieldFlagsTestDex,
+      "field_flags_wrong_list",
+      [](DexFile* dex_file) {
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+        ApplyMaskToFieldFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+        OrMaskToFieldFlags(dex_file, "foo", kAccStatic);
+      },
+      "Static/instance field not in expected list");
+  VerifyModification(
+      kFieldFlagsTestDex,
+      "field_flags_wrong_list",
+      [](DexFile* dex_file) {
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+        ApplyMaskToFieldFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToFieldFlags(dex_file, "bar", ~kAccStatic);
+      },
+      "Static/instance field not in expected list");
+}
+
+TEST_F(DexFileVerifierTest, FieldAccessFlagsPPP) {
+  static const char* kFields[] = { "foo", "bar" };
+  for (size_t i = 0; i < arraysize(kFields); ++i) {
+    // Should be OK to remove public.
+    VerifyModification(
+        kFieldFlagsTestDex,
+        "field_flags_non_public",
+        [&](DexFile* dex_file) {
+          ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToFieldFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          ApplyMaskToFieldFlags(dex_file, kFields[i], ~kAccPublic);
+        },
+        nullptr);
+    constexpr uint32_t kAccFlags = kAccPublic | kAccPrivate | kAccProtected;
+    uint32_t bits = POPCOUNT(kAccFlags);
+    for (uint32_t j = 1; j < (1u << bits); ++j) {
+      if (POPCOUNT(j) < 2) {
+        continue;
+      }
+      VerifyModification(
+           kFieldFlagsTestDex,
+           "field_flags_ppp",
+           [&](DexFile* dex_file) {
+             ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+             ApplyMaskToFieldFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+             ApplyMaskToFieldFlags(dex_file, kFields[i], ~kAccPublic);
+             uint32_t mask = ApplyMaskShifted(kAccFlags, j);
+             OrMaskToFieldFlags(dex_file, kFields[i], mask);
+           },
+           "Field may have only one of public/protected/private");
+    }
+  }
+}
+
+TEST_F(DexFileVerifierTest, FieldAccessFlagsIgnoredOK) {
+  constexpr const char* kFields[] = { "foo", "bar"};
+  for (size_t i = 0; i < arraysize(kFields); ++i) {
+    // All interesting method flags, other flags are to be ignored.
+    constexpr uint32_t kAllFieldFlags =
+        kAccPublic |
+        kAccPrivate |
+        kAccProtected |
+        kAccStatic |
+        kAccFinal |
+        kAccVolatile |
+        kAccTransient |
+        kAccSynthetic |
+        kAccEnum;
+    constexpr uint32_t kIgnoredMask = ~kAllFieldFlags & 0xFFFF;
+    VerifyModification(
+        kFieldFlagsTestDex,
+        "field_flags_ignored",
+        [&](DexFile* dex_file) {
+          ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToFieldFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          OrMaskToFieldFlags(dex_file, kFields[i], kIgnoredMask);
+        },
+        nullptr);
+  }
+}
+
+TEST_F(DexFileVerifierTest, FieldAccessFlagsVolatileFinal) {
+  constexpr const char* kFields[] = { "foo", "bar"};
+  for (size_t i = 0; i < arraysize(kFields); ++i) {
+    VerifyModification(
+        kFieldFlagsTestDex,
+        "field_flags_final_and_volatile",
+        [&](DexFile* dex_file) {
+          ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+          ApplyMaskToFieldFlags(dex_file, "bar", ~kAccDeclaredSynchronized);
+
+          OrMaskToFieldFlags(dex_file, kFields[i], kAccVolatile | kAccFinal);
+        },
+        "Fields may not be volatile and final");
   }
 }
 
+// Standard interface. Needs to be separate from class as interfaces do not allow instance fields.
+// Use declared-synchronized again for 3B encoding.
+//
+// .class public interface LInterfaceFieldFlags;
+// .super Ljava/lang/Object;
+//
+// .field declared-synchronized public static final foo:I
+
+static const char kFieldFlagsInterfaceTestDex[] =
+    "ZGV4CjAzNQCVMHfEimR1zZPk6hl6O9GPAYqkl3u0umFkAQAAcAAAAHhWNBIAAAAAAAAAAPQAAAAE"
+    "AAAAcAAAAAMAAACAAAAAAAAAAAAAAAABAAAAjAAAAAAAAAAAAAAAAQAAAJQAAACwAAAAtAAAALQA"
+    "AAC3AAAAzgAAAOIAAAAAAAAAAQAAAAIAAAABAAAAAwAAAAEAAAABAgAAAgAAAAAAAAD/////AAAA"
+    "AOwAAAAAAAAAAUkAFUxJbnRlcmZhY2VGaWVsZEZsYWdzOwASTGphdmEvbGFuZy9PYmplY3Q7AANm"
+    "b28AAAAAAAABAAAAAJmACAkAAAAAAAAAAQAAAAAAAAABAAAABAAAAHAAAAACAAAAAwAAAIAAAAAE"
+    "AAAAAQAAAIwAAAAGAAAAAQAAAJQAAAACIAAABAAAALQAAAADEAAAAQAAAOgAAAAAIAAAAQAAAOwA"
+    "AAAAEAAAAQAAAPQAAAA=";
+
+TEST_F(DexFileVerifierTest, FieldAccessFlagsInterface) {
+  VerifyModification(
+      kFieldFlagsInterfaceTestDex,
+      "field_flags_interface",
+      [](DexFile* dex_file) {
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+      },
+      nullptr);
+
+  VerifyModification(
+      kFieldFlagsInterfaceTestDex,
+      "field_flags_interface_non_public",
+      [](DexFile* dex_file) {
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccPublic);
+      },
+      "Interface field is not public final static");
+  VerifyModification(
+      kFieldFlagsInterfaceTestDex,
+      "field_flags_interface_non_final",
+      [](DexFile* dex_file) {
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccFinal);
+      },
+      "Interface field is not public final static");
+  VerifyModification(
+      kFieldFlagsInterfaceTestDex,
+      "field_flags_interface_protected",
+      [](DexFile* dex_file) {
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccPublic);
+        OrMaskToFieldFlags(dex_file, "foo", kAccProtected);
+      },
+      "Interface field is not public final static");
+  VerifyModification(
+      kFieldFlagsInterfaceTestDex,
+      "field_flags_interface_private",
+      [](DexFile* dex_file) {
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccPublic);
+        OrMaskToFieldFlags(dex_file, "foo", kAccPrivate);
+      },
+      "Interface field is not public final static");
+
+  VerifyModification(
+      kFieldFlagsInterfaceTestDex,
+      "field_flags_interface_synthetic",
+      [](DexFile* dex_file) {
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+        OrMaskToFieldFlags(dex_file, "foo", kAccSynthetic);
+      },
+      nullptr);
+
+  constexpr uint32_t kAllFieldFlags =
+      kAccPublic |
+      kAccPrivate |
+      kAccProtected |
+      kAccStatic |
+      kAccFinal |
+      kAccVolatile |
+      kAccTransient |
+      kAccSynthetic |
+      kAccEnum;
+  constexpr uint32_t kInterfaceFieldFlags = kAccPublic | kAccStatic | kAccFinal | kAccSynthetic;
+  constexpr uint32_t kInterfaceDisallowed = kAllFieldFlags &
+                                            ~kInterfaceFieldFlags &
+                                            ~kAccProtected &
+                                            ~kAccPrivate;
+  static_assert(kInterfaceDisallowed != 0, "There should be disallowed flags.");
+
+  uint32_t bits = POPCOUNT(kInterfaceDisallowed);
+  for (uint32_t i = 1; i < (1u << bits); ++i) {
+    VerifyModification(
+        kFieldFlagsInterfaceTestDex,
+        "field_flags_interface_disallowed",
+        [&](DexFile* dex_file) {
+          ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+
+          uint32_t mask = ApplyMaskShifted(kInterfaceDisallowed, i);
+          if ((mask & kAccProtected) != 0) {
+            mask &= ~kAccProtected;
+            ApplyMaskToFieldFlags(dex_file, "foo", ~kAccPublic);
+          }
+          OrMaskToFieldFlags(dex_file, "foo", mask);
+        },
+        "Interface field has disallowed flag");
+  }
+}
+
+// Standard bad interface. Needs to be separate from class as interfaces do not allow instance
+// fields. Use declared-synchronized again for 3B encoding.
+//
+// .class public interface LInterfaceFieldFlags;
+// .super Ljava/lang/Object;
+//
+// .field declared-synchronized public final foo:I
+
+static const char kFieldFlagsInterfaceBadTestDex[] =
+    "ZGV4CjAzNQByMUnqYKHBkUpvvNp+9CnZ2VyDkKnRN6VkAQAAcAAAAHhWNBIAAAAAAAAAAPQAAAAE"
+    "AAAAcAAAAAMAAACAAAAAAAAAAAAAAAABAAAAjAAAAAAAAAAAAAAAAQAAAJQAAACwAAAAtAAAALQA"
+    "AAC3AAAAzgAAAOIAAAAAAAAAAQAAAAIAAAABAAAAAwAAAAEAAAABAgAAAgAAAAAAAAD/////AAAA"
+    "AOwAAAAAAAAAAUkAFUxJbnRlcmZhY2VGaWVsZEZsYWdzOwASTGphdmEvbGFuZy9PYmplY3Q7AANm"
+    "b28AAAAAAAAAAQAAAJGACAkAAAAAAAAAAQAAAAAAAAABAAAABAAAAHAAAAACAAAAAwAAAIAAAAAE"
+    "AAAAAQAAAIwAAAAGAAAAAQAAAJQAAAACIAAABAAAALQAAAADEAAAAQAAAOgAAAAAIAAAAQAAAOwA"
+    "AAAAEAAAAQAAAPQAAAA=";
+
+TEST_F(DexFileVerifierTest, FieldAccessFlagsInterfaceNonStatic) {
+  VerifyModification(
+      kFieldFlagsInterfaceBadTestDex,
+      "field_flags_interface_non_static",
+      [](DexFile* dex_file) {
+        ApplyMaskToFieldFlags(dex_file, "foo", ~kAccDeclaredSynchronized);
+      },
+      "Interface field is not public final static");
+}
+
 // Generated from:
 //
 // .class public LTest;
@@ -305,15 +1275,14 @@ TEST_F(DexFileVerifierTest, DebugInfoTypeIdxTest) {
     ASSERT_TRUE(raw.get() != nullptr) << error_msg;
   }
 
-  {
-    // Modify the debug information entry.
-    ScratchFile tmp;
-    std::string error_msg;
-    bool success = !ModifyAndLoad(kDebugInfoTestDex, tmp.GetFilename().c_str(), 416, 0x14U,
-                                  &error_msg);
-    ASSERT_TRUE(success);
-    ASSERT_NE(error_msg.find("DBG_START_LOCAL type_idx"), std::string::npos) << error_msg;
-  }
+  // Modify the debug information entry.
+  VerifyModification(
+      kDebugInfoTestDex,
+      "debug_start_type_idx",
+      [](DexFile* dex_file) {
+        *(const_cast<uint8_t*>(dex_file->Begin()) + 416) = 0x14U;
+      },
+      "DBG_START_LOCAL type_idx");
 }
 
 }  // namespace art
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index d768afda54..1ed6980c96 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -424,6 +424,7 @@ MethodVerifier::MethodVerifier(Thread* self,
       has_virtual_or_interface_invokes_(false),
       verify_to_dump_(verify_to_dump),
       allow_thread_suspension_(allow_thread_suspension),
+      is_constructor_(false),
       link_(nullptr) {
   self->PushVerifier(this);
   DCHECK(class_def != nullptr);
@@ -555,15 +556,124 @@ SafeMap<uint32_t, std::set<uint32_t>>& MethodVerifier::FindStringInitMap() {
 }
 
 bool MethodVerifier::Verify() {
+  // Some older code doesn't correctly mark constructors as such. Test for this case by looking at
+  // the name.
+  const DexFile::MethodId& method_id = dex_file_->GetMethodId(dex_method_idx_);
+  const char* method_name = dex_file_->StringDataByIdx(method_id.name_idx_);
+  bool instance_constructor_by_name = strcmp("<init>", method_name) == 0;
+  bool static_constructor_by_name = strcmp("<clinit>", method_name) == 0;
+  bool constructor_by_name = instance_constructor_by_name || static_constructor_by_name;
+  // Check that only constructors are tagged, and check for bad code that doesn't tag constructors.
+  if ((method_access_flags_ & kAccConstructor) != 0) {
+    if (!constructor_by_name) {
+      Fail(VERIFY_ERROR_BAD_CLASS_HARD)
+            << "method is marked as constructor, but not named accordingly";
+      return false;
+    }
+    is_constructor_ = true;
+  } else if (constructor_by_name) {
+    LOG(WARNING) << "Method " << PrettyMethod(dex_method_idx_, *dex_file_)
+                 << " not marked as constructor.";
+    is_constructor_ = true;
+  }
+  // If it's a constructor, check whether IsStatic() matches the name.
+  // This should have been rejected by the dex file verifier. Only do in debug build.
+  if (kIsDebugBuild) {
+    if (IsConstructor()) {
+      if (IsStatic() ^ static_constructor_by_name) {
+        Fail(VERIFY_ERROR_BAD_CLASS_HARD)
+              << "constructor name doesn't match static flag";
+        return false;
+      }
+    }
+  }
+
+  // Methods may only have one of public/protected/private.
+  // This should have been rejected by the dex file verifier. Only do in debug build.
+  if (kIsDebugBuild) {
+    size_t access_mod_count =
+        (((method_access_flags_ & kAccPublic) == 0) ? 0 : 1) +
+        (((method_access_flags_ & kAccProtected) == 0) ? 0 : 1) +
+        (((method_access_flags_ & kAccPrivate) == 0) ? 0 : 1);
+    if (access_mod_count > 1) {
+      Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "method has more than one of public/protected/private";
+      return false;
+    }
+  }
+
   // If there aren't any instructions, make sure that's expected, then exit successfully.
   if (code_item_ == nullptr) {
-    if ((method_access_flags_ & (kAccNative | kAccAbstract)) == 0) {
-      Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "zero-length code in concrete non-native method";
+    // This should have been rejected by the dex file verifier. Only do in debug build.
+    if (kIsDebugBuild) {
+      // Only native or abstract methods may not have code.
+      if ((method_access_flags_ & (kAccNative | kAccAbstract)) == 0) {
+        Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "zero-length code in concrete non-native method";
+        return false;
+      }
+      if ((method_access_flags_ & kAccAbstract) != 0) {
+        // Abstract methods are not allowed to have the following flags.
+        static constexpr uint32_t kForbidden =
+            kAccPrivate |
+            kAccStatic |
+            kAccFinal |
+            kAccNative |
+            kAccStrict |
+            kAccSynchronized;
+        if ((method_access_flags_ & kForbidden) != 0) {
+          Fail(VERIFY_ERROR_BAD_CLASS_HARD)
+                << "method can't be abstract and private/static/final/native/strict/synchronized";
+          return false;
+        }
+      }
+      if ((class_def_->GetJavaAccessFlags() & kAccInterface) != 0) {
+        // Interface methods must be public and abstract.
+        if ((method_access_flags_ & (kAccPublic | kAccAbstract)) != (kAccPublic | kAccAbstract)) {
+          Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "interface methods must be public and abstract";
+          return false;
+        }
+        // In addition to the above, interface methods must not be protected.
+        static constexpr uint32_t kForbidden = kAccProtected;
+        if ((method_access_flags_ & kForbidden) != 0) {
+          Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "interface methods can't be protected";
+          return false;
+        }
+      }
+      // We also don't allow constructors to be abstract or native.
+      if (IsConstructor()) {
+        Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "constructors can't be abstract or native";
+        return false;
+      }
+    }
+    return true;
+  }
+
+  // This should have been rejected by the dex file verifier. Only do in debug build.
+  if (kIsDebugBuild) {
+    // When there's code, the method must not be native or abstract.
+    if ((method_access_flags_ & (kAccNative | kAccAbstract)) != 0) {
+      Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "non-zero-length code in abstract or native method";
       return false;
-    } else {
-      return true;
+    }
+
+    // Only the static initializer may have code in an interface.
+    if ((class_def_->GetJavaAccessFlags() & kAccInterface) != 0) {
+      // Interfaces may have static initializers for their fields.
+      if (!IsConstructor() || !IsStatic()) {
+        Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "interface methods must be abstract";
+        return false;
+      }
+    }
+
+    // Instance constructors must not be synchronized.
+    if (IsInstanceConstructor()) {
+      static constexpr uint32_t kForbidden = kAccSynchronized;
+      if ((method_access_flags_ & kForbidden) != 0) {
+        Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "constructors can't be synchronized";
+        return false;
+      }
     }
   }
+
   // Sanity-check the register counts. ins + locals = registers, so make sure that ins <= registers.
   if (code_item_->ins_size_ > code_item_->registers_size_) {
     Fail(VERIFY_ERROR_BAD_CLASS_HARD) << "bad register counts (ins=" << code_item_->ins_size_
diff --git a/runtime/verifier/method_verifier.h b/runtime/verifier/method_verifier.h
index b57abf537d..5e661a59b8 100644
--- a/runtime/verifier/method_verifier.h
+++ b/runtime/verifier/method_verifier.h
@@ -262,29 +262,29 @@ class MethodVerifier {
   ArtField* GetQuickFieldAccess(const Instruction* inst, RegisterLine* reg_line)
       SHARED_REQUIRES(Locks::mutator_lock_);
 
-  // Is the method being verified a constructor?
-  bool IsConstructor() const {
-    return (method_access_flags_ & kAccConstructor) != 0;
+  SafeMap<uint32_t, std::set<uint32_t>>& GetStringInitPcRegMap() {
+    return string_init_pc_reg_map_;
   }
 
-  // Is the method verified static?
-  bool IsStatic() const {
-    return (method_access_flags_ & kAccStatic) != 0;
+  uint32_t GetEncounteredFailureTypes() {
+    return encountered_failure_types_;
   }
 
   bool IsInstanceConstructor() const {
     return IsConstructor() && !IsStatic();
   }
 
-  SafeMap<uint32_t, std::set<uint32_t>>& GetStringInitPcRegMap() {
-    return string_init_pc_reg_map_;
+ private:
+  // Is the method being verified a constructor? See the comment on the field.
+  bool IsConstructor() const {
+    return is_constructor_;
   }
 
-  uint32_t GetEncounteredFailureTypes() {
-    return encountered_failure_types_;
+  // Is the method verified static?
+  bool IsStatic() const {
+    return (method_access_flags_ & kAccStatic) != 0;
   }
 
- private:
   // Private constructor for dumping.
   MethodVerifier(Thread* self, const DexFile* dex_file, Handle<mirror::DexCache> dex_cache,
                  Handle<mirror::ClassLoader> class_loader, const DexFile::ClassDef* class_def,
@@ -780,6 +780,13 @@ class MethodVerifier {
   // FindLocksAtDexPC, resulting in deadlocks.
   const bool allow_thread_suspension_;
 
+  // Whether the method seems to be a constructor. Note that this field exists as we can't trust
+  // the flags in the dex file. Some older code does not mark methods named "<init>" and "<clinit>"
+  // correctly.
+  //
+  // Note: this flag is only valid once Verify() has started.
+  bool is_constructor_;
+
   // Link, for the method verifier root linked list.
   MethodVerifier* link_;
 
diff --git a/test/800-smali/smali/b_18380491AbstractBase.smali b/test/800-smali/smali/b_18380491AbstractBase.smali
index 7aa1b1a12e..cc05221cdf 100644
--- a/test/800-smali/smali/b_18380491AbstractBase.smali
+++ b/test/800-smali/smali/b_18380491AbstractBase.smali
@@ -1,4 +1,4 @@
-.class public LB18380491ActractBase;
+.class public abstract LB18380491AbstractBase;
 
 .super Ljava/lang/Object;
 
diff --git a/test/800-smali/smali/b_18380491ConcreteClass.smali b/test/800-smali/smali/b_18380491ConcreteClass.smali
index db5ef3ba6b..1ba684f796 100644
--- a/test/800-smali/smali/b_18380491ConcreteClass.smali
+++ b/test/800-smali/smali/b_18380491ConcreteClass.smali
@@ -1,10 +1,10 @@
 .class public LB18380491ConcreteClass;
 
-.super LB18380491ActractBase;
+.super LB18380491AbstractBase;
 
 .method public constructor <init>()V
     .locals 0
-    invoke-direct {p0}, LB18380491ActractBase;-><init>()V
+    invoke-direct {p0}, LB18380491AbstractBase;-><init>()V
     return-void
 .end method
 
@@ -13,7 +13,7 @@
   if-eqz p1, :invoke_super_abstract
   return p1
   :invoke_super_abstract
-  invoke-super {p0, p1}, LB18380491ActractBase;->foo(I)I
+  invoke-super {p0, p1}, LB18380491AbstractBase;->foo(I)I
   move-result v0
   return v0
 .end method