Never unmap memory reserved for RELRO file creation.

base/android/linker/modern_linker_jni.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This is the version of the Android-specific Chromium linker that uses
 // the Android M and later system linker to load libraries.
 
 // This source code *cannot* depend on anything from base/ or the C++
 // STL, to keep the final library small, and avoid ugly dependency issues.
 
@@ skipping 272 matching lines @@
     return false;
   }
 
   *load_size = callback_data.load_size;
   *min_vaddr = callback_data.min_vaddr;
   return true;
 }
 
 // Helper for LoadLibrary(). We reserve an address space larger than
 // needed. After library loading we want to trim that reservation to only
-// what is needed.
-bool ResizeReservedAddressSpace(void* addr,
+// what is needed. Failure to trim should not occur, but if it does then
+// everything will still run, so we treat it as a warning rather than
+// an error.
+void ResizeReservedAddressSpace(void* addr,
                                 size_t reserved_size,
                                 size_t load_size,
                                 size_t min_vaddr) {
   LOG_INFO("Called for %p, reserved %d, loaded %d, min_vaddr %d",
            addr, static_cast<int>(reserved_size),
            static_cast<int>(load_size), static_cast<int>(min_vaddr));
 
   const uintptr_t uintptr_addr = reinterpret_cast<uintptr_t>(addr);
 
-  if (reserved_size < load_size) {
+  if (reserved_size > load_size) {
+    // Unmap the part of the reserved address space that is beyond the end of
+    // the loaded library data.
+    void* unmap = reinterpret_cast<void*>(uintptr_addr + load_size);
+    const size_t length = reserved_size - load_size;
+    if (munmap(unmap, length) == -1) {
+      LOG_ERROR("WARNING: unmap of %d bytes at %p failed: %s",
+                static_cast<int>(length), unmap, strerror(errno));
+    }
+  } else {
     LOG_ERROR("WARNING: library reservation was too small");

[rmcilroy, 2016/01/15 15:56:00]

LOG_WARN ?

[simonb (inactive), 2016/01/15 16:18:18]

Doesn't exist, unfortunately. The linker JNI code has its own cut-down version
of base logging (can't use base directly), and LOG_WARN is one of the missing
bits.

-    return true;
-  }
-
-  // Unmap the part of the reserved address space that is beyond the end of
-  // the loaded library data.
-  void* unmap = reinterpret_cast<void*>(uintptr_addr + load_size);
-  size_t length = reserved_size - load_size;
-  if (munmap(unmap, length) == -1) {
-    LOG_ERROR("Failed to unmap %d at %p", static_cast<int>(length), unmap);
-    return false;
   }
 
 #if RESERVE_BREAKPAD_GUARD_REGION
-  if (min_vaddr > kBreakpadGuardRegionBytes) {
+  if (kBreakpadGuardRegionBytes > min_vaddr) {
+    // Unmap the part of the reserved address space that is ahead of where we
+    // actually need the guard region to start. Resizes the guard region to
+    // min_vaddr bytes.
+    void* unmap =
+        reinterpret_cast<void*>(uintptr_addr - kBreakpadGuardRegionBytes);
+    const size_t length = kBreakpadGuardRegionBytes - min_vaddr;
+    if (munmap(unmap, length) == -1) {
+      LOG_ERROR("WARNING: unmap of %d bytes at %p failed: %s",
+                static_cast<int>(length), unmap, strerror(errno));
+    }
+  } else {
     LOG_ERROR("WARNING: breakpad guard region reservation was too small");
-    return true;
-  }
-
-  // Unmap the part of the reserved address space that is ahead of where we
-  // actually need the guard region to start. Resizes the guard region to
-  // min_vaddr bytes.
-  unmap = reinterpret_cast<void*>(uintptr_addr - kBreakpadGuardRegionBytes);
-  length = kBreakpadGuardRegionBytes - min_vaddr;
-  if (munmap(unmap, length) == -1) {
-    LOG_ERROR("Failed to unmap %d at %p", static_cast<int>(length), unmap);
-    return false;
   }
 #endif
-
-  return true;
 }
 
 // Load a library with the chromium linker, using android_dlopen_ext().
 //
 // android_dlopen_ext() understands how to directly load from a zipfile,
 // based on the format of |dlopen_ext_path|. If it contains a "!/" separator
 // then the string indicates <zip_path>!/<file_path> and indicates the
 // file_path element within the zip file at zip_path. A library in a
 // zipfile must be uncompressed and page aligned. The library is expected
 // to be lib/<abi_tag>/crazy.<basename>. The <abi_tag> used will be the
@@ skipping 67 matching lines @@
   void* handle = nullptr;
   if (!AndroidDlopenExt(path, RTLD_NOW, &dlextinfo, &handle)) {
     LOG_ERROR("No android_dlopen_ext function found");
     return false;
   }
   if (handle == nullptr) {
     LOG_ERROR("android_dlopen_ext: %s", dlerror());
     return false;
   }
 
-  // After loading, trim the mapping to match the library's actual size.
+  // For https://crbug.com/568880.
+  //
+  // Release the scoped mapping. Now that the library has loaded we can no
+  // longer assume we have control of all of this area. libdl knows addr and
+  // has loaded the library into some portion of the reservation. It will
+  // not expect that portion of memory to be arbitrarily unmapped.
+  mapping.Release();
+
+  // After loading we can find the actual size of the library. It should
+  // be less than the space we reserved for it.
   size_t load_size = 0;
   size_t min_vaddr = 0;
   if (!GetLibraryLoadSize(addr, &load_size, &min_vaddr)) {
     LOG_ERROR("Unable to find size for load at %p", addr);
     return false;
   }
-  if (!ResizeReservedAddressSpace(addr, size, load_size, min_vaddr)) {
-    LOG_ERROR("Unable to resize reserved address mapping");
-    return false;
-  }
+
+  // Trim the reservation mapping to match the library's actual size. Failure
+  // to resize is not a fatal error. At worst we lose a portion of virtual
+  // address space that we might otherwise have recovered. Note that trimming
+  // the mapping here requires that we have already released the scoped
+  // mapping.
+  ResizeReservedAddressSpace(addr, size, load_size, min_vaddr);
 
   // Locate and if found then call the loaded library's JNI_OnLoad() function.
   using JNI_OnLoadFunctionPtr = int (*)(void* vm, void* reserved);
   auto jni_onload =
       reinterpret_cast<JNI_OnLoadFunctionPtr>(dlsym(handle, "JNI_OnLoad"));
   if (jni_onload != nullptr) {
     // Check that JNI_OnLoad returns a usable JNI version.
     int jni_version = (*jni_onload)(s_java_vm, nullptr);
     if (jni_version < JNI_VERSION_1_4) {
       LOG_ERROR("JNI version is invalid: %d", jni_version);
       return false;
     }
   }
 
-  // Release mapping before returning so that we do not unmap reserved space.
-  mapping.Release();
-
   // Note the load address and load size in the supplied libinfo object.
   const size_t cast_addr = reinterpret_cast<size_t>(addr);
   s_lib_info_fields.SetLoadInfo(env, lib_info_obj, cast_addr, load_size);
 
   LOG_INFO("Success loading library %s", dlopen_library_path.c_str());
   return true;
 }
 
 // Create a shared RELRO file for a library, using android_dlopen_ext().
 //
@@ skipping 64 matching lines @@
     LOG_ERROR("No android_dlopen_ext function found");
     close(relro_fd);
     return false;
   }
   if (handle == nullptr) {
     LOG_ERROR("android_dlopen_ext: %s", dlerror());
     close(relro_fd);
     return false;
   }
 
-  // Unload the library from this address. The reserved space is
-  // automatically unmapped on exit from this function.
+  // For https://crbug.com/568880.
+  //
+  // Release the scoped mapping. See comment in LoadLibrary() above for more.

[rmcilroy, 2016/01/15 15:56:00]

Isn't this covered by the comment below on dlclose ?

[simonb (inactive), 2016/01/15 16:18:18]

Sort of, but also for reasons in LoadLibrary(), which are valid in addition to
the note before dlclose().

+  mapping.Release();
+
+  // For https://crbug.com/568880.
+  //
+  // Unload the library from this address. Calling dlclose() will unmap the
+  // part of the reservation occupied by the libary, but will leave the
+  // remainder of the reservation mapped, and we have no effective way of
+  // unmapping the leftover portions because we don't know where dlclose's
+  // unmap ended.
+  //
+  // For now we live with this. It is a loss of some virtual address space
+  // (but not actual memory), and because it occurs only once and only in
+  // the browser process, and never in renderer processes, it is not a
+  // significant issue.

[rmcilroy, 2016/01/15 15:56:00]

Please add the TODO here as discussed.

[simonb (inactive), 2016/01/15 16:18:18]

Done.

   dlclose(handle);
 
-  // Reopen the shared RELFO fd in read-only mode. This ensures that nothing
+  // Reopen the shared RELRO fd in read-only mode. This ensures that nothing
   // can write to it through the RELRO fd that we return in libinfo.
   close(relro_fd);
   relro_fd = open(filepath, O_RDONLY);
   if (relro_fd == -1) {
     LOG_ERROR("open: %s: %s", filepath, strerror(errno));
     return false;
   }
 
   // Delete the directory entry for the RELRO file. The fd we hold ensures
   // that its data remains intact.
@@ skipping 54 matching lines @@
                        kNativeMethods,
                        sizeof(kNativeMethods) / sizeof(kNativeMethods[0]));
 
   // Record the Java VM handle.
   s_java_vm = vm;
 
   return true;
 }
 
 }  // namespace chromium_android_linker