Fix GPU Ring Buffer alignment crash

Fixes an issue in gpu::RingBuffer where requesting an Alloc with the
value returned by RingBuffer::GetLargestFreeOrPendingSize would result
in trying to allocate more memory than the maximum buffer size if the
ring buffer's size was not aligned.

Bug: 834444
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I672f67944a1ae4c5e34fe5b6d9f209f4a9112400
Reviewed-on: https://chromium-review.googlesource.com/1018525
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Brandon Jones <bajones@chromium.org>
Reviewed-by: Bill Orr <billorr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552118}

gpu/command_buffer/client/ring_buffer.cc

@@ -64,6 +64,8 @@ void* RingBuffer::Alloc(unsigned int size) {
   // Allocate rounded to alignment size so that the offsets are always
   // memory-aligned.
   size = RoundToAlignment(size);
+  DCHECK_LE(size, size_)
+      << "attempt to allocate more than maximum memory after rounding";
 
   // Wait until there is enough room.
   while (size > GetLargestFreeSizeNoWaitingInternal()) {
@@ -197,6 +199,8 @@ void RingBuffer::ShrinkLastBlock(unsigned int new_size) {
   // Allocate rounded to alignment size so that the offsets are always
   // memory-aligned.
   new_size = RoundToAlignment(new_size);
+  if (new_size == block.size)
+    return;
   free_offset_ = block.offset + new_size;
   block.size = new_size;
 }

gpu/command_buffer/client/ring_buffer.h

@@ -74,7 +74,10 @@ class GPU_EXPORT RingBuffer {
   // caller can wait. Allocating a block of this size will succeed, but may
   // block.
   unsigned int GetLargestFreeOrPendingSize() {
-    return size_;
+    // If size_ is not a multiple of alignment_, then trying to allocate it will
+    // cause us to try to allocate more than we actually can due to rounding up.
+    // So, round down here.
+    return size_ - size_ % alignment_;
   }
 
   // Gets a pointer to a memory block given the base memory and the offset.

gpu/command_buffer/client/ring_buffer_test.cc

@@ -135,6 +135,20 @@ TEST_F(RingBufferTest, TestBasic) {
   allocator_->FreePendingToken(pointer, token);
 }
 
+// Checks that the value returned by GetLargestFreeOrPendingSize can actually be
+// allocated. This is a regression test for https://crbug.com/834444, where an
+// unaligned buffer could cause an alloc using the value returned by
+// GetLargestFreeOrPendingSize to try to allocate more memory than was allowed.
+TEST_F(RingBufferTest, TestCanAllocGetLargestFreeOrPendingSize) {
+  // Make sure we aren't actually aligned
+  buffer_.reset(new int8_t[kBufferSize + 2 + kBaseOffset]);
+  buffer_start_ = buffer_.get() + kBaseOffset;
+  allocator_.reset(new RingBuffer(kAlignment, kBaseOffset, kBufferSize + 2,
+                                  helper_.get(), buffer_start_));
+  void* pointer = allocator_->Alloc(allocator_->GetLargestFreeOrPendingSize());
+  allocator_->FreePendingToken(pointer, helper_->InsertToken());
+}
+
 // Checks the free-pending-token mechanism.
 TEST_F(RingBufferTest, TestFreePendingToken) {
   const unsigned int kSize = 16;

gpu/command_buffer/client/transfer_buffer.cc

@@ -142,6 +142,8 @@ static unsigned int ComputePOTSize(unsigned int dimension) {
 void TransferBuffer::ReallocateRingBuffer(unsigned int size) {
   // What size buffer would we ask for if we needed a new one?
   unsigned int needed_buffer_size = ComputePOTSize(size + result_size_);
+  DCHECK_EQ(needed_buffer_size % alignment_, 0u)
+      << "Buffer size is not a multiple of alignment_";
   needed_buffer_size = std::max(needed_buffer_size, min_buffer_size_);
   needed_buffer_size = std::max(needed_buffer_size, default_buffer_size_);
   needed_buffer_size = std::min(needed_buffer_size, max_buffer_size_);