ART: Move access flags checking to dex file verifier
Actually implement all the access flags checking in the dex file
verifier. Add tests.
Change-Id: I8b797357831b588589d56d6e2e22f7b410f33008
diff --git a/runtime/dex_file.h b/runtime/dex_file.h
index 98d4e59..47e5c12 100644
--- a/runtime/dex_file.h
+++ b/runtime/dex_file.h
@@ -1275,6 +1275,8 @@
// 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 @@
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 eec4983..2dd96aa 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 @@
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,60 +711,185 @@
return true;
}
+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);
+
+ // First check if the index is valid.
+ if (index >= (is_field ? header_->field_ids_size_ : header_->method_ids_size_)) {
+ 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;
+ }
+ }
+
+ // 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;
+ }
+ }
+ 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 (!CheckClassDataItemField(curr_index,
+ it->GetRawMemberAccessFlags(),
+ *class_access_flags,
+ *class_type_index,
+ kStatic)) {
+ return false;
+ }
+ }
+
+ 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(),
+ *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;
- // 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;
- }
+ // 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;
}
- 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;
- }
- prev_index = curr_index;
- if (!CheckClassDataItemField(curr_index, it.GetRawMemberAccessFlags(), false)) {
- return false;
- }
+ if (!CheckIntraClassDataItemFields<false>(&it,
+ &have_class,
+ &class_type_index,
+ &class_access_flags)) {
+ 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 false;
- }
- prev_index = curr_index;
- if (!CheckClassDataItemMethod(curr_index, it.GetRawMemberAccessFlags(),
- it.GetMethodCodeItemOffset(), direct_method_indexes, true)) {
- return false;
- }
+
+ // Check methods.
+ if (!CheckIntraClassDataItemMethods<true>(&it,
+ &direct_method_indexes,
+ &have_class,
+ &class_type_index,
+ &class_access_flags)) {
+ 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 false;
- }
- prev_index = curr_index;
- if (!CheckClassDataItemMethod(curr_index, it.GetRawMemberAccessFlags(),
- it.GetMethodCodeItemOffset(), direct_method_indexes, false)) {
- return false;
- }
+ if (!CheckIntraClassDataItemMethods<false>(&it,
+ &direct_method_indexes,
+ &have_class,
+ &class_type_index,
+ &class_access_flags)) {
+ return false;
}
ptr_ = it.EndDataPointer();
@@ -2149,4 +2296,259 @@
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 ccc40d4..c964b79 100644
--- a/runtime/dex_file_verifier.h
+++ b/runtime/dex_file_verifier.h
@@ -57,16 +57,48 @@
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 @@
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 9f1ffec..1b529c9 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 @@
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 @@
return dex_file;
}
-
// For reference.
static const char kGoodTestDex[] =
"ZGV4CjAzNQDrVbyVkxX1HljTznNf95AglkUAhQuFtmKkAgAAcAAAAHhWNBIAAAAAAAAAAAQCAAAN"
@@ -157,92 +216,1003 @@
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;
-}
-
-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);
-
- // Fixup of checksum.
- FixUpChecksum(bytes);
-
- // 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";
- }
- if (file->FlushCloseOrErase() != 0) {
- PLOG(FATAL) << "Could not flush and close test file.";
- }
- 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;
- }
- 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;
-}
-
-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);
-
- // Make modifications.
- dex_bytes.get()[offset] = new_val;
-
- // Fixup and load.
- std::unique_ptr<const DexFile> file(FixChecksumAndOpen(dex_bytes.get(), length, location,
- error_msg));
- return file.get() != nullptr;
-}
-
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;
+ // 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");
+}
+
+// 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
+
+static const char kMethodFlagsTestDex[] =
+ "ZGV4CjAzNQCyOQrJaDBwiIWv5MIuYKXhxlLLsQcx5SwgAgAAcAAAAHhWNBIAAAAAAAAAAJgBAAAH"
+ "AAAAcAAAAAMAAACMAAAAAQAAAJgAAAAAAAAAAAAAAAQAAACkAAAAAQAAAMQAAAA8AQAA5AAAAOQA"
+ "AADuAAAA9gAAAAUBAAAZAQAAHAEAACEBAAACAAAAAwAAAAQAAAAEAAAAAgAAAAAAAAAAAAAAAAAA"
+ "AAAAAAABAAAAAAAAAAUAAAAAAAAABgAAAAAAAAABAAAAAQAAAAAAAAD/////AAAAAHoBAAAAAAAA"
+ "CDxjbGluaXQ+AAY8aW5pdD4ADUxNZXRob2RGbGFnczsAEkxqYXZhL2xhbmcvT2JqZWN0OwABVgAD"
+ "YmFyAANmb28AAAAAAAAAAQAAAAAAAAAAAAAAAQAAAA4AAAABAAEAAAAAAAAAAAABAAAADgAAAAEA"
+ "AQAAAAAAAAAAAAEAAAAOAAAAAQABAAAAAAAAAAAAAQAAAA4AAAADAQCJgASsAgGBgATAAgKCgAjU"
+ "AgKBgAjoAgAACwAAAAAAAAABAAAAAAAAAAEAAAAHAAAAcAAAAAIAAAADAAAAjAAAAAMAAAABAAAA"
+ "mAAAAAUAAAAEAAAApAAAAAYAAAABAAAAxAAAAAIgAAAHAAAA5AAAAAMQAAABAAAAKAEAAAEgAAAE"
+ "AAAALAEAAAAgAAABAAAAegEAAAAQAAABAAAAmAEAAA==";
+
+// 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();
}
- {
- // 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;
+ 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();
}
- {
- // 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;
+ 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;
}
+ *mod = 0x00;
+}
+
+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);
+
+ 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);
+
+ 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, 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");
+ }
+ }
+}
+
+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);
+ }
+}
+
+// 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:
@@ -305,15 +1275,14 @@
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 d768afd..1ed6980 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -424,6 +424,7 @@
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 @@
}
bool MethodVerifier::Verify() {
- // 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";
+ // 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;
- } else {
- return true;
+ }
+ 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) {
+ // 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;
+ }
+
+ // 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 b57abf5..5e661a5 100644
--- a/runtime/verifier/method_verifier.h
+++ b/runtime/verifier/method_verifier.h
@@ -262,20 +262,6 @@
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;
- }
-
- // Is the method verified static?
- bool IsStatic() const {
- return (method_access_flags_ & kAccStatic) != 0;
- }
-
- bool IsInstanceConstructor() const {
- return IsConstructor() && !IsStatic();
- }
-
SafeMap<uint32_t, std::set<uint32_t>>& GetStringInitPcRegMap() {
return string_init_pc_reg_map_;
}
@@ -284,7 +270,21 @@
return encountered_failure_types_;
}
+ bool IsInstanceConstructor() const {
+ return IsConstructor() && !IsStatic();
+ }
+
private:
+ // Is the method being verified a constructor? See the comment on the field.
+ bool IsConstructor() const {
+ return is_constructor_;
+ }
+
+ // Is the method verified static?
+ bool IsStatic() const {
+ return (method_access_flags_ & kAccStatic) != 0;
+ }
+
// 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 @@
// 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 7aa1b1a..cc05221 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 db5ef3b..1ba684f 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
