ART: Give better error messages in dex-file verifier
Try to decode field and method names when an access-flag violation
has been found. This is not guaranteed to work, if the file is
broken enough.
Bug: 27064244
Bug: 27070841
Change-Id: Ie913076462e958d4f21b481631bc874cf6f67c0d
diff --git a/runtime/dex_file_verifier.cc b/runtime/dex_file_verifier.cc
index 7a852e2..ddf2749 100644
--- a/runtime/dex_file_verifier.cc
+++ b/runtime/dex_file_verifier.cc
@@ -478,7 +478,7 @@
// Check field access flags.
std::string error_msg;
- if (!CheckFieldAccessFlags(access_flags, class_access_flags, &error_msg)) {
+ if (!CheckFieldAccessFlags(idx, access_flags, class_access_flags, &error_msg)) {
ErrorStringPrintf("%s", error_msg.c_str());
return false;
}
@@ -2312,12 +2312,88 @@
return count <= 1;
}
-bool DexFileVerifier::CheckFieldAccessFlags(uint32_t field_access_flags,
+// Helper functions to retrieve names from the dex file. We do not want to rely on DexFile
+// functionality, as we're still verifying the dex file. begin and header correspond to the
+// underscored variants in the DexFileVerifier.
+
+static std::string GetStringOrError(const uint8_t* const begin,
+ const DexFile::Header* const header,
+ uint32_t string_idx) {
+ if (header->string_ids_size_ < string_idx) {
+ return "(error)";
+ }
+
+ const DexFile::StringId* string_id =
+ reinterpret_cast<const DexFile::StringId*>(begin + header->string_ids_off_) + string_idx;
+
+ // Assume that the data is OK at this point. String data has been checked at this point.
+
+ const uint8_t* ptr = begin + string_id->string_data_off_;
+ DecodeUnsignedLeb128(&ptr);
+ return reinterpret_cast<const char*>(ptr);
+}
+
+static std::string GetClassOrError(const uint8_t* const begin,
+ const DexFile::Header* const header,
+ uint32_t class_idx) {
+ if (header->type_ids_size_ < class_idx) {
+ return "(error)";
+ }
+
+ const DexFile::TypeId* type_id =
+ reinterpret_cast<const DexFile::TypeId*>(begin + header->type_ids_off_) + class_idx;
+
+ // Assume that the data is OK at this point. Type id offsets have been checked at this point.
+
+ return GetStringOrError(begin, header, type_id->descriptor_idx_);
+}
+
+static std::string GetFieldDescriptionOrError(const uint8_t* const begin,
+ const DexFile::Header* const header,
+ uint32_t idx) {
+ if (header->field_ids_size_ < idx) {
+ return "(error)";
+ }
+
+ const DexFile::FieldId* field_id =
+ reinterpret_cast<const DexFile::FieldId*>(begin + header->field_ids_off_) + idx;
+
+ // Assume that the data is OK at this point. Field id offsets have been checked at this point.
+
+ std::string class_name = GetClassOrError(begin, header, field_id->class_idx_);
+ std::string field_name = GetStringOrError(begin, header, field_id->name_idx_);
+
+ return class_name + "." + field_name;
+}
+
+static std::string GetMethodDescriptionOrError(const uint8_t* const begin,
+ const DexFile::Header* const header,
+ uint32_t idx) {
+ if (header->method_ids_size_ < idx) {
+ return "(error)";
+ }
+
+ const DexFile::MethodId* method_id =
+ reinterpret_cast<const DexFile::MethodId*>(begin + header->method_ids_off_) + idx;
+
+ // Assume that the data is OK at this point. Method id offsets have been checked at this point.
+
+ std::string class_name = GetClassOrError(begin, header, method_id->class_idx_);
+ std::string method_name = GetStringOrError(begin, header, method_id->name_idx_);
+
+ return class_name + "." + method_name;
+}
+
+bool DexFileVerifier::CheckFieldAccessFlags(uint32_t idx,
+ 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);
+ *error_msg = StringPrintf("Bad field access_flags for %s: %x(%s)",
+ GetFieldDescriptionOrError(begin_, header_, idx).c_str(),
+ field_access_flags,
+ PrettyJavaAccessFlags(field_access_flags).c_str());
return false;
}
@@ -2334,8 +2410,10 @@
// 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);
+ *error_msg = StringPrintf("Field may have only one of public/protected/private, %s: %x(%s)",
+ GetFieldDescriptionOrError(begin_, header_, idx).c_str(),
+ field_access_flags,
+ PrettyJavaAccessFlags(field_access_flags).c_str());
return false;
}
@@ -2344,14 +2422,19 @@
// 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);
+ *error_msg = StringPrintf("Interface field is not public final static, %s: %x(%s)",
+ GetFieldDescriptionOrError(begin_, header_, idx).c_str(),
+ field_access_flags,
+ PrettyJavaAccessFlags(field_access_flags).c_str());
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);
+ *error_msg = StringPrintf("Interface field has disallowed flag, %s: %x(%s)",
+ GetFieldDescriptionOrError(begin_, header_, idx).c_str(),
+ field_access_flags,
+ PrettyJavaAccessFlags(field_access_flags).c_str());
return false;
}
return true;
@@ -2360,7 +2443,8 @@
// 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";
+ *error_msg = StringPrintf("Fields may not be volatile and final: %s",
+ GetFieldDescriptionOrError(begin_, header_, idx).c_str());
return false;
}
@@ -2410,7 +2494,9 @@
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);
+ *error_msg = StringPrintf("Bad method access_flags for %s: %x",
+ GetMethodDescriptionOrError(begin_, header_, method_index).c_str(),
+ method_access_flags);
return false;
}
@@ -2430,7 +2516,8 @@
// 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",
+ *error_msg = StringPrintf("Method may have only one of public/protected/private, %s: %x",
+ GetMethodDescriptionOrError(begin_, header_, method_index).c_str(),
method_access_flags);
return false;
}
@@ -2456,8 +2543,10 @@
// 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);
+ *error_msg =
+ StringPrintf("Method %" PRIu32 "(%s) is marked constructor, but doesn't match name",
+ method_index,
+ GetMethodDescriptionOrError(begin_, header_, method_index).c_str());
return false;
}
// Check that the static constructor (= static initializer) is named "<clinit>" and that the
@@ -2465,8 +2554,9 @@
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);
+ *error_msg = StringPrintf("Constructor %" PRIu32 "(%s) is not flagged correctly wrt/ static.",
+ method_index,
+ GetMethodDescriptionOrError(begin_, header_, method_index).c_str());
return false;
}
}
@@ -2474,8 +2564,9 @@
// 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",
+ *error_msg = StringPrintf("Direct/virtual method %" PRIu32 "(%s) not in expected list %d",
method_index,
+ GetMethodDescriptionOrError(begin_, header_, method_index).c_str(),
expect_direct);
return false;
}
@@ -2488,14 +2579,17 @@
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 "
+ *error_msg = StringPrintf("Method %" PRIu32 "(%s) has no code, but is not marked native or "
"abstract",
- method_index);
+ method_index,
+ GetMethodDescriptionOrError(begin_, header_, method_index).c_str());
return false;
}
// Constructors must always have code.
if (is_constructor) {
- *error_msg = StringPrintf("Constructor %u must not be abstract or native", method_index);
+ *error_msg = StringPrintf("Constructor %u(%s) must not be abstract or native",
+ method_index,
+ GetMethodDescriptionOrError(begin_, header_, method_index).c_str());
return false;
}
if ((method_access_flags & kAccAbstract) != 0) {
@@ -2503,14 +2597,15 @@
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);
+ *error_msg = StringPrintf("Abstract method %" PRIu32 "(%s) has disallowed access flags %x",
+ method_index,
+ GetMethodDescriptionOrError(begin_, header_, method_index).c_str(),
+ method_access_flags);
return false;
}
// Abstract methods should be in an abstract class or interface.
if ((class_access_flags & (kAccInterface | kAccAbstract)) == 0) {
- LOG(WARNING) << "Method " << PrettyMethod(method_index, *dex_file_)
+ LOG(WARNING) << "Method " << GetMethodDescriptionOrError(begin_, header_, method_index)
<< " is abstract, but the declaring class is neither abstract nor an "
<< "interface in dex file "
<< dex_file_->GetLocation();
@@ -2520,8 +2615,9 @@
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);
+ *error_msg = StringPrintf("Interface method %" PRIu32 "(%s) is not public and abstract",
+ method_index,
+ GetMethodDescriptionOrError(begin_, header_, method_index).c_str());
return false;
}
// At this point, we know the method is public and abstract. This means that all the checks
@@ -2533,8 +2629,9 @@
// 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);
+ *error_msg = StringPrintf("Method %" PRIu32 "(%s) has code, but is marked native or abstract",
+ method_index,
+ GetMethodDescriptionOrError(begin_, header_, method_index).c_str());
return false;
}
@@ -2543,8 +2640,9 @@
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",
+ *error_msg = StringPrintf("Constructor %" PRIu32 "(%s) flagged inappropriately %x",
method_index,
+ GetMethodDescriptionOrError(begin_, header_, method_index).c_str(),
method_access_flags);
return false;
}
diff --git a/runtime/dex_file_verifier.h b/runtime/dex_file_verifier.h
index 6c63749..ddfeea2 100644
--- a/runtime/dex_file_verifier.h
+++ b/runtime/dex_file_verifier.h
@@ -157,9 +157,10 @@
// 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);
+ bool CheckFieldAccessFlags(uint32_t idx,
+ 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,
diff --git a/runtime/dex_file_verifier_test.cc b/runtime/dex_file_verifier_test.cc
index b67af53..558a6ed 100644
--- a/runtime/dex_file_verifier_test.cc
+++ b/runtime/dex_file_verifier_test.cc
@@ -527,7 +527,7 @@
ApplyMaskToMethodFlags(dex_file, "<init>", ~kAccPublic);
OrMaskToMethodFlags(dex_file, "<init>", kAccStatic);
},
- "Constructor 1 is not flagged correctly wrt/ static");
+ "Constructor 1(LMethodFlags;.<init>) is not flagged correctly wrt/ static");
static constexpr uint32_t kInitNotAllowed[] = {
kAccFinal,
kAccSynchronized,
@@ -544,7 +544,7 @@
ApplyMaskToMethodFlags(dex_file, "<init>", ~kAccPublic);
OrMaskToMethodFlags(dex_file, "<init>", kInitNotAllowed[i]);
},
- "Constructor 1 flagged inappropriately");
+ "Constructor 1(LMethodFlags;.<init>) flagged inappropriately");
}
}
@@ -742,7 +742,7 @@
ApplyMaskToMethodFlags(dex_file, "foo", ~kAccPublic);
},
- "Interface method 1 is not public and abstract");
+ "Interface method 1(LInterfaceMethodFlags;.foo) is not public and abstract");
VerifyModification(
kMethodFlagsInterface,
"method_flags_interface_non_abstract",
@@ -751,7 +751,7 @@
ApplyMaskToMethodFlags(dex_file, "foo", ~kAccAbstract);
},
- "Method 1 has no code, but is not marked native or abstract");
+ "Method 1(LInterfaceMethodFlags;.foo) has no code, but is not marked native or abstract");
VerifyModification(
kMethodFlagsInterface,
@@ -761,7 +761,7 @@
OrMaskToMethodFlags(dex_file, "foo", kAccStatic);
},
- "Direct/virtual method 1 not in expected list 0");
+ "Direct/virtual method 1(LInterfaceMethodFlags;.foo) not in expected list 0");
VerifyModification(
kMethodFlagsInterface,
"method_flags_interface_private",
@@ -771,7 +771,7 @@
ApplyMaskToMethodFlags(dex_file, "foo", ~kAccPublic);
OrMaskToMethodFlags(dex_file, "foo", kAccPrivate);
},
- "Direct/virtual method 1 not in expected list 0");
+ "Direct/virtual method 1(LInterfaceMethodFlags;.foo) not in expected list 0");
VerifyModification(
kMethodFlagsInterface,
@@ -781,7 +781,7 @@
ApplyMaskToMethodFlags(dex_file, "foo", ~kAccPublic);
},
- "Interface method 1 is not public and abstract");
+ "Interface method 1(LInterfaceMethodFlags;.foo) is not public and abstract");
VerifyModification(
kMethodFlagsInterface,
"method_flags_interface_protected",
@@ -791,7 +791,7 @@
ApplyMaskToMethodFlags(dex_file, "foo", ~kAccPublic);
OrMaskToMethodFlags(dex_file, "foo", kAccProtected);
},
- "Interface method 1 is not public and abstract");
+ "Interface method 1(LInterfaceMethodFlags;.foo) is not public and abstract");
constexpr uint32_t kAllMethodFlags =
kAccPublic |
@@ -831,7 +831,7 @@
}
OrMaskToMethodFlags(dex_file, "foo", mask);
},
- "Abstract method 1 has disallowed access flags");
+ "Abstract method 1(LInterfaceMethodFlags;.foo) has disallowed access flags");
}
}
