Fix PartialCircularBuffer OOB memcpy().

chrome/common/partial_circular_buffer.cc

 // Copyright 2013 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 "chrome/common/partial_circular_buffer.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 
@@ skipping 43 matching lines @@
     DCHECK_LT(buffer_data_->wrap_position, data_size_);
     position_ = buffer_data_->end_position;
   } else {
     DCHECK_LT(wrap_position, data_size_);
     buffer_data_->total_written = 0;
     buffer_data_->wrap_position = wrap_position;
     buffer_data_->end_position = 0;
   }
 }
 
 uint32 PartialCircularBuffer::Read(void* buffer, uint32 buffer_size) {

[Nico, 2015/04/08 23:13:05]

Ah, hmm, this doesn't allow reading more than the buffer size, I see. So if I
write 100 bytes to the buffer (with your patch) and the buffer size is only 6,
I'll only get 6 bytes back. That makes sense.

   DCHECK(buffer_data_);
   if (total_read_ >= buffer_data_->total_written)
     return 0;
 
   uint8* buffer_uint8 = reinterpret_cast<uint8*>(buffer);
   uint32 read = 0;
 
   // Read from beginning part.
   if (position_ < buffer_data_->wrap_position) {
     uint32 to_wrap_pos = buffer_data_->wrap_position - position_;
@@ skipping 57 matching lines @@
   position_ += to_read;
   total_read_ += to_read;
   read += to_read;
   DCHECK_LE(read, buffer_size);
   DCHECK_LE(total_read_, buffer_data_->total_written);
   return read;
 }
 
 void PartialCircularBuffer::Write(const void* buffer, uint32 buffer_size) {
   DCHECK(buffer_data_);
-  uint32 position_before_write = position_;
+  const uint8* input = static_cast<const uint8*>(buffer);
+  uint32 wrap_position = buffer_data_->wrap_position;
+  uint32 cycle_size = data_size_ - wrap_position;
 
-  uint32 to_eof = data_size_ - position_;
-  uint32 to_write = std::min(buffer_size, to_eof);
-  DoWrite(buffer_data_->data + position_, buffer, to_write);
-  if (position_ >= data_size_) {
-    DCHECK_EQ(position_, data_size_);
-    position_ = buffer_data_->wrap_position;
+  // First write the non-wrapping part.
+  if (position_ < wrap_position) {
+    uint32 space_left = wrap_position - position_;
+    uint32 write_size = std::min(buffer_size, space_left);
+    DoWrite(input, write_size);
+    input += write_size;
+    buffer_size -= write_size;
   }
 
-  if (to_write < buffer_size) {
-    uint32 remainder_to_write = buffer_size - to_write;
-    DCHECK_LT(position_, position_before_write);
-    DCHECK_LE(position_ + remainder_to_write, position_before_write);
-    DoWrite(buffer_data_->data + position_,
-            reinterpret_cast<const uint8*>(buffer) + to_write,
-            remainder_to_write);
+  // Skip the part that would overlap.
+  if (buffer_size > cycle_size) {
+    uint32 skip = buffer_size - cycle_size;
+    input += skip;
+    buffer_size -= skip;
+    position_ = wrap_position + (position_ - wrap_position + skip) % cycle_size;
+  }
+
+  // Finally write the wrapping part.

[Nico, 2015/04/08 23:13:05]

Say that this loop will run at most twice.

[gzobqq, 2015/04/09 08:36:40]

Done.

+  while (buffer_size > 0) {
+    uint32 written = DoWrite(input, buffer_size);
+    input += written;
+    buffer_size -= written;
   }
 }
 
-void PartialCircularBuffer::DoWrite(void* dest, const void* src, uint32 num) {
-  memcpy(dest, src, num);
-  position_ += num;
+uint32 PartialCircularBuffer::DoWrite(const uint8* input, uint32 input_size) {
+  DCHECK_LT(position_, data_size_);
+  uint32 space_left = data_size_ - position_;
+  uint32 write_size = std::min(input_size, space_left);
+  memcpy(buffer_data_->data + position_, input, write_size);
+  position_ += write_size;
   buffer_data_->total_written =
-      std::min(buffer_data_->total_written + num, data_size_);
+      std::min(buffer_data_->total_written + write_size, data_size_);
+  if (position_ >= data_size_)

[Nico, 2015/04/08 23:13:05]

If this happens, it will always be ==, not >=, right? Maybe keep the
DCHECK_EQ(position_, data_size_) in this if?

[gzobqq, 2015/04/09 08:36:40]

Done.

+    position_ = buffer_data_->wrap_position;
   buffer_data_->end_position = position_;
+  return write_size;
 }