TCMalloc: restrict maximum size of memory ranges

third_party/tcmalloc/chromium/src/tcmalloc.cc

Index: third_party/tcmalloc/chromium/src/tcmalloc.cc
diff --git a/third_party/tcmalloc/chromium/src/tcmalloc.cc b/third_party/tcmalloc/chromium/src/tcmalloc.cc
index 2ff2a3b47ed6f4c82d0de1446d8eaac4318ea36d..591c687983208b18305d8d6db3d350029a9fc852 100644
--- a/third_party/tcmalloc/chromium/src/tcmalloc.cc
+++ b/third_party/tcmalloc/chromium/src/tcmalloc.cc
@@ -296,6 +296,16 @@ size_t InvalidGetAllocatedSize(const void* ptr) {
       "Attempt to get the size of an invalid pointer", ptr);
   return 0;
 }
+
+// For security reasons, we want to limit the size of allocations.
+// See crbug.com/169327.
+inline bool IsAllocSizePermitted(size_t alloc_size) {
+  // Never allow an allocation larger than what can be indexed via an int.
+  // Remove kPageSize to account for various rounding, padding and to have a
+  // small margin.
+  return alloc_size <= ((std::numeric_limits<int>::max)() - kPageSize);
+}
+
 }  // unnamed namespace
 
 // Extract interesting stats
@@ -1078,27 +1088,31 @@ inline void* do_malloc(size_t size) {
 
   // The following call forces module initialization
   ThreadCache* heap = ThreadCache::GetCache();
-  if (size <= kMaxSize) {
-    size_t cl = Static::sizemap()->SizeClass(size);
-    size = Static::sizemap()->class_to_size(cl);
-
-    // TODO(jar): If this has any detectable performance impact, it can be
-    // optimized by only tallying sizes if the profiler was activated to recall
-    // these tallies.  I don't think this is performance critical, but we really
-    // should measure it.
-    heap->AddToByteAllocatedTotal(size);  // Chromium profiling.
-
-    if ((FLAGS_tcmalloc_sample_parameter > 0) && heap->SampleAllocation(size)) {
-      ret = DoSampledAllocation(size);
-      MarkAllocatedRegion(ret);
+  // First, check if our security policy allows this size.
+  if (IsAllocSizePermitted(size)) {

[Chris Evans, 2013/01/15 02:13:32]

Move the test so it's in the "else" branch of "size <= kMaxSize"
It'll avoid a test/branch on the hot path.

[jln (very slow on Chromium), 2013/01/15 03:10:57]

In release mode, here is the assembly:

cmp     r14, 7FFFEFFFh
ja      loc_14F4
cmp     r14, 8000h
ja      loc_1206

As you can see, it does get inlined, but we don't issue branch prediction hints.
(Apparently modern Intel processors don't even honor such hints anymore).

If we really care about this I think we should rather give hints to the compiler
as to which one is the likely branch (and as you can see, as compiled, we
already don't take any branch anyways). But it seems very unlikely to have any
difference on a modern architecture. And if we care about that level of
optimization, we should optimize GetCache() above, which has more impact.

+    if (size <= kMaxSize) {
+      size_t cl = Static::sizemap()->SizeClass(size);
+      size = Static::sizemap()->class_to_size(cl);
+
+      // TODO(jar): If this has any detectable performance impact, it can be
+      // optimized by only tallying sizes if the profiler was activated to
+      // recall these tallies.  I don't think this is performance critical, but
+      // we really should measure it.
+      heap->AddToByteAllocatedTotal(size);  // Chromium profiling.
+
+      if ((FLAGS_tcmalloc_sample_parameter > 0) &&
+          heap->SampleAllocation(size)) {
+        ret = DoSampledAllocation(size);
+        MarkAllocatedRegion(ret);
+      } else {
+        // The common case, and also the simplest.  This just pops the
+        // size-appropriate freelist, after replenishing it if it's empty.
+        ret = CheckMallocResult(heap->Allocate(size, cl));
+      }
     } else {
-      // The common case, and also the simplest.  This just pops the
-      // size-appropriate freelist, after replenishing it if it's empty.
-      ret = CheckMallocResult(heap->Allocate(size, cl));
+      ret = do_malloc_pages(heap, size);
+      MarkAllocatedRegion(ret);
     }
-  } else {
-    ret = do_malloc_pages(heap, size);
-    MarkAllocatedRegion(ret);
   }
   if (ret == NULL) errno = ENOMEM;
   return ret;
@@ -1233,8 +1247,8 @@ inline void* do_realloc_with_callback(
   //    . If we need to grow, grow to max(new_size, old_size * 1.X)
   //    . Don't shrink unless new_size < old_size * 0.Y
   // X and Y trade-off time for wasted space.  For now we do 1.25 and 0.5.
-  const int lower_bound_to_grow = old_size + old_size / 4;
-  const int upper_bound_to_shrink = old_size / 2;
+  const size_t lower_bound_to_grow = old_size + old_size / 4;

[jar (doing other things), 2013/01/15 20:06:00]

These two lines should be upstreamed to google3.

+  const size_t upper_bound_to_shrink = old_size / 2;
   if ((new_size > old_size) || (new_size < upper_bound_to_shrink)) {
     // Need to reallocate.
     void* new_ptr = NULL;