Fix ProtoId ordering check in DexFileVerifier.
The code previously checked for kNoDexIndex16 as the type
list terminator. This is incorrect as we should not actually
see the kNoDexIndex16 in type lists in supported dex files.
To make sure that we don't see kNoDexIndex16, check the size
of the arrays with documented limits, i.e. type-ids and
proto-ids, see dex_file.h. In the ProtoId ordering check,
DCHECK() that we don't encounter kNoDexIndex16 and verify
that the previous list is not longer if the current list's
elements match.
Bug: 28580925
(cherry picked from commit 0ca8add2ae34c26291694ecc784d64f0cc1c1380)
Change-Id: Ied6dcbd8d04e3dfef5695dbd9b3a30a079038b2b
diff --git a/runtime/dex_file_verifier.cc b/runtime/dex_file_verifier.cc
index c504074..bbffbbb 100644
--- a/runtime/dex_file_verifier.cc
+++ b/runtime/dex_file_verifier.cc
@@ -251,6 +251,14 @@
return true;
}
+bool DexFileVerifier::CheckSizeLimit(uint32_t size, uint32_t limit, const char* label) {
+ if (size > limit) {
+ ErrorStringPrintf("Size(%u) should not exceed limit(%u) for %s.", size, limit, label);
+ return false;
+ }
+ return true;
+}
+
bool DexFileVerifier::CheckHeader() {
// Check file size from the header.
uint32_t expected_size = header_->file_size_;
@@ -298,10 +306,12 @@
header_->type_ids_size_,
4,
"type-ids") &&
+ CheckSizeLimit(header_->type_ids_size_, DexFile::kDexNoIndex16, "type-ids") &&
CheckValidOffsetAndSize(header_->proto_ids_off_,
header_->proto_ids_size_,
4,
"proto-ids") &&
+ CheckSizeLimit(header_->proto_ids_size_, DexFile::kDexNoIndex16, "proto-ids") &&
CheckValidOffsetAndSize(header_->field_ids_off_,
header_->field_ids_size_,
4,
@@ -1786,13 +1796,8 @@
while (curr_it.HasNext() && prev_it.HasNext()) {
uint16_t prev_idx = prev_it.GetTypeIdx();
uint16_t curr_idx = curr_it.GetTypeIdx();
- if (prev_idx == DexFile::kDexNoIndex16) {
- break;
- }
- if (UNLIKELY(curr_idx == DexFile::kDexNoIndex16)) {
- ErrorStringPrintf("Out-of-order proto_id arguments");
- return false;
- }
+ DCHECK_NE(prev_idx, DexFile::kDexNoIndex16);
+ DCHECK_NE(curr_idx, DexFile::kDexNoIndex16);
if (prev_idx < curr_idx) {
break;
@@ -1804,6 +1809,12 @@
prev_it.Next();
curr_it.Next();
}
+ if (!curr_it.HasNext()) {
+ // Either a duplicate ProtoId or a ProtoId with a shorter argument list follows
+ // a ProtoId with a longer one. Both cases are forbidden by the specification.
+ ErrorStringPrintf("Out-of-order proto_id arguments");
+ return false;
+ }
}
}
diff --git a/runtime/dex_file_verifier.h b/runtime/dex_file_verifier.h
index be0e6d8..90409db 100644
--- a/runtime/dex_file_verifier.h
+++ b/runtime/dex_file_verifier.h
@@ -49,6 +49,8 @@
// Checks whether the offset is zero (when size is zero) or that the offset falls within the area
// claimed by the file.
bool CheckValidOffsetAndSize(uint32_t offset, uint32_t size, size_t alignment, const char* label);
+ // Checks whether the size is less than the limit.
+ bool CheckSizeLimit(uint32_t size, uint32_t limit, const char* label);
bool CheckIndex(uint32_t field, uint32_t limit, const char* label);
bool CheckHeader();
diff --git a/runtime/dex_file_verifier_test.cc b/runtime/dex_file_verifier_test.cc
index 177a373..3741c1e 100644
--- a/runtime/dex_file_verifier_test.cc
+++ b/runtime/dex_file_verifier_test.cc
@@ -1444,4 +1444,81 @@
}
}
+// Generated from
+//
+// .class LOverloading;
+//
+// .super Ljava/lang/Object;
+//
+// .method public static foo()V
+// .registers 1
+// return-void
+// .end method
+//
+// .method public static foo(I)V
+// .registers 1
+// return-void
+// .end method
+static const char kProtoOrderingTestDex[] =
+ "ZGV4CjAzNQA1L+ABE6voQ9Lr4Ci//efB53oGnDr5PinsAQAAcAAAAHhWNBIAAAAAAAAAAFgBAAAG"
+ "AAAAcAAAAAQAAACIAAAAAgAAAJgAAAAAAAAAAAAAAAIAAACwAAAAAQAAAMAAAAAMAQAA4AAAAOAA"
+ "AADjAAAA8gAAAAYBAAAJAQAADQEAAAAAAAABAAAAAgAAAAMAAAADAAAAAwAAAAAAAAAEAAAAAwAA"
+ "ABQBAAABAAAABQAAAAEAAQAFAAAAAQAAAAAAAAACAAAAAAAAAP////8AAAAASgEAAAAAAAABSQAN"
+ "TE92ZXJsb2FkaW5nOwASTGphdmEvbGFuZy9PYmplY3Q7AAFWAAJWSQADZm9vAAAAAQAAAAAAAAAA"
+ "AAAAAAAAAAEAAAAAAAAAAAAAAAEAAAAOAAAAAQABAAAAAAAAAAAAAQAAAA4AAAACAAAJpAIBCbgC"
+ "AAAMAAAAAAAAAAEAAAAAAAAAAQAAAAYAAABwAAAAAgAAAAQAAACIAAAAAwAAAAIAAACYAAAABQAA"
+ "AAIAAACwAAAABgAAAAEAAADAAAAAAiAAAAYAAADgAAAAARAAAAEAAAAUAQAAAxAAAAIAAAAcAQAA"
+ "ASAAAAIAAAAkAQAAACAAAAEAAABKAQAAABAAAAEAAABYAQAA";
+
+TEST_F(DexFileVerifierTest, ProtoOrdering) {
+ {
+ // The input dex file should be good before modification.
+ ScratchFile tmp;
+ std::string error_msg;
+ std::unique_ptr<const DexFile> raw(OpenDexFileBase64(kProtoOrderingTestDex,
+ tmp.GetFilename().c_str(),
+ &error_msg));
+ ASSERT_TRUE(raw.get() != nullptr) << error_msg;
+ }
+
+ // Modify the order of the ProtoIds for two overloads of "foo" with the
+ // same return type and one having longer parameter list than the other.
+ for (size_t i = 0; i != 2; ++i) {
+ VerifyModification(
+ kProtoOrderingTestDex,
+ "proto_ordering",
+ [i](DexFile* dex_file) {
+ uint32_t method_idx;
+ const uint8_t* data = FindMethodData(dex_file, "foo", &method_idx);
+ CHECK(data != nullptr);
+ // There should be 2 methods called "foo".
+ CHECK_LT(method_idx + 1u, dex_file->NumMethodIds());
+ CHECK_EQ(dex_file->GetMethodId(method_idx).name_idx_,
+ dex_file->GetMethodId(method_idx + 1).name_idx_);
+ CHECK_EQ(dex_file->GetMethodId(method_idx).proto_idx_ + 1u,
+ dex_file->GetMethodId(method_idx + 1).proto_idx_);
+ // Their return types should be the same.
+ uint32_t proto1_idx = dex_file->GetMethodId(method_idx).proto_idx_;
+ const DexFile::ProtoId& proto1 = dex_file->GetProtoId(proto1_idx);
+ const DexFile::ProtoId& proto2 = dex_file->GetProtoId(proto1_idx + 1u);
+ CHECK_EQ(proto1.return_type_idx_, proto2.return_type_idx_);
+ // And the first should not have any parameters while the second should have some.
+ CHECK(!DexFileParameterIterator(*dex_file, proto1).HasNext());
+ CHECK(DexFileParameterIterator(*dex_file, proto2).HasNext());
+ if (i == 0) {
+ // Swap the proto parameters and shorties to break the ordering.
+ std::swap(const_cast<uint32_t&>(proto1.parameters_off_),
+ const_cast<uint32_t&>(proto2.parameters_off_));
+ std::swap(const_cast<uint32_t&>(proto1.shorty_idx_),
+ const_cast<uint32_t&>(proto2.shorty_idx_));
+ } else {
+ // Copy the proto parameters and shorty to create duplicate proto id.
+ const_cast<uint32_t&>(proto1.parameters_off_) = proto2.parameters_off_;
+ const_cast<uint32_t&>(proto1.shorty_idx_) = proto2.shorty_idx_;
+ }
+ },
+ "Out-of-order proto_id arguments");
+ }
+}
+
} // namespace art
