Merge "Fix index checks for error strings in DexFileVerifier." into nyc-dev
diff --git a/runtime/dex_file_verifier.cc b/runtime/dex_file_verifier.cc
index 7006033..c504074 100644
--- a/runtime/dex_file_verifier.cc
+++ b/runtime/dex_file_verifier.cc
@@ -2358,7 +2358,8 @@
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) {
+ // The `string_idx` is not guaranteed to be valid yet.
+ if (header->string_ids_size_ <= string_idx) {
return "(error)";
}
@@ -2375,9 +2376,11 @@
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)";
- }
+ // The `class_idx` is either `FieldId::class_idx_` or `MethodId::class_idx_` and
+ // it has already been checked in `DexFileVerifier::CheckClassDataItemField()`
+ // or `DexFileVerifier::CheckClassDataItemMethod()`, respectively, to match
+ // a valid defining class.
+ CHECK_LT(class_idx, header->type_ids_size_);
const DexFile::TypeId* type_id =
reinterpret_cast<const DexFile::TypeId*>(begin + header->type_ids_off_) + class_idx;
@@ -2390,9 +2393,8 @@
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)";
- }
+ // The `idx` has already been checked in `DexFileVerifier::CheckClassDataItemField()`.
+ CHECK_LT(idx, header->field_ids_size_);
const DexFile::FieldId* field_id =
reinterpret_cast<const DexFile::FieldId*>(begin + header->field_ids_off_) + idx;
@@ -2408,9 +2410,8 @@
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)";
- }
+ // The `idx` has already been checked in `DexFileVerifier::CheckClassDataItemMethod()`.
+ CHECK_LT(idx, header->method_ids_size_);
const DexFile::MethodId* method_id =
reinterpret_cast<const DexFile::MethodId*>(begin + header->method_ids_off_) + idx;
diff --git a/runtime/dex_file_verifier_test.cc b/runtime/dex_file_verifier_test.cc
index 990ad5a..177a373 100644
--- a/runtime/dex_file_verifier_test.cc
+++ b/runtime/dex_file_verifier_test.cc
@@ -64,7 +64,7 @@
*(const_cast<uint8_t*>(dex_file->Begin()) + offset) = '7';
}
-static inline uint8_t* DecodeBase64(const char* src, size_t* dst_size) {
+static inline std::unique_ptr<uint8_t[]> DecodeBase64(const char* src, size_t* dst_size) {
std::vector<uint8_t> tmp;
uint32_t t = 0, y = 0;
int g = 3;
@@ -107,7 +107,7 @@
*dst_size = 0;
}
std::copy(tmp.begin(), tmp.end(), dst.get());
- return dst.release();
+ return dst;
}
static void FixUpChecksum(uint8_t* dex_file) {
@@ -120,25 +120,18 @@
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));
+ size_t length;
+ std::unique_ptr<uint8_t[]> dex_bytes = DecodeBase64(dex_file_base64_content, &length);
+ CHECK(dex_bytes != nullptr);
+ // Note: `dex_file` will be destroyed before `dex_bytes`.
+ std::unique_ptr<DexFile> dex_file(
+ new DexFile(dex_bytes.get(), length, "tmp", 0, nullptr, nullptr));
f(dex_file.get());
FixUpChecksum(const_cast<uint8_t*>(dex_file->Begin()));
@@ -157,15 +150,6 @@
}
}
}
-
- 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,
@@ -297,7 +281,9 @@
// 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) {
+static const uint8_t* FindMethodData(const DexFile* dex_file,
+ const char* name,
+ /*out*/ uint32_t* method_idx = nullptr) {
const DexFile::ClassDef& class_def = dex_file->GetClassDef(0);
const uint8_t* class_data = dex_file->GetClassData(class_def);
@@ -323,6 +309,9 @@
const DexFile::StringId& string_id = dex_file->GetStringId(name_index);
const char* str = dex_file->GetStringData(string_id);
if (strcmp(name, str) == 0) {
+ if (method_idx != nullptr) {
+ *method_idx = method_index;
+ }
DecodeUnsignedLeb128(&trailing);
return trailing;
}
@@ -693,6 +682,22 @@
}
}
+TEST_F(DexFileVerifierTest, B28552165) {
+ // Regression test for bad error string retrieval in different situations.
+ // Using invalid access flags to trigger the error.
+ VerifyModification(
+ kMethodFlagsTestDex,
+ "b28552165",
+ [](DexFile* dex_file) {
+ OrMaskToMethodFlags(dex_file, "foo", kAccPublic | kAccProtected);
+ uint32_t method_idx;
+ FindMethodData(dex_file, "foo", &method_idx);
+ auto* method_id = const_cast<DexFile::MethodId*>(&dex_file->GetMethodId(method_idx));
+ method_id->name_idx_ = dex_file->NumStringIds();
+ },
+ "Method may have only one of public/protected/private, LMethodFlags;.(error)");
+}
+
// 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.
