ART: Make sure dex files are verified in the compiler
Follow-up to 9bdf108885a27ba05fae8501725649574d7c491b. Make sure
that dex files from the oat writer are verified with the dex file
verifier.
Bug: 26793137
Bug: 26808512
Change-Id: I1a5f51751491eead21d8f9f1b31e37c7374c72a5
diff --git a/compiler/image_test.cc b/compiler/image_test.cc
index b65fb36..a5a7796 100644
--- a/compiler/image_test.cc
+++ b/compiler/image_test.cc
@@ -119,6 +119,7 @@
compiler_driver_->GetInstructionSet(),
compiler_driver_->GetInstructionSetFeatures(),
&key_value_store,
+ /* verify */ false, // Dex files may be dex-to-dex-ed, don't verify.
&opened_dex_files_map,
&opened_dex_files);
ASSERT_TRUE(dex_files_ok);
diff --git a/compiler/oat_test.cc b/compiler/oat_test.cc
index c0d15f3..cff2f47 100644
--- a/compiler/oat_test.cc
+++ b/compiler/oat_test.cc
@@ -126,7 +126,8 @@
bool WriteElf(File* file,
const std::vector<const DexFile*>& dex_files,
- SafeMap<std::string, std::string>& key_value_store) {
+ SafeMap<std::string, std::string>& key_value_store,
+ bool verify) {
TimingLogger timings("WriteElf", false, false);
OatWriter oat_writer(/*compiling_boot_image*/false, &timings);
for (const DexFile* dex_file : dex_files) {
@@ -139,12 +140,13 @@
return false;
}
}
- return DoWriteElf(file, oat_writer, key_value_store);
+ return DoWriteElf(file, oat_writer, key_value_store, verify);
}
bool WriteElf(File* file,
const std::vector<const char*>& dex_filenames,
- SafeMap<std::string, std::string>& key_value_store) {
+ SafeMap<std::string, std::string>& key_value_store,
+ bool verify) {
TimingLogger timings("WriteElf", false, false);
OatWriter oat_writer(/*compiling_boot_image*/false, &timings);
for (const char* dex_filename : dex_filenames) {
@@ -152,24 +154,26 @@
return false;
}
}
- return DoWriteElf(file, oat_writer, key_value_store);
+ return DoWriteElf(file, oat_writer, key_value_store, verify);
}
bool WriteElf(File* file,
ScopedFd&& zip_fd,
const char* location,
- SafeMap<std::string, std::string>& key_value_store) {
+ SafeMap<std::string, std::string>& key_value_store,
+ bool verify) {
TimingLogger timings("WriteElf", false, false);
OatWriter oat_writer(/*compiling_boot_image*/false, &timings);
if (!oat_writer.AddZippedDexFilesSource(std::move(zip_fd), location)) {
return false;
}
- return DoWriteElf(file, oat_writer, key_value_store);
+ return DoWriteElf(file, oat_writer, key_value_store, verify);
}
bool DoWriteElf(File* file,
OatWriter& oat_writer,
- SafeMap<std::string, std::string>& key_value_store) {
+ SafeMap<std::string, std::string>& key_value_store,
+ bool verify) {
std::unique_ptr<ElfWriter> elf_writer = CreateElfWriterQuick(
compiler_driver_->GetInstructionSet(),
&compiler_driver_->GetCompilerOptions(),
@@ -183,6 +187,7 @@
compiler_driver_->GetInstructionSet(),
compiler_driver_->GetInstructionSetFeatures(),
&key_value_store,
+ verify,
&opened_dex_files_map,
&opened_dex_files)) {
return false;
@@ -219,6 +224,9 @@
return elf_writer->End();
}
+ void TestDexFileInput(bool verify);
+ void TestZipFileInput(bool verify);
+
std::unique_ptr<const InstructionSetFeatures> insn_features_;
std::unique_ptr<QuickCompilerCallbacks> callbacks_;
};
@@ -354,7 +362,7 @@
ScratchFile tmp;
SafeMap<std::string, std::string> key_value_store;
key_value_store.Put(OatHeader::kImageLocationKey, "lue.art");
- bool success = WriteElf(tmp.GetFile(), class_linker->GetBootClassPath(), key_value_store);
+ bool success = WriteElf(tmp.GetFile(), class_linker->GetBootClassPath(), key_value_store, false);
ASSERT_TRUE(success);
if (kCompile) { // OatWriter strips the code, regenerate to compare
@@ -480,7 +488,7 @@
ScratchFile tmp;
SafeMap<std::string, std::string> key_value_store;
key_value_store.Put(OatHeader::kImageLocationKey, "test.art");
- bool success = WriteElf(tmp.GetFile(), dex_files, key_value_store);
+ bool success = WriteElf(tmp.GetFile(), dex_files, key_value_store, false);
ASSERT_TRUE(success);
std::unique_ptr<OatFile> oat_file(OatFile::Open(tmp.GetFilename(),
@@ -494,7 +502,15 @@
EXPECT_LT(static_cast<size_t>(oat_file->Size()), static_cast<size_t>(tmp.GetFile()->GetLength()));
}
-TEST_F(OatTest, DexFileInput) {
+static void MaybeModifyDexFileToFail(bool verify, std::unique_ptr<const DexFile>& data) {
+ // If in verify mode (= fail the verifier mode), make sure we fail early. We'll fail already
+ // because of the missing map, but that may lead to out of bounds reads.
+ if (verify) {
+ const_cast<DexFile::Header*>(&data->GetHeader())->checksum_++;
+ }
+}
+
+void OatTest::TestDexFileInput(bool verify) {
TimingLogger timings("OatTest::DexFileInput", false, false);
std::vector<const char*> input_filenames;
@@ -504,6 +520,9 @@
builder1.AddField("Lsome.TestClass;", "int", "someField");
builder1.AddMethod("Lsome.TestClass;", "()I", "foo");
std::unique_ptr<const DexFile> dex_file1_data = builder1.Build(dex_file1.GetFilename());
+
+ MaybeModifyDexFileToFail(verify, dex_file1_data);
+
bool success = dex_file1.GetFile()->WriteFully(&dex_file1_data->GetHeader(),
dex_file1_data->GetHeader().file_size_);
ASSERT_TRUE(success);
@@ -516,6 +535,9 @@
builder2.AddField("Land.AnotherTestClass;", "boolean", "someOtherField");
builder2.AddMethod("Land.AnotherTestClass;", "()J", "bar");
std::unique_ptr<const DexFile> dex_file2_data = builder2.Build(dex_file2.GetFilename());
+
+ MaybeModifyDexFileToFail(verify, dex_file2_data);
+
success = dex_file2.GetFile()->WriteFully(&dex_file2_data->GetHeader(),
dex_file2_data->GetHeader().file_size_);
ASSERT_TRUE(success);
@@ -526,7 +548,14 @@
ScratchFile oat_file;
SafeMap<std::string, std::string> key_value_store;
key_value_store.Put(OatHeader::kImageLocationKey, "test.art");
- success = WriteElf(oat_file.GetFile(), input_filenames, key_value_store);
+ success = WriteElf(oat_file.GetFile(), input_filenames, key_value_store, verify);
+
+ // In verify mode, we expect failure.
+ if (verify) {
+ ASSERT_FALSE(success);
+ return;
+ }
+
ASSERT_TRUE(success);
std::string error_msg;
@@ -557,7 +586,15 @@
ASSERT_EQ(dex_file2_data->GetLocation(), opened_dex_file2->GetLocation());
}
-TEST_F(OatTest, ZipFileInput) {
+TEST_F(OatTest, DexFileInputCheckOutput) {
+ TestDexFileInput(false);
+}
+
+TEST_F(OatTest, DexFileInputCheckVerifier) {
+ TestDexFileInput(true);
+}
+
+void OatTest::TestZipFileInput(bool verify) {
TimingLogger timings("OatTest::DexFileInput", false, false);
ScratchFile zip_file;
@@ -568,6 +605,9 @@
builder1.AddField("Lsome.TestClass;", "long", "someField");
builder1.AddMethod("Lsome.TestClass;", "()D", "foo");
std::unique_ptr<const DexFile> dex_file1_data = builder1.Build(dex_file1.GetFilename());
+
+ MaybeModifyDexFileToFail(verify, dex_file1_data);
+
bool success = dex_file1.GetFile()->WriteFully(&dex_file1_data->GetHeader(),
dex_file1_data->GetHeader().file_size_);
ASSERT_TRUE(success);
@@ -583,6 +623,9 @@
builder2.AddField("Land.AnotherTestClass;", "boolean", "someOtherField");
builder2.AddMethod("Land.AnotherTestClass;", "()J", "bar");
std::unique_ptr<const DexFile> dex_file2_data = builder2.Build(dex_file2.GetFilename());
+
+ MaybeModifyDexFileToFail(verify, dex_file2_data);
+
success = dex_file2.GetFile()->WriteFully(&dex_file2_data->GetHeader(),
dex_file2_data->GetHeader().file_size_);
ASSERT_TRUE(success);
@@ -603,37 +646,42 @@
std::vector<const char*> input_filenames { zip_file.GetFilename().c_str() }; // NOLINT [readability/braces] [4]
ScratchFile oat_file;
- success = WriteElf(oat_file.GetFile(), input_filenames, key_value_store);
- ASSERT_TRUE(success);
+ success = WriteElf(oat_file.GetFile(), input_filenames, key_value_store, verify);
- std::string error_msg;
- std::unique_ptr<OatFile> opened_oat_file(OatFile::Open(oat_file.GetFilename(),
- oat_file.GetFilename(),
- nullptr,
- nullptr,
- false,
- nullptr,
- &error_msg));
- ASSERT_TRUE(opened_oat_file != nullptr);
- ASSERT_EQ(2u, opened_oat_file->GetOatDexFiles().size());
- std::unique_ptr<const DexFile> opened_dex_file1 =
- opened_oat_file->GetOatDexFiles()[0]->OpenDexFile(&error_msg);
- std::unique_ptr<const DexFile> opened_dex_file2 =
- opened_oat_file->GetOatDexFiles()[1]->OpenDexFile(&error_msg);
+ if (verify) {
+ ASSERT_FALSE(success);
+ } else {
+ ASSERT_TRUE(success);
- ASSERT_EQ(dex_file1_data->GetHeader().file_size_, opened_dex_file1->GetHeader().file_size_);
- ASSERT_EQ(0, memcmp(&dex_file1_data->GetHeader(),
- &opened_dex_file1->GetHeader(),
- dex_file1_data->GetHeader().file_size_));
- ASSERT_EQ(DexFile::GetMultiDexLocation(0, zip_file.GetFilename().c_str()),
- opened_dex_file1->GetLocation());
+ std::string error_msg;
+ std::unique_ptr<OatFile> opened_oat_file(OatFile::Open(oat_file.GetFilename(),
+ oat_file.GetFilename(),
+ nullptr,
+ nullptr,
+ false,
+ nullptr,
+ &error_msg));
+ ASSERT_TRUE(opened_oat_file != nullptr);
+ ASSERT_EQ(2u, opened_oat_file->GetOatDexFiles().size());
+ std::unique_ptr<const DexFile> opened_dex_file1 =
+ opened_oat_file->GetOatDexFiles()[0]->OpenDexFile(&error_msg);
+ std::unique_ptr<const DexFile> opened_dex_file2 =
+ opened_oat_file->GetOatDexFiles()[1]->OpenDexFile(&error_msg);
- ASSERT_EQ(dex_file2_data->GetHeader().file_size_, opened_dex_file2->GetHeader().file_size_);
- ASSERT_EQ(0, memcmp(&dex_file2_data->GetHeader(),
- &opened_dex_file2->GetHeader(),
- dex_file2_data->GetHeader().file_size_));
- ASSERT_EQ(DexFile::GetMultiDexLocation(1, zip_file.GetFilename().c_str()),
- opened_dex_file2->GetLocation());
+ ASSERT_EQ(dex_file1_data->GetHeader().file_size_, opened_dex_file1->GetHeader().file_size_);
+ ASSERT_EQ(0, memcmp(&dex_file1_data->GetHeader(),
+ &opened_dex_file1->GetHeader(),
+ dex_file1_data->GetHeader().file_size_));
+ ASSERT_EQ(DexFile::GetMultiDexLocation(0, zip_file.GetFilename().c_str()),
+ opened_dex_file1->GetLocation());
+
+ ASSERT_EQ(dex_file2_data->GetHeader().file_size_, opened_dex_file2->GetHeader().file_size_);
+ ASSERT_EQ(0, memcmp(&dex_file2_data->GetHeader(),
+ &opened_dex_file2->GetHeader(),
+ dex_file2_data->GetHeader().file_size_));
+ ASSERT_EQ(DexFile::GetMultiDexLocation(1, zip_file.GetFilename().c_str()),
+ opened_dex_file2->GetLocation());
+ }
}
{
@@ -645,38 +693,51 @@
success = WriteElf(oat_file.GetFile(),
std::move(zip_fd),
zip_file.GetFilename().c_str(),
- key_value_store);
- ASSERT_TRUE(success);
+ key_value_store,
+ verify);
+ if (verify) {
+ ASSERT_FALSE(success);
+ } else {
+ ASSERT_TRUE(success);
- std::string error_msg;
- std::unique_ptr<OatFile> opened_oat_file(OatFile::Open(oat_file.GetFilename(),
- oat_file.GetFilename(),
- nullptr,
- nullptr,
- false,
- nullptr,
- &error_msg));
- ASSERT_TRUE(opened_oat_file != nullptr);
- ASSERT_EQ(2u, opened_oat_file->GetOatDexFiles().size());
- std::unique_ptr<const DexFile> opened_dex_file1 =
- opened_oat_file->GetOatDexFiles()[0]->OpenDexFile(&error_msg);
- std::unique_ptr<const DexFile> opened_dex_file2 =
- opened_oat_file->GetOatDexFiles()[1]->OpenDexFile(&error_msg);
+ std::string error_msg;
+ std::unique_ptr<OatFile> opened_oat_file(OatFile::Open(oat_file.GetFilename(),
+ oat_file.GetFilename(),
+ nullptr,
+ nullptr,
+ false,
+ nullptr,
+ &error_msg));
+ ASSERT_TRUE(opened_oat_file != nullptr);
+ ASSERT_EQ(2u, opened_oat_file->GetOatDexFiles().size());
+ std::unique_ptr<const DexFile> opened_dex_file1 =
+ opened_oat_file->GetOatDexFiles()[0]->OpenDexFile(&error_msg);
+ std::unique_ptr<const DexFile> opened_dex_file2 =
+ opened_oat_file->GetOatDexFiles()[1]->OpenDexFile(&error_msg);
- ASSERT_EQ(dex_file1_data->GetHeader().file_size_, opened_dex_file1->GetHeader().file_size_);
- ASSERT_EQ(0, memcmp(&dex_file1_data->GetHeader(),
- &opened_dex_file1->GetHeader(),
- dex_file1_data->GetHeader().file_size_));
- ASSERT_EQ(DexFile::GetMultiDexLocation(0, zip_file.GetFilename().c_str()),
- opened_dex_file1->GetLocation());
+ ASSERT_EQ(dex_file1_data->GetHeader().file_size_, opened_dex_file1->GetHeader().file_size_);
+ ASSERT_EQ(0, memcmp(&dex_file1_data->GetHeader(),
+ &opened_dex_file1->GetHeader(),
+ dex_file1_data->GetHeader().file_size_));
+ ASSERT_EQ(DexFile::GetMultiDexLocation(0, zip_file.GetFilename().c_str()),
+ opened_dex_file1->GetLocation());
- ASSERT_EQ(dex_file2_data->GetHeader().file_size_, opened_dex_file2->GetHeader().file_size_);
- ASSERT_EQ(0, memcmp(&dex_file2_data->GetHeader(),
- &opened_dex_file2->GetHeader(),
- dex_file2_data->GetHeader().file_size_));
- ASSERT_EQ(DexFile::GetMultiDexLocation(1, zip_file.GetFilename().c_str()),
- opened_dex_file2->GetLocation());
+ ASSERT_EQ(dex_file2_data->GetHeader().file_size_, opened_dex_file2->GetHeader().file_size_);
+ ASSERT_EQ(0, memcmp(&dex_file2_data->GetHeader(),
+ &opened_dex_file2->GetHeader(),
+ dex_file2_data->GetHeader().file_size_));
+ ASSERT_EQ(DexFile::GetMultiDexLocation(1, zip_file.GetFilename().c_str()),
+ opened_dex_file2->GetLocation());
+ }
}
}
+TEST_F(OatTest, ZipFileInputCheckOutput) {
+ TestZipFileInput(false);
+}
+
+TEST_F(OatTest, ZipFileInputCheckVerifier) {
+ TestZipFileInput(true);
+}
+
} // namespace art
diff --git a/compiler/oat_writer.cc b/compiler/oat_writer.cc
index a542162..5eb9000 100644
--- a/compiler/oat_writer.cc
+++ b/compiler/oat_writer.cc
@@ -397,6 +397,7 @@
InstructionSet instruction_set,
const InstructionSetFeatures* instruction_set_features,
SafeMap<std::string, std::string>* key_value_store,
+ bool verify,
/*out*/ std::unique_ptr<MemMap>* opened_dex_files_map,
/*out*/ std::vector<std::unique_ptr<const DexFile>>* opened_dex_files) {
CHECK(write_state_ == WriteState::kAddingDexFileSources);
@@ -424,7 +425,7 @@
}
if (!WriteOatDexFiles(rodata) ||
!ExtendForTypeLookupTables(rodata, file, size_after_type_lookup_tables) ||
- !OpenDexFiles(file, &dex_files_map, &dex_files) ||
+ !OpenDexFiles(file, verify, &dex_files_map, &dex_files) ||
!WriteTypeLookupTables(dex_files_map.get(), dex_files)) {
return false;
}
@@ -2143,6 +2144,7 @@
bool OatWriter::OpenDexFiles(
File* file,
+ bool verify,
/*out*/ std::unique_ptr<MemMap>* opened_dex_files_map,
/*out*/ std::vector<std::unique_ptr<const DexFile>>* opened_dex_files) {
TimingLogger::ScopedTiming split("OpenDexFiles", timings_);
@@ -2201,9 +2203,11 @@
oat_dex_file.GetLocation(),
oat_dex_file.dex_file_location_checksum_,
/* oat_dex_file */ nullptr,
+ verify,
&error_msg));
if (dex_files.back() == nullptr) {
- LOG(ERROR) << "Failed to open dex file from oat file. File:" << oat_dex_file.GetLocation();
+ LOG(ERROR) << "Failed to open dex file from oat file. File: " << oat_dex_file.GetLocation()
+ << " Error: " << error_msg;
return false;
}
}
diff --git a/compiler/oat_writer.h b/compiler/oat_writer.h
index d681998..14c6d50 100644
--- a/compiler/oat_writer.h
+++ b/compiler/oat_writer.h
@@ -139,12 +139,15 @@
CreateTypeLookupTable create_type_lookup_table = CreateTypeLookupTable::kDefault);
dchecked_vector<const char*> GetSourceLocations() const;
- // Write raw dex files to the .rodata section and open them from the oat file.
+ // Write raw dex files to the .rodata section and open them from the oat file. The verify
+ // setting dictates whether the dex file verifier should check the dex files. This is generally
+ // the case, and should only be false for tests.
bool WriteAndOpenDexFiles(OutputStream* rodata,
File* file,
InstructionSet instruction_set,
const InstructionSetFeatures* instruction_set_features,
SafeMap<std::string, std::string>* key_value_store,
+ bool verify,
/*out*/ std::unique_ptr<MemMap>* opened_dex_files_map,
/*out*/ std::vector<std::unique_ptr<const DexFile>>* opened_dex_files);
// Prepare layout of remaining data.
@@ -258,6 +261,7 @@
bool WriteOatDexFiles(OutputStream* rodata);
bool ExtendForTypeLookupTables(OutputStream* rodata, File* file, size_t offset);
bool OpenDexFiles(File* file,
+ bool verify,
/*out*/ std::unique_ptr<MemMap>* opened_dex_files_map,
/*out*/ std::vector<std::unique_ptr<const DexFile>>* opened_dex_files);
bool WriteTypeLookupTables(MemMap* opened_dex_files_map,
diff --git a/compiler/utils/test_dex_file_builder.h b/compiler/utils/test_dex_file_builder.h
index e57a540..2958dc6 100644
--- a/compiler/utils/test_dex_file_builder.h
+++ b/compiler/utils/test_dex_file_builder.h
@@ -89,11 +89,12 @@
DexFile::Header* header = reinterpret_cast<DexFile::Header*>(&header_data.data);
std::copy_n(DexFile::kDexMagic, 4u, header->magic_);
std::copy_n(DexFile::kDexMagicVersion, 4u, header->magic_ + 4u);
- header->header_size_ = sizeof(header);
+ header->header_size_ = sizeof(DexFile::Header);
header->endian_tag_ = DexFile::kDexEndianConstant;
header->link_size_ = 0u; // Unused.
header->link_off_ = 0u; // Unused.
- header->map_off_ = 0u; // Unused.
+ header->map_off_ = 0u; // Unused. TODO: This is wrong. Dex files created by this builder
+ // cannot be verified. b/26808512
uint32_t data_section_size = 0u;
@@ -213,13 +214,22 @@
// Leave signature as zeros.
header->file_size_ = dex_file_data_.size();
+
+ // Write the complete header early, as part of it needs to be checksummed.
+ std::memcpy(&dex_file_data_[0], header_data.data, sizeof(DexFile::Header));
+
+ // Checksum starts after the checksum field.
size_t skip = sizeof(header->magic_) + sizeof(header->checksum_);
- header->checksum_ = adler32(0u, dex_file_data_.data() + skip, dex_file_data_.size() - skip);
+ header->checksum_ = adler32(adler32(0L, Z_NULL, 0),
+ dex_file_data_.data() + skip,
+ dex_file_data_.size() - skip);
+
+ // Write the complete header again, just simpler that way.
std::memcpy(&dex_file_data_[0], header_data.data, sizeof(DexFile::Header));
std::string error_msg;
std::unique_ptr<const DexFile> dex_file(DexFile::Open(
- &dex_file_data_[0], dex_file_data_.size(), dex_location, 0u, nullptr, &error_msg));
+ &dex_file_data_[0], dex_file_data_.size(), dex_location, 0u, nullptr, false, &error_msg));
CHECK(dex_file != nullptr) << error_msg;
return dex_file;
}