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/07 16:42:16]

Doesn't this have the same problem?

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

It is a complicated function. I spent some time thinking about it though and it
is probably correct.

   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_;
-
-  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;
   }
 }
 
 void PartialCircularBuffer::DoWrite(void* dest, const void* src, uint32 num) {
   memcpy(dest, src, num);
   position_ += num;
   buffer_data_->total_written =
       std::min(buffer_data_->total_written + num, data_size_);
   buffer_data_->end_position = position_;
 }