Fix PartialCircularBuffer OOB memcpy().

chrome/common/partial_circular_buffer.cc

Index: chrome/common/partial_circular_buffer.cc
diff --git a/chrome/common/partial_circular_buffer.cc b/chrome/common/partial_circular_buffer.cc
index 4161cc1864706b2ddbce9037d26c72ecf0203e48..2239f885e9a3263fc86e4da0a3c417aba27a8790 100644
--- a/chrome/common/partial_circular_buffer.cc
+++ b/chrome/common/partial_circular_buffer.cc
@@ -139,23 +139,17 @@ uint32 PartialCircularBuffer::Read(void* buffer, uint32 buffer_size) {
 
 void PartialCircularBuffer::Write(const void* buffer, uint32 buffer_size) {
   DCHECK(buffer_data_);
-  uint32 position_before_write = 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;
-  }
-
-  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);
+  const uint8* input = static_cast<const uint8*>(buffer);

[Nico, 2015/04/07 16:42:16]

I can't see the bug, but if writing a buffer several times the size of the
circular buffer makes sense in some situations, should the implementation be
less wasteful? This does a memcpy() per overlap at the moment. It's probably
possible to do something like

  if (buffer_size > size_of_cycle) {
    buffer = buffer + buffer_size - size_of_cycle;
    int slop = buffer_size % size_of_cycle;
    position_ = position_ + (position_ - wrap_position + slop + size_of_cycle) %
size_of_cycle;
    buffer_size = size_of_cycle;
  }

somewhere at the top of the function and keep the exisiting code, so that every
element is written only once.

(warning: code written without testing or even drawing boxes on a piece of
paper; it's likely wrong)

[gzobqq, 2015/04/08 12:59:03]

The second DoWrite() lacks a bounds check and can OOB write.
It's a bit more complicated, because the part to skip might be in the middle of
the buffer. That happens when the first part is non-wrapping, position_ <
wrap_position.

Done.

+  while (buffer_size > 0) {
+    uint32 space_left = data_size_ - position_;
+    uint32 write_size = std::min(buffer_size, space_left);
+    DoWrite(buffer_data_->data + position_, input, write_size);
+    if (position_ >= data_size_) {
+      DCHECK_EQ(position_, data_size_);
+      position_ = buffer_data_->wrap_position;
+    }
+    input += write_size;
+    buffer_size -= write_size;
   }
 }