Merge "Fix ProtoId ordering check in DexFileVerifier." into nyc-dev
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