From d91775bf59f883b75cce2071ac8b76f7aee062a2 Mon Sep 17 00:00:00 2001 From: simonb Date: Wed, 1 Jul 2015 09:30:51 -0700 Subject: [PATCH] crazy linker: Add a Breakpad "guard region" to reserved space. Breakpad requires currently that a library's reported start_addr actually be its load_bias. It will also complain if there is an apparent overlap in the memory mappings it reads in from a microdump. If something in the process mmaps into the address space between load_bias and start_addr, this will break Breakpad's minidump processor. Work round by adding a "guard region" into the reserved address space but ahead of the start_addr where the library is loaded. Making this part of the reserved address space for the library ensures that nothing will later mmap into it. BUG=504410,499747 Review URL: https://codereview.chromium.org/1218493004 Cr-Commit-Position: refs/heads/master@{#337035} --- base/android/linker/linker_jni.cc | 21 ++++++ third_party/android_crazy_linker/README.chromium | 2 + .../src/src/crazy_linker_elf_loader.cpp | 82 +++++++++++++++++++--- .../src/src/crazy_linker_elf_loader.h | 3 + 4 files changed, 100 insertions(+), 8 deletions(-) diff --git a/base/android/linker/linker_jni.cc b/base/android/linker/linker_jni.cc index 2bc480cf776d..d464b6d661ff 100644 --- a/base/android/linker/linker_jni.cc +++ b/base/android/linker/linker_jni.cc @@ -22,6 +22,13 @@ #include #include +// See commentary in crazy_linker_elf_loader.cpp for the effect of setting +// this. If changing there, change here also. +// +// For more, see: +// https://crbug.com/504410 +#define RESERVE_BREAKPAD_GUARD_REGION 1 + // Set this to 1 to enable debug traces to the Android log. // Note that LOG() from "base/logging.h" cannot be used, since it is // in base/ which hasn't been loaded yet. @@ -626,6 +633,13 @@ jboolean CanUseSharedRelro(JNIEnv* env, jclass clazz) { } jlong GetRandomBaseLoadAddress(JNIEnv* env, jclass clazz, jlong bytes) { +#if RESERVE_BREAKPAD_GUARD_REGION + // Add a Breakpad guard region. 16Mb should be comfortably larger than + // the largest relocation packer saving we expect to encounter. + static const size_t kBreakpadGuardRegionBytes = 16 * 1024 * 1024; + bytes += kBreakpadGuardRegionBytes; +#endif + void* address = mmap(NULL, bytes, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if (address == MAP_FAILED) { @@ -633,6 +647,13 @@ jlong GetRandomBaseLoadAddress(JNIEnv* env, jclass clazz, jlong bytes) { return 0; } munmap(address, bytes); + +#if RESERVE_BREAKPAD_GUARD_REGION + // Allow for a Breakpad guard region ahead of the returned address. + address = reinterpret_cast( + reinterpret_cast(address) + kBreakpadGuardRegionBytes); +#endif + LOG_INFO("%s: Random base load address is %p\n", __FUNCTION__, address); return static_cast(reinterpret_cast(address)); } diff --git a/third_party/android_crazy_linker/README.chromium b/third_party/android_crazy_linker/README.chromium index 4def4036bea5..9fb91b6e9ed2 100644 --- a/third_party/android_crazy_linker/README.chromium +++ b/third_party/android_crazy_linker/README.chromium @@ -84,3 +84,5 @@ Local Modifications: - Change relocation packing constant names for C++ style (cosmetic only). +- Add a Breakpad "guard region" to the start of reserved address space. + diff --git a/third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp b/third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp index e265ac39b03d..bf5ec4e76672 100644 --- a/third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp +++ b/third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.cpp @@ -21,6 +21,34 @@ namespace crazy { MAYBE_MAP_FLAG((x), PF_R, PROT_READ) | \ MAYBE_MAP_FLAG((x), PF_W, PROT_WRITE)) +// Whether to add a Breakpad guard region. Breakpad currently requires +// a library's reported start_addr to be equal to its load_bias. It will +// also complain if there is an apparent overlap in the memory mappings +// it reads in from a microdump. So if load_bias and start_addr differ +// due to relocation packing, and if something in the process mmaps into +// the address space between load_bias and start_addr, this will break +// Breakpad's minidump processor. +// +// For now, the expedient workround is to add a "guard region" ahead of +// the start_addr where a library with a non-zero min vaddr is loaded. +// Making this part of the reserved address space for the library ensures +// that nothing will later mmap into it. +// +// Note: ~SharedLibrary() calls munmap() on the values returned from here +// through load_start() and load_size(). Where we added a guard region, +// these values do not cover the entire reserved mapping. The result is +// that we potentially 'leak' between 2Mb and 6Mb of virtual address space +// if we unload a library in a controlled fashion. For now we live with +// this, since a) we do not unload libraries on Android targets, and b) any +// leakage that might occur if we did is small and should not consume any +// real memory. +// +// Also defined in linker_jni.cc. If changing here, change there also. +// +// For more, see: +// https://crbug.com/504410 +#define RESERVE_BREAKPAD_GUARD_REGION 1 + ElfLoader::ElfLoader() : fd_(), path_(NULL), @@ -33,7 +61,9 @@ ElfLoader::ElfLoader() load_start_(NULL), load_size_(0), load_bias_(0), - loaded_phdr_(NULL) {} + loaded_phdr_(NULL), + reserved_start_(NULL), + reserved_size_(0) {} ElfLoader::~ElfLoader() { if (phdr_mmap_) { @@ -91,8 +121,8 @@ bool ElfLoader::LoadAt(const char* lib_path, if (!LoadSegments(error) || !FindPhdr(error)) { // An error occured, cleanup the address space by un-mapping the // range that was reserved by ReserveAddressSpace(). - if (load_start_ && load_size_) - munmap(load_start_, load_size_); + if (reserved_start_ && reserved_size_) + munmap(reserved_start_, reserved_size_); return false; } @@ -201,20 +231,56 @@ bool ElfLoader::ReserveAddressSpace(Error* error) { addr = static_cast(wanted_load_address_); } - LOG("%s: address=%p size=%p\n", __FUNCTION__, addr, load_size_); - void* start = mmap(addr, load_size_, PROT_NONE, mmap_flags, -1, 0); + reserved_size_ = load_size_; + +#if RESERVE_BREAKPAD_GUARD_REGION + // Increase size to extend the address reservation mapping so that it will + // also include a min_vaddr bytes region from load_bias_ to start_addr. + // If loading at a fixed address, move our requested address back by the + // guard region size. + if (min_vaddr) { + reserved_size_ += min_vaddr; + if (wanted_load_address_) { + addr -= min_vaddr; + } + LOG("%s: added %d to size, for Breakpad guard\n", __FUNCTION__, min_vaddr); + } +#endif + + LOG("%s: address=%p size=%p\n", __FUNCTION__, addr, reserved_size_); + void* start = mmap(addr, reserved_size_, PROT_NONE, mmap_flags, -1, 0); if (start == MAP_FAILED) { - error->Format("Could not reserve %d bytes of address space", load_size_); + error->Format("Could not reserve %d bytes of address space", + reserved_size_); return false; } - if (wanted_load_address_ && start != addr) { + if (addr && start != addr) { error->Format("Could not map at %p requested, backing out", addr); - munmap(start, load_size_); + munmap(start, reserved_size_); return false; } + reserved_start_ = start; + LOG("%s: reserved start=%p\n", __FUNCTION__, reserved_start_); + load_start_ = start; load_bias_ = reinterpret_cast(start) - min_vaddr; + +#if RESERVE_BREAKPAD_GUARD_REGION + // If we increased size to accommodate a Breakpad guard region, move + // load_start_ and load_bias_ upwards by the size of the guard region. + // File data is mapped at load_start_, and while nothing ever uses the + // guard region between load_bias_ and load_start_, the fact that we + // included it in the mmap() above means that nothing else will map it. + if (min_vaddr) { + load_start_ = reinterpret_cast( + reinterpret_cast(load_start_) + min_vaddr); + load_bias_ += min_vaddr; + LOG("%s: moved map by %d, for Breakpad guard\n", __FUNCTION__, min_vaddr); + } +#endif + + LOG("%s: load start=%p, bias=%p\n", __FUNCTION__, load_start_, load_bias_); return true; } diff --git a/third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.h b/third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.h index ac5b9bc0fd81..1c3f473da5c7 100644 --- a/third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.h +++ b/third_party/android_crazy_linker/src/src/crazy_linker_elf_loader.h @@ -73,6 +73,9 @@ class ElfLoader { const ELF::Phdr* loaded_phdr_; // points to the loaded program header. + void* reserved_start_; // Real first page of reserved address space. + size_t reserved_size_; // Real size in bytes of reserved address space. + // Individual steps used by ::LoadAt() bool ReadElfHeader(Error* error); bool ReadProgramHeader(Error* error); -- 2.11.4.GIT