From 0ca8add2ae34c26291694ecc784d64f0cc1c1380 Mon Sep 17 00:00:00 2001 From: Vladimir Marko Date: Tue, 3 May 2016 17:17:50 +0100 Subject: [PATCH] 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 Change-Id: I7b17ce54b0047d97e13be88377676b1b7c713ae1 --- runtime/dex_file_verifier.cc | 25 +++++++++---- runtime/dex_file_verifier.h | 2 + runtime/dex_file_verifier_test.cc | 77 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 7 deletions(-) diff --git a/runtime/dex_file_verifier.cc b/runtime/dex_file_verifier.cc index 21ee4bd52..bafa05792 100644 --- a/runtime/dex_file_verifier.cc +++ b/runtime/dex_file_verifier.cc @@ -251,6 +251,14 @@ bool DexFileVerifier::CheckValidOffsetAndSize(uint32_t offset, 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 @@ bool DexFileVerifier::CheckHeader() { 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 @@ bool DexFileVerifier::CheckInterProtoIdItem() { 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 @@ bool DexFileVerifier::CheckInterProtoIdItem() { 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 be0e6d83f..90409db44 100644 --- a/runtime/dex_file_verifier.h +++ b/runtime/dex_file_verifier.h @@ -49,6 +49,8 @@ class DexFileVerifier { // 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 84f48dacc..721f26c0b 100644 --- a/runtime/dex_file_verifier_test.cc +++ b/runtime/dex_file_verifier_test.cc @@ -1441,4 +1441,81 @@ TEST_F(DexFileVerifierTest, SectionAlignment) { } } +// 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 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(proto1.parameters_off_), + const_cast(proto2.parameters_off_)); + std::swap(const_cast(proto1.shorty_idx_), + const_cast(proto2.shorty_idx_)); + } else { + // Copy the proto parameters and shorty to create duplicate proto id. + const_cast(proto1.parameters_off_) = proto2.parameters_off_; + const_cast(proto1.shorty_idx_) = proto2.shorty_idx_; + } + }, + "Out-of-order proto_id arguments"); + } +} + } // namespace art -- 2.11.0