Make MemMap::MapAnonymous() fail if the requested address is not available.
Change MapAnonymous() so that a requested address vs. actual map
address mismatch will cause a failure. The existing MapAnonymous()
call sites do not check this. This should prevent potential rare case
bugs where mmap does not happen to map a region at an specified
address.
There's a potential bug that if MapAnonymous() does not guarantee the
requested address (and there's a gap between the image/oat files and
the zygote/malloc space), then GC could in theory allocate a large
object space in the gap. This would break the GC notion of the immune
space. This change will prevent this by causing all non-moving spaces
to be (really) adjacent, with no gaps in between, which CL 87711
missed.
Change-Id: Id4adb0e30adbad497334d7e00def4c0c66b15719
diff --git a/runtime/elf_file.cc b/runtime/elf_file.cc
index 4de46e3..0c8a4f0 100644
--- a/runtime/elf_file.cc
+++ b/runtime/elf_file.cc
@@ -835,7 +835,7 @@
if ((program_header.p_flags & PF_R) != 0) {
prot |= PROT_READ;
}
- int flags = MAP_FIXED;
+ int flags = 0;
if (writable_) {
prot |= PROT_WRITE;
flags |= MAP_SHARED;
@@ -853,7 +853,7 @@
program_header.p_memsz,
prot, flags, file_->Fd(),
program_header.p_offset,
- true,
+ true, // implies MAP_FIXED
file_->GetPath().c_str(),
error_msg));
if (segment.get() == nullptr) {
diff --git a/runtime/gc/space/image_space.cc b/runtime/gc/space/image_space.cc
index 5480639..bb52c66 100644
--- a/runtime/gc/space/image_space.cc
+++ b/runtime/gc/space/image_space.cc
@@ -201,7 +201,7 @@
UniquePtr<MemMap> map(MemMap::MapFileAtAddress(image_header.GetImageBegin(),
image_header.GetImageSize(),
PROT_READ | PROT_WRITE,
- MAP_PRIVATE | MAP_FIXED,
+ MAP_PRIVATE,
file->Fd(),
0,
false,
diff --git a/runtime/mem_map.cc b/runtime/mem_map.cc
index fdfb477..582ab6e 100644
--- a/runtime/mem_map.cc
+++ b/runtime/mem_map.cc
@@ -32,8 +32,6 @@
namespace art {
-#if !defined(NDEBUG)
-
static std::ostream& operator<<(
std::ostream& os,
std::pair<BacktraceMap::const_iterator, BacktraceMap::const_iterator> iters) {
@@ -48,43 +46,61 @@
return os;
}
-static void CheckMapRequest(byte* addr, size_t byte_count) {
- if (addr == NULL) {
- return;
+static bool CheckMapRequest(byte* expected_ptr, void* actual_ptr, size_t byte_count,
+ std::ostringstream* error_msg) {
+ // Handled first by caller for more specific error messages.
+ CHECK(actual_ptr != MAP_FAILED);
+
+ if (expected_ptr == nullptr) {
+ return true;
}
- uintptr_t base = reinterpret_cast<uintptr_t>(addr);
- uintptr_t limit = base + byte_count;
+ if (expected_ptr == actual_ptr) {
+ return true;
+ }
+
+ // We asked for an address but didn't get what we wanted, all paths below here should fail.
+ int result = munmap(actual_ptr, byte_count);
+ if (result == -1) {
+ PLOG(WARNING) << StringPrintf("munmap(%p, %zd) failed", actual_ptr, byte_count);
+ }
+
+ uintptr_t actual = reinterpret_cast<uintptr_t>(actual_ptr);
+ uintptr_t expected = reinterpret_cast<uintptr_t>(expected_ptr);
+ uintptr_t limit = expected + byte_count;
UniquePtr<BacktraceMap> map(BacktraceMap::Create(getpid()));
if (!map->Build()) {
- PLOG(WARNING) << "Failed to build process map";
- return;
+ *error_msg << StringPrintf("Failed to build process map to determine why mmap returned "
+ "0x%08" PRIxPTR " instead of 0x%08" PRIxPTR, actual, expected);
+
+ return false;
}
for (BacktraceMap::const_iterator it = map->begin(); it != map->end(); ++it) {
- CHECK(!(base >= it->start && base < it->end) // start of new within old
- && !(limit > it->start && limit < it->end) // end of new within old
- && !(base <= it->start && limit > it->end)) // start/end of new includes all of old
- << StringPrintf("Requested region 0x%08" PRIxPTR "-0x%08" PRIxPTR " overlaps with "
- "existing map 0x%08" PRIxPTR "-0x%08" PRIxPTR " (%s)\n",
- base, limit,
- static_cast<uintptr_t>(it->start), static_cast<uintptr_t>(it->end),
- it->name.c_str())
- << std::make_pair(it, map->end());
+ if ((expected >= it->start && expected < it->end) // start of new within old
+ || (limit > it->start && limit < it->end) // end of new within old
+ || (expected <= it->start && limit > it->end)) { // start/end of new includes all of old
+ *error_msg
+ << StringPrintf("Requested region 0x%08" PRIxPTR "-0x%08" PRIxPTR " overlaps with "
+ "existing map 0x%08" PRIxPTR "-0x%08" PRIxPTR " (%s)\n",
+ expected, limit,
+ static_cast<uintptr_t>(it->start), static_cast<uintptr_t>(it->end),
+ it->name.c_str())
+ << std::make_pair(it, map->end());
+ return false;
+ }
}
+ *error_msg << StringPrintf("Failed to mmap at expected address, mapped at "
+ "0x%08" PRIxPTR " instead of 0x%08" PRIxPTR, actual, expected);
+ return false;
}
-#else
-static void CheckMapRequest(byte*, size_t) { }
-#endif
-
-MemMap* MemMap::MapAnonymous(const char* name, byte* addr, size_t byte_count, int prot,
+MemMap* MemMap::MapAnonymous(const char* name, byte* expected, size_t byte_count, int prot,
bool low_4gb, std::string* error_msg) {
if (byte_count == 0) {
- return new MemMap(name, NULL, 0, NULL, 0, prot);
+ return new MemMap(name, nullptr, 0, nullptr, 0, prot);
}
size_t page_aligned_byte_count = RoundUp(byte_count, kPageSize);
- CheckMapRequest(addr, page_aligned_byte_count);
#ifdef USE_ASHMEM
// android_os_Debug.cpp read_mapinfo assumes all ashmem regions associated with the VM are
@@ -92,11 +108,11 @@
std::string debug_friendly_name("dalvik-");
debug_friendly_name += name;
ScopedFd fd(ashmem_create_region(debug_friendly_name.c_str(), page_aligned_byte_count));
- int flags = MAP_PRIVATE;
if (fd.get() == -1) {
*error_msg = StringPrintf("ashmem_create_region failed for '%s': %s", name, strerror(errno));
return nullptr;
}
+ int flags = MAP_PRIVATE;
#else
ScopedFd fd(-1);
int flags = MAP_PRIVATE | MAP_ANONYMOUS;
@@ -106,64 +122,80 @@
flags |= MAP_32BIT;
}
#endif
- byte* actual = reinterpret_cast<byte*>(mmap(addr, page_aligned_byte_count, prot, flags, fd.get(), 0));
+
+ void* actual = mmap(expected, page_aligned_byte_count, prot, flags, fd.get(), 0);
+ std::string strerr(strerror(errno));
if (actual == MAP_FAILED) {
std::string maps;
ReadFileToString("/proc/self/maps", &maps);
- *error_msg = StringPrintf("anonymous mmap(%p, %zd, 0x%x, 0x%x, %d, 0) failed\n%s",
- addr, page_aligned_byte_count, prot, flags, fd.get(),
- maps.c_str());
+ *error_msg = StringPrintf("Failed anonymous mmap(%p, %zd, 0x%x, 0x%x, %d, 0): %s\n%s",
+ expected, page_aligned_byte_count, prot, flags, fd.get(),
+ strerr.c_str(), maps.c_str());
return nullptr;
}
- return new MemMap(name, actual, byte_count, actual, page_aligned_byte_count, prot);
+ std::ostringstream check_map_request_error_msg;
+ if (!CheckMapRequest(expected, actual, page_aligned_byte_count, &check_map_request_error_msg)) {
+ *error_msg = check_map_request_error_msg.str();
+ return nullptr;
+ }
+ return new MemMap(name, reinterpret_cast<byte*>(actual), byte_count, actual,
+ page_aligned_byte_count, prot);
}
-MemMap* MemMap::MapFileAtAddress(byte* addr, size_t byte_count, int prot, int flags, int fd,
+MemMap* MemMap::MapFileAtAddress(byte* expected, size_t byte_count, int prot, int flags, int fd,
off_t start, bool reuse, const char* filename,
std::string* error_msg) {
CHECK_NE(0, prot);
CHECK_NE(0, flags & (MAP_SHARED | MAP_PRIVATE));
+ if (reuse) {
+ // reuse means it is okay that it overlaps an existing page mapping.
+ // Only use this if you actually made the page reservation yourself.
+ CHECK(expected != nullptr);
+ flags |= MAP_FIXED;
+ } else {
+ CHECK_EQ(0, flags & MAP_FIXED);
+ }
+
if (byte_count == 0) {
- return new MemMap(filename, NULL, 0, NULL, 0, prot);
+ return new MemMap(filename, nullptr, 0, nullptr, 0, prot);
}
// Adjust 'offset' to be page-aligned as required by mmap.
int page_offset = start % kPageSize;
off_t page_aligned_offset = start - page_offset;
// Adjust 'byte_count' to be page-aligned as we will map this anyway.
size_t page_aligned_byte_count = RoundUp(byte_count + page_offset, kPageSize);
- // The 'addr' is modified (if specified, ie non-null) to be page aligned to the file but not
- // necessarily to virtual memory. mmap will page align 'addr' for us.
- byte* page_aligned_addr = (addr == NULL) ? NULL : (addr - page_offset);
- if (!reuse) {
- // reuse means it is okay that it overlaps an existing page mapping.
- // Only use this if you actually made the page reservation yourself.
- CheckMapRequest(page_aligned_addr, page_aligned_byte_count);
- } else {
- CHECK(addr != NULL);
- }
- byte* actual = reinterpret_cast<byte*>(mmap(page_aligned_addr,
+ // The 'expected' is modified (if specified, ie non-null) to be page aligned to the file but not
+ // necessarily to virtual memory. mmap will page align 'expected' for us.
+ byte* page_aligned_expected = (expected == nullptr) ? nullptr : (expected - page_offset);
+
+ byte* actual = reinterpret_cast<byte*>(mmap(page_aligned_expected,
page_aligned_byte_count,
prot,
flags,
fd,
page_aligned_offset));
+ std::string strerr(strerror(errno));
if (actual == MAP_FAILED) {
- std::string strerr(strerror(errno));
std::string maps;
ReadFileToString("/proc/self/maps", &maps);
*error_msg = StringPrintf("mmap(%p, %zd, 0x%x, 0x%x, %d, %" PRId64
") of file '%s' failed: %s\n%s",
- page_aligned_addr, page_aligned_byte_count, prot, flags, fd,
+ page_aligned_expected, page_aligned_byte_count, prot, flags, fd,
static_cast<int64_t>(page_aligned_offset), filename, strerr.c_str(),
maps.c_str());
- return NULL;
+ return nullptr;
+ }
+ std::ostringstream check_map_request_error_msg;
+ if (!CheckMapRequest(expected, actual, page_aligned_byte_count, &check_map_request_error_msg)) {
+ *error_msg = check_map_request_error_msg.str();
+ return nullptr;
}
return new MemMap(filename, actual + page_offset, byte_count, actual, page_aligned_byte_count,
prot);
}
MemMap::~MemMap() {
- if (base_begin_ == NULL && base_size_ == 0) {
+ if (base_begin_ == nullptr && base_size_ == 0) {
return;
}
int result = munmap(base_begin_, base_size_);
@@ -177,12 +209,12 @@
: name_(name), begin_(begin), size_(size), base_begin_(base_begin), base_size_(base_size),
prot_(prot) {
if (size_ == 0) {
- CHECK(begin_ == NULL);
- CHECK(base_begin_ == NULL);
+ CHECK(begin_ == nullptr);
+ CHECK(base_begin_ == nullptr);
CHECK_EQ(base_size_, 0U);
} else {
- CHECK(begin_ != NULL);
- CHECK(base_begin_ != NULL);
+ CHECK(begin_ != nullptr);
+ CHECK(base_begin_ != nullptr);
CHECK_NE(base_size_, 0U);
}
};
@@ -201,7 +233,7 @@
byte* new_base_end = new_end;
DCHECK_LE(new_base_end, old_base_end);
if (new_base_end == old_base_end) {
- return new MemMap(tail_name, NULL, 0, NULL, 0, tail_prot);
+ return new MemMap(tail_name, nullptr, 0, nullptr, 0, tail_prot);
}
size_ = new_end - reinterpret_cast<byte*>(begin_);
base_size_ = new_base_end - reinterpret_cast<byte*>(base_begin_);
@@ -257,7 +289,7 @@
}
bool MemMap::Protect(int prot) {
- if (base_begin_ == NULL && base_size_ == 0) {
+ if (base_begin_ == nullptr && base_size_ == 0) {
prot_ = prot;
return true;
}
diff --git a/runtime/mem_map_test.cc b/runtime/mem_map_test.cc
index 6cb59b4..eea3307 100644
--- a/runtime/mem_map_test.cc
+++ b/runtime/mem_map_test.cc
@@ -120,6 +120,39 @@
}
#endif
+TEST_F(MemMapTest, MapAnonymousExactAddr) {
+ std::string error_msg;
+ // Map at an address that should work, which should succeed.
+ UniquePtr<MemMap> map0(MemMap::MapAnonymous("MapAnonymous0",
+ reinterpret_cast<byte*>(ART_BASE_ADDRESS),
+ kPageSize,
+ PROT_READ | PROT_WRITE,
+ false,
+ &error_msg));
+ ASSERT_TRUE(map0.get() != nullptr) << error_msg;
+ ASSERT_TRUE(error_msg.empty());
+ ASSERT_TRUE(map0->BaseBegin() == reinterpret_cast<void*>(ART_BASE_ADDRESS));
+ // Map at an unspecified address, which should succeed.
+ UniquePtr<MemMap> map1(MemMap::MapAnonymous("MapAnonymous1",
+ nullptr,
+ kPageSize,
+ PROT_READ | PROT_WRITE,
+ false,
+ &error_msg));
+ ASSERT_TRUE(map1.get() != nullptr) << error_msg;
+ ASSERT_TRUE(error_msg.empty());
+ ASSERT_TRUE(map1->BaseBegin() != nullptr);
+ // Attempt to map at the same address, which should fail.
+ UniquePtr<MemMap> map2(MemMap::MapAnonymous("MapAnonymous2",
+ reinterpret_cast<byte*>(map1->BaseBegin()),
+ kPageSize,
+ PROT_READ | PROT_WRITE,
+ false,
+ &error_msg));
+ ASSERT_TRUE(map2.get() == nullptr) << error_msg;
+ ASSERT_TRUE(!error_msg.empty());
+}
+
TEST_F(MemMapTest, RemapAtEnd) {
RemapAtEndTest(false);
}