Linux: grow a unique random mapping in ASLR

third_party/tcmalloc/chromium/src/system-alloc.cc

Index: third_party/tcmalloc/chromium/src/system-alloc.cc
diff --git a/third_party/tcmalloc/chromium/src/system-alloc.cc b/third_party/tcmalloc/chromium/src/system-alloc.cc
index 05338963505152ceb5301030f9762c2727ea95b4..50f611fc503f48541fdc4d761e1e5e7b9f94499a 100644
--- a/third_party/tcmalloc/chromium/src/system-alloc.cc
+++ b/third_party/tcmalloc/chromium/src/system-alloc.cc
@@ -193,6 +193,33 @@ void* GetRandomAddrHint() {
 #endif  // ASLR_IS_SUPPORTED
 }
 
+// Allocate |length| bytes of memory using mmap(). The memory will be
+// readable and writeable, but not executable.
+// Like mmap(), we will return MAP_FAILED on failure.
+// |is_aslr_enabled| controls address space layout randomization. When true, we
+// will put the first mapping at a random address and will then try to grow it.
+// If it's not possible to grow an existing mapping, a new one will be created.
+void* AllocWithMmap(size_t length, bool is_aslr_enabled) {
+  static void* address_hint = NULL;

[Chris Evans, 2013/02/01 09:22:30]

What's the threading story here? Is it possible for two threads to be in here at
once? If not, maybe a comment declaring the lock that protects this?

[jln (very slow on Chromium), 2013/02/01 09:53:10]

Done.

+  if (!address_hint && is_aslr_enabled) {
+    address_hint = GetRandomAddrHint();
+  }

[Chris Evans, 2013/02/01 09:22:30]

Style: don't need braces for single-line if with single-line body.

[jln (very slow on Chromium), 2013/02/01 09:53:10]

The style guide is open on this. I usually always put them, I've been bitten by
it before.

But since I didn't do it below, you're right that I should be consistent.

+  void* result = mmap(address_hint, length, PROT_READ|PROT_WRITE,

[Chris Evans, 2013/02/01 09:22:30]

Maybe some form of comment that the intent here is to map right at the end of
the previous mapping, leading to the extension of the existing VMA instead of a
new VMA?

[jln (very slow on Chromium), 2013/02/01 09:53:10]

Isn't the comment below good enough ? Should I move the comment here?

+                      MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
+  if (result != static_cast<void*>(MAP_FAILED) && is_aslr_enabled) {

[Chris Evans, 2013/02/01 09:22:30]

I think you can simplify this block to simply

if (result != address_hint)
  address_hint = NULL;

It'll work in the MAP_FAILED case and reset the hint for _any_ VMA collision.

[jln (very slow on Chromium), 2013/02/01 09:53:10]

But is that what we want ? If the goal is to grow existing areas as much as
possible, this is not optimal.

I was already tempted to do it, so you convinced me. It's closer to the balance
we'll eventually get when we start skewing this more towards security later.

+    // If mmap() succeeded at a random address, our next mmap() will try to grow
+    // the current mapping.
+    // This has been done for performance reasons, see crbug.com/173371.
+    // It should be possible to strike a better balance between performance
+    // and security but will be done at a later date.
+    address_hint = static_cast<char*>(result) + length;
+    if (address_hint < result)
+      address_hint = NULL;
+    ASSERT((reinterpret_cast<uintptr_t>(address_hint) & 0xfff) == 0);
+  }
+  return result;
+}
+
 }  // Anonymous namespace to avoid name conflicts on "CheckAddressBits".
 
 COMPILE_ASSERT(kAddressBits <= 8 * sizeof(void*),
@@ -405,14 +432,7 @@ void* MmapSysAllocator::Alloc(size_t size, size_t *actual_size,
   //            size + alignment < (1<<NBITS).
   // and        extra <= alignment
   // therefore  size + extra < (1<<NBITS)
-  void* address_hint = NULL;
-  if (FLAGS_malloc_random_allocator) {
-    address_hint = GetRandomAddrHint();
-  }
-  void* result = mmap(address_hint, size + extra,
-                      PROT_READ|PROT_WRITE,
-                      MAP_PRIVATE|MAP_ANONYMOUS,
-                      -1, 0);
+  void* result = AllocWithMmap(size + extra, FLAGS_malloc_random_allocator);
   if (result == reinterpret_cast<void*>(MAP_FAILED)) {
     return NULL;
   }