Make mojo buffer allocators more robust.

mojo/public/cpp/bindings/lib/scratch_buffer.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "mojo/public/cpp/bindings/lib/scratch_buffer.h"
 
 #include <assert.h>
 #include <stdlib.h>
 #include <string.h>
 
@@ skipping 28 matching lines @@
     Segment* doomed = overflow_;
     overflow_ = overflow_->next;
     DEBUG_SCRUB(doomed, doomed->end - reinterpret_cast<char*>(doomed));
     free(doomed);
   }
   DEBUG_SCRUB(fixed_data_, sizeof(fixed_data_));
 }
 
 void* ScratchBuffer::Allocate(size_t delta, Destructor func) {
   delta = internal::Align(delta);
+  void* result = AllocateInSegment(&fixed_, delta);
+  if (!result && overflow_)
+    result = AllocateInSegment(overflow_, delta);
 
-  void* result = AllocateInSegment(&fixed_, delta);
-  if (!result) {
-    if (overflow_)
-      result = AllocateInSegment(overflow_, delta);
+  if (!result && AddOverflowSegment(delta))
+    result = AllocateInSegment(overflow_, delta);
 
-    if (!result) {
-      AddOverflowSegment(delta);
-      result = AllocateInSegment(overflow_, delta);
-    }
-  }
-
-  if (func) {
+  if (func && result) {

[darin (slow to review), 2014/04/09 04:52:42]

I'm pretty sure the bindings don't respond kindly to Buffer::Allocate returning
NULL. That might something to look at as well.

     PendingDestructor dtor;
     dtor.func = func;
     dtor.address = result;
     pending_dtors_.push_back(dtor);
   }
   return result;
 }
 
 void* ScratchBuffer::AllocateInSegment(Segment* segment, size_t delta) {
-  void* result;
   if (static_cast<size_t>(segment->end - segment->cursor) >= delta) {
-    result = segment->cursor;
-    memset(result, 0, delta);
+    void* result = segment->cursor;
+    memset(result, 0, delta);  // Required to avoid info leaks.
     segment->cursor += delta;
-  } else {
-    result = NULL;
+    return result;
   }
-  return result;
+  return NULL;
 }
 
-void ScratchBuffer::AddOverflowSegment(size_t delta) {
+bool ScratchBuffer::AddOverflowSegment(size_t delta) {
   if (delta < kMinSegmentSize)
     delta = kMinSegmentSize;
 
+  if (delta > kMaxSegmentSize)
+    return false;
+
   // Ensure segment buffer is aligned.
   size_t segment_size = internal::Align(sizeof(Segment)) + delta;
+  Segment* segment = static_cast<Segment*>(malloc(segment_size));
+  if (segment) {

[darin (slow to review), 2014/04/09 04:52:42]

I guess you are doing this because this code shouldn't count on malloc aborting
the program if it fails to allocate memory? Makes sense.

+    segment->next = overflow_;
+    segment->cursor = reinterpret_cast<char*>(segment + 1);
+    segment->end = segment->cursor + delta;
+    overflow_ = segment;
+    return true;
+  }
 
-  Segment* segment = static_cast<Segment*>(malloc(segment_size));
-  segment->next = overflow_;
-  segment->cursor = reinterpret_cast<char*>(segment + 1);
-  segment->end = segment->cursor + delta;
-
-  overflow_ = segment;
+  return false;
 }
 
 }  // namespace internal
 }  // namespace mojo