Reorder checks in tcmalloc.

third_party/tcmalloc/chromium/src/tcmalloc.cc

 // Copyright (c) 2005, Google Inc.
 // All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 // notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
@@ skipping 1071 matching lines @@
   return result;
 }
 
 inline void* do_malloc(size_t size) {
   AddRoomForMark(&size);
 
   void* ret = NULL;
 
   // 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);
+  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.
+    // 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));
-      }
+    if ((FLAGS_tcmalloc_sample_parameter > 0) &&
+        heap->SampleAllocation(size)) {
+      ret = DoSampledAllocation(size);
+      MarkAllocatedRegion(ret);
     } else {
-      ret = do_malloc_pages(heap, size);
-      MarkAllocatedRegion(ret);
+      // 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;
 }
 
 inline void* do_calloc(size_t n, size_t elem_size) {
   // Overflow check
   const size_t size = n * elem_size;
   if (elem_size != 0 && size / elem_size != n) return NULL;
 
@@ skipping 744 matching lines @@
   *mark = ~allocated_mark;  //  Distinctively not allocated.
 }
 
 static void MarkAllocatedRegion(void* ptr) {
   if (ptr == NULL) return;
   MarkType* mark = GetMarkLocation(ptr);
   *mark = GetMarkValue(ptr, mark);
 }
 
 #endif  // TCMALLOC_VALIDATION