TCMalloc: restrict maximum size of memory ranges

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 b755b3fdc9bb3e0d34e644b03b7e16582a3aa61a..fc832d84825f4278bb13bb1e8007d895b490514d 100644
--- a/third_party/tcmalloc/chromium/src/system-alloc.cc
+++ b/third_party/tcmalloc/chromium/src/system-alloc.cc
@@ -47,6 +47,7 @@
 #ifdef HAVE_UNISTD_H
 #include <unistd.h>                     // for sbrk, getpagesize, off_t
 #endif
+#include <limits>
 #include <new>                          // for operator new
 #include <gperftools/malloc_extension.h>
 #include "base/basictypes.h"
@@ -208,6 +209,10 @@ void* SbrkSysAllocator::Alloc(size_t size, size_t *actual_size,
     return NULL;
   }
 
+  // The first time we call into the allocator, record the current pointer
+  // to the break. This will be used to calculate the total size.
+  static const char* initial_brk_address = static_cast<char*>(sbrk(0));
+
   // sbrk will release memory if passed a negative number, so we do
   // a strict check here
   if (static_cast<ptrdiff_t>(size + alignment) < 0) return NULL;
@@ -229,10 +234,20 @@ void* SbrkSysAllocator::Alloc(size_t size, size_t *actual_size,
   //    http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libc/port/sys/sbrk.c?a=true
   //    http://sourceware.org/cgi-bin/cvsweb.cgi/~checkout~/libc/misc/sbrk.c?rev=1.1.2.1&content-type=text/plain&cvsroot=glibc
   // Without this check, sbrk may succeed when it ought to fail.)
-  if (reinterpret_cast<intptr_t>(sbrk(0)) + size < size) {
+  const char* current_brk_address = static_cast<char*>(sbrk(0));
+  if (reinterpret_cast<intptr_t>(current_brk_address) + size < size) {

[Chris Evans, 2013/01/11 19:51:51]

Ooh... this was here before, but intptr_t is a signed long int usually. It'll
get cast correctly to unsigned due to addition to a size_t, but still... maybe
use uintptr_t ?

     return NULL;
   }
 
+  ASSERT(current_brk_address >= initial_brk_address);
+  const size_t current_alloc_size =
+      static_cast<size_t>(current_brk_address - initial_brk_address);
+  ASSERT(current_alloc_size <=
+         std::numeric_limits<std::size_t>::max() - size);
+
+  if (!tcmalloc::IsContiguousAllocSizePermitted(current_alloc_size + size))
+    return NULL;

[Chris Evans, 2013/01/11 19:51:51]

This seems like a very low-level way to accomplish this.

In terms of the pattern of calls during the vulnerability we're worried about,
we'll likely see a realloc() with a size parameter > INT_MAX.

Why aren't we just adding a check for the individual size parameter at malloc()
and realloc() etc. It seems simpler?

[jln (very slow on Chromium), 2013/01/11 20:02:04]

The change to GrowHeap should take care of that in pageheap.cc.

The reason for dealing with the brk() heap directly is that it's the only
allocator that will generate contiguous mappings. I didn't land the test for
this yet, but I thought we didn't want multiple small malloc() to generate a 4GB
slab in any case. This would make sure that array[negative_int] has a great
change to segfault right away.

+
   void* result = sbrk(size);
   if (result == reinterpret_cast<void*>(-1)) {
     return NULL;