Make DexFiles opened from files readonly by default, but writable during dex2oat
Bug: 9618388
Change-Id: I83f2e16ee8446a79a94a84971146d807bb0c9ee0
diff --git a/compiler/dex/dex_to_dex_compiler.cc b/compiler/dex/dex_to_dex_compiler.cc
index a0e2c1e..a392f82 100644
--- a/compiler/dex/dex_to_dex_compiler.cc
+++ b/compiler/dex/dex_to_dex_compiler.cc
@@ -95,55 +95,6 @@
DISALLOW_COPY_AND_ASSIGN(DexCompiler);
};
-// Ensures write access to a part of DEX file.
-//
-// If a DEX file is read-only, it modifies its protection (mprotect) so it allows
-// write access to the part of DEX file defined by an address and a length.
-// In this case, it also takes the DexFile::modification_lock to prevent from
-// concurrent protection modification from a parallel DEX-to-DEX compilation on
-// the same DEX file.
-// When the instance is destroyed, it recovers original protection and releases
-// the lock.
-// TODO: as this scoped class is similar to a MutexLock we should use annotalysis
-// to capture the locking behavior.
-class ScopedDexWriteAccess {
- public:
- ScopedDexWriteAccess(DexFile& dex_file, Instruction* inst,
- size_t length)
- : dex_file_(dex_file),
- address_(reinterpret_cast<uint8_t*>(inst)),
- length_(length),
- is_read_only_(dex_file_.IsReadOnly()) {
- if (is_read_only_) {
- // We need to enable DEX write access. To avoid concurrent DEX write access
- // modification, we take the DexFile::modification_lock before.
- dex_file_.GetModificationLock().ExclusiveLock(Thread::Current());
- bool success = dex_file_.EnableWrite(address_, length_);
- DCHECK(success) << "Failed to enable DEX write access";
- }
- }
-
- ~ScopedDexWriteAccess() {
- DCHECK_EQ(is_read_only_, dex_file_.IsReadOnly());
- if (is_read_only_) {
- bool success = dex_file_.DisableWrite(address_, length_);
- DCHECK(success) << "Failed to disable DEX write access";
- // Now we recovered original read-only protection, we can release the
- // DexFile::modification_lock.
- dex_file_.GetModificationLock().ExclusiveUnlock(Thread::Current());
- }
- }
-
- private:
- DexFile& dex_file_;
- // TODO: make address_ const.
- uint8_t* address_;
- const size_t length_;
- const bool is_read_only_;
-
- DISALLOW_COPY_AND_ASSIGN(ScopedDexWriteAccess);
-};
-
void DexCompiler::Compile() {
DCHECK_GE(dex_to_dex_compilation_level_, kRequired);
const DexFile::CodeItem* code_item = unit_.GetCodeItem();
@@ -223,7 +174,6 @@
<< " by " << Instruction::Name(Instruction::RETURN_VOID_BARRIER)
<< " at dex pc " << StringPrintf("0x%x", dex_pc) << " in method "
<< PrettyMethod(unit_.GetDexMethodIndex(), GetDexFile(), true);
- ScopedDexWriteAccess sdwa(GetModifiableDexFile(), inst, 2u);
inst->SetOpcode(Instruction::RETURN_VOID_BARRIER);
}
@@ -246,7 +196,6 @@
<< StringPrintf("0x%x", dex_pc) << " in method "
<< PrettyMethod(unit_.GetDexMethodIndex(), GetDexFile(), true);
// We are modifying 4 consecutive bytes.
- ScopedDexWriteAccess sdwa(GetModifiableDexFile(), inst, 4u);
inst->SetOpcode(Instruction::NOP);
inst->SetVRegA_10x(0u); // keep compliant with verifier.
// Get to next instruction which is the second half of check-cast and replace
@@ -277,7 +226,6 @@
<< " at dex pc " << StringPrintf("0x%x", dex_pc) << " in method "
<< PrettyMethod(unit_.GetDexMethodIndex(), GetDexFile(), true);
// We are modifying 4 consecutive bytes.
- ScopedDexWriteAccess sdwa(GetModifiableDexFile(), inst, 4u);
inst->SetOpcode(new_opcode);
// Replace field index by field offset.
inst->SetVRegC_22c(static_cast<uint16_t>(field_offset));
@@ -313,7 +261,6 @@
<< " at dex pc " << StringPrintf("0x%x", dex_pc) << " in method "
<< PrettyMethod(unit_.GetDexMethodIndex(), GetDexFile(), true);
// We are modifying 4 consecutive bytes.
- ScopedDexWriteAccess sdwa(GetModifiableDexFile(), inst, 4u);
inst->SetOpcode(new_opcode);
// Replace method index by vtable index.
if (is_range) {
diff --git a/dex2oat/dex2oat.cc b/dex2oat/dex2oat.cc
index 5c96d74..b6a7683 100644
--- a/dex2oat/dex2oat.cc
+++ b/dex2oat/dex2oat.cc
@@ -417,9 +417,13 @@
const char* dex_location = dex_locations[i];
const DexFile* dex_file = DexFile::Open(dex_filename, dex_location);
if (dex_file == NULL) {
- LOG(WARNING) << "Could not open .dex from file '" << dex_filename << "'\n";
+ LOG(WARNING) << "Failed to open .dex from file '" << dex_filename << "'\n";
++failure_count;
} else {
+ // Ensure writable for dex-to-dex transformations.
+ if (!dex_file->EnableWrite()) {
+ PLOG(ERROR) << "Failed to make .dex file writeable '" << dex_filename << "'\n";
+ }
dex_files.push_back(dex_file);
}
}
@@ -567,7 +571,7 @@
argc--;
if (argc == 0) {
- Usage("no arguments specified");
+ Usage("No arguments specified");
}
std::vector<const char*> dex_filenames;
@@ -622,7 +626,7 @@
} else if (option.starts_with("--zip-fd=")) {
const char* zip_fd_str = option.substr(strlen("--zip-fd=")).data();
if (!ParseInt(zip_fd_str, &zip_fd)) {
- Usage("could not parse --zip-fd argument '%s' as an integer", zip_fd_str);
+ Usage("Failed to parse --zip-fd argument '%s' as an integer", zip_fd_str);
}
} else if (option.starts_with("--zip-location=")) {
zip_location = option.substr(strlen("--zip-location=")).data();
@@ -633,7 +637,7 @@
} else if (option.starts_with("--oat-fd=")) {
const char* oat_fd_str = option.substr(strlen("--oat-fd=")).data();
if (!ParseInt(oat_fd_str, &oat_fd)) {
- Usage("could not parse --oat-fd argument '%s' as an integer", oat_fd_str);
+ Usage("Failed to parse --oat-fd argument '%s' as an integer", oat_fd_str);
}
} else if (option == "--watch-dog") {
watch_dog_enabled = true;
@@ -642,7 +646,7 @@
} else if (option.starts_with("-j")) {
const char* thread_count_str = option.substr(strlen("-j")).data();
if (!ParseInt(thread_count_str, &thread_count)) {
- Usage("could not parse -j argument '%s' as an integer", thread_count_str);
+ Usage("Failed to parse -j argument '%s' as an integer", thread_count_str);
}
} else if (option.starts_with("--oat-location=")) {
oat_location = option.substr(strlen("--oat-location=")).data();
@@ -696,7 +700,7 @@
} else if (option == "--dump-timing") {
dump_timing = true;
} else {
- Usage("unknown argument %s", option.data());
+ Usage("Unknown argument %s", option.data());
}
}
@@ -789,7 +793,7 @@
if (boot_image_option.empty()) {
if (image_base == 0) {
- Usage("non-zero --base not specified");
+ Usage("Non-zero --base not specified");
}
}
diff --git a/runtime/common_test.h b/runtime/common_test.h
index e2dda86..dc1f592 100644
--- a/runtime/common_test.h
+++ b/runtime/common_test.h
@@ -434,6 +434,8 @@
filename += ".jar";
const DexFile* dex_file = DexFile::Open(filename, filename);
CHECK(dex_file != NULL) << "Failed to open " << filename;
+ CHECK_EQ(PROT_READ, dex_file->GetPermissions());
+ CHECK(dex_file->IsReadOnly());
opened_dex_files_.push_back(dex_file);
return dex_file;
}
diff --git a/runtime/dex_file.cc b/runtime/dex_file.cc
index 45b3427..4fd9a60 100644
--- a/runtime/dex_file.cc
+++ b/runtime/dex_file.cc
@@ -111,21 +111,21 @@
return GetPermissions() == PROT_READ;
}
-bool DexFile::EnableWrite(uint8_t* addr, size_t length) const {
+bool DexFile::EnableWrite() const {
CHECK(IsReadOnly());
if (mem_map_.get() == NULL) {
return false;
} else {
- return mem_map_->ProtectRegion(addr, length, PROT_READ | PROT_WRITE);
+ return mem_map_->Protect(PROT_READ | PROT_WRITE);
}
}
-bool DexFile::DisableWrite(uint8_t* addr, size_t length) const {
+bool DexFile::DisableWrite() const {
CHECK(!IsReadOnly());
if (mem_map_.get() == NULL) {
return false;
} else {
- return mem_map_->ProtectRegion(addr, length, PROT_READ);
+ return mem_map_->Protect(PROT_READ);
}
}
@@ -208,24 +208,26 @@
LOG(ERROR) << "Failed to find classes.dex within '" << location << "'";
return NULL;
}
-
UniquePtr<MemMap> map(zip_entry->ExtractToMemMap(kClassesDex));
if (map.get() == NULL) {
LOG(ERROR) << "Failed to extract '" << kClassesDex << "' from '" << location << "'";
return NULL;
}
- const DexFile* dex_file = OpenMemory(location, zip_entry->GetCrc32(), map.release());
- if (dex_file == NULL) {
+ UniquePtr<const DexFile> dex_file(OpenMemory(location, zip_entry->GetCrc32(), map.release()));
+ if (dex_file.get() == NULL) {
LOG(ERROR) << "Failed to open dex file '" << location << "' from memory";
return NULL;
}
-
- if (!DexFileVerifier::Verify(dex_file, dex_file->Begin(), dex_file->Size())) {
+ if (!DexFileVerifier::Verify(dex_file.get(), dex_file->Begin(), dex_file->Size())) {
LOG(ERROR) << "Failed to verify dex file '" << location << "'";
return NULL;
}
-
- return dex_file;
+ if (!dex_file->DisableWrite()) {
+ LOG(ERROR) << "Failed to make dex file read only '" << location << "'";
+ return NULL;
+ }
+ CHECK(dex_file->IsReadOnly()) << location;
+ return dex_file.release();
}
const DexFile* DexFile::OpenMemory(const byte* base,
@@ -839,7 +841,9 @@
DexDebugNewPositionCb position_cb, DexDebugNewLocalCb local_cb,
void* context) const {
const byte* stream = GetDebugInfoStream(code_item);
- UniquePtr<LocalInfo[]> local_in_reg(local_cb != NULL ? new LocalInfo[code_item->registers_size_] : NULL);
+ UniquePtr<LocalInfo[]> local_in_reg(local_cb != NULL ?
+ new LocalInfo[code_item->registers_size_] :
+ NULL);
if (stream != NULL) {
DecodeDebugInfo0(code_item, is_static, method_idx, position_cb, local_cb, context, stream, &local_in_reg[0]);
}
diff --git a/runtime/dex_file.h b/runtime/dex_file.h
index 006b692..26635ae 100644
--- a/runtime/dex_file.h
+++ b/runtime/dex_file.h
@@ -805,9 +805,9 @@
bool IsReadOnly() const;
- bool EnableWrite(uint8_t* addr, size_t size) const;
+ bool EnableWrite() const;
- bool DisableWrite(uint8_t* addr, size_t size) const;
+ bool DisableWrite() const;
private:
// Opens a .dex file
diff --git a/runtime/dex_file_test.cc b/runtime/dex_file_test.cc
index 6449493..32a8354 100644
--- a/runtime/dex_file_test.cc
+++ b/runtime/dex_file_test.cc
@@ -75,6 +75,8 @@
ScopedObjectAccess soa(Thread::Current());
const DexFile* dex_file = DexFile::Open(location, location);
CHECK(dex_file != NULL);
+ EXPECT_EQ(PROT_READ, dex_file->GetPermissions());
+ EXPECT_TRUE(dex_file->IsReadOnly());
return dex_file;
}
diff --git a/runtime/mem_map.cc b/runtime/mem_map.cc
index 9a34610..6451d5c 100644
--- a/runtime/mem_map.cc
+++ b/runtime/mem_map.cc
@@ -192,27 +192,4 @@
return false;
}
-bool MemMap::ProtectRegion(uint8_t* addr, size_t length, int prot) {
- CHECK_GE(addr, base_begin_);
- CHECK_LT(addr + length, reinterpret_cast<const uint8_t*>(base_begin_) + base_size_);
-
- /*
- * Align "addr" to a page boundary and adjust "length" appropriately.
- * (The address must be page-aligned, the length doesn't need to be,
- * but we do need to ensure we cover the same range.)
- */
- uint8_t* alignAddr = reinterpret_cast<uint8_t*>(RoundDown(reinterpret_cast<uintptr_t>(addr),
- kPageSize));
- size_t alignLength = length + (addr - alignAddr);
-
- if (mprotect(alignAddr, alignLength, prot) == 0) {
- prot_ = prot;
- return true;
- }
-
- PLOG(ERROR) << "mprotect(" << reinterpret_cast<void*>(alignAddr) << ", " << alignLength << ", "
- << prot << ") failed";
- return false;
-}
-
} // namespace art
diff --git a/runtime/mem_map.h b/runtime/mem_map.h
index 7d418a5..e294824 100644
--- a/runtime/mem_map.h
+++ b/runtime/mem_map.h
@@ -61,8 +61,6 @@
bool Protect(int prot);
- bool ProtectRegion(uint8_t* addr, size_t length, int prot);
-
int GetProtect() const {
return prot_;
}