Reorder checks in tcmalloc.

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 48e71c228ba4feb3396d9aefc0fa14bba4a65ebd..e0b7908ed20663c389a5e7893040e6e685e4b614 100644
--- a/third_party/tcmalloc/chromium/src/tcmalloc.cc
+++ b/third_party/tcmalloc/chromium/src/tcmalloc.cc
@@ -1089,31 +1089,28 @@ inline void* do_malloc(size_t size) {
 
   // The following call forces module initialization
   ThreadCache* heap = ThreadCache::GetCache();
-  // First, check if our security policy allows this size.
-  if (IsAllocSizePermitted(size)) {
-    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 {
-      ret = do_malloc_pages(heap, size);
+  if (size <= kMaxSize && IsAllocSizePermitted(size)) {

[jar (doing other things), 2013/03/15 23:00:47]

It would probably be clearer to not even risk calling IsAllocSizePermitted()
here, and instead put a DEBUG assertion

ASSERT(IsAllocSizePermitted(size)); 

in front of line 1093.  This way you don't have to trust the compiler (and the
meaning of the code is clearer).

[jln (very slow on Chromium), 2013/03/17 10:21:51]

For security, I would really be wary of making anything debug only. This check
is useful when an attacker is trying to "trick" something into allocating a
large object to exploit vulnerabilities such as the prevalent "int instead of
size_t" on 64 bits Chrome.
Making it a debug check won't work for security.

I would rather leave the check and hope that the compiler makes the right
decision, and if it doesn't, have a small perf impact rather than a security
vulnerability.

If you wish, we could instead have a DCHECK inside IsAllocSizePermitted()
instead, for size > kMaxSize, with an explaining comment.

+    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.

[jar (doing other things), 2013/03/15 23:00:47]

...but this line of optimization points a scary finger at me here.  Is this
costly, or is it free, since we have the struct in memory via a cache line? 
Inquiring minds want to know.  If it is not expensive, you could clobber my
paranoid comment (inserted when I added the instrumentation).

+
+    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 if (IsAllocSizePermitted(size)) {
+    ret = do_malloc_pages(heap, size);
+    MarkAllocatedRegion(ret);
   }
   if (ret == NULL) errno = ENOMEM;
   return ret;