Fix race condition in SkROBuffer.

src/core/SkRWBuffer.cpp

Index: src/core/SkRWBuffer.cpp
diff --git a/src/core/SkRWBuffer.cpp b/src/core/SkRWBuffer.cpp
index c0a93bdf53c037ce768a0699983f1d7cf1ce30d5..75a51b81b62231698a897825a1d804023de5dbc4 100644
--- a/src/core/SkRWBuffer.cpp
+++ b/src/core/SkRWBuffer.cpp
@@ -12,9 +12,9 @@
 static const size_t kMinAllocSize = 4096;
 
 struct SkBufferBlock {
-    SkBufferBlock*  fNext;
-    size_t          fUsed;
-    size_t          fCapacity;
+    SkBufferBlock*  fNext;      // updated by the writer
+    size_t          fUsed;      // updated by the writer
+    size_t          fCapacity;  // never changes after the block is created

[scroggo, 2016/04/08 18:18:52]

const? I guess we need a constructor to do that...

[bungeman-skia, 2016/04/08 18:39:29]

It would actually be nice to write a private constructor and then call placement
new to call it in Alloc below. It would make it more obvious that we're
constructing an SkBufferBlock right there (and then we can mark this const).

[reed1, 2016/04/08 19:18:11]

Done.

 
     const void* startData() const { return this + 1; };
 
@@ -30,7 +30,9 @@ struct SkBufferBlock {
         return block;
     }
 
-    // Return number of bytes actually appended
+    // Return number of bytes actually appended. Important that we always completely this block
+    // before spilling into the next, since the reader uses fCapacity to know how many it can read.
+    //
     size_t append(const void* src, size_t length) {
         this->validate();
         size_t amount = SkTMin(this->avail(), length);
@@ -114,19 +116,25 @@ struct SkBufferHead {
     }
 };
 
-SkROBuffer::SkROBuffer(const SkBufferHead* head, size_t used) : fHead(head), fUsed(used) {
+///////////////////////////////////////////////////////////////////////////////////////////////////
+// The reader can only access block.fCapacity (which never changes), and cannot access
+// block.fUsed, which may be updated by the writer.
+//
+SkROBuffer::SkROBuffer(const SkBufferHead* head, size_t available)
+    : fHead(head), fAvailable(available)
+{
     if (head) {
         fHead->ref();
-        SkASSERT(used > 0);
-        head->validate(used);
+        SkASSERT(available > 0);
+        head->validate(available);
     } else {
-        SkASSERT(0 == used);
+        SkASSERT(0 == available);
     }
 }
 
 SkROBuffer::~SkROBuffer() {
     if (fHead) {
-        fHead->validate(fUsed);
+        fHead->validate(fAvailable);
         fHead->unref();
     }
 }
@@ -138,7 +146,7 @@ SkROBuffer::Iter::Iter(const SkROBuffer* buffer) {
 void SkROBuffer::Iter::reset(const SkROBuffer* buffer) {
     if (buffer) {
         fBlock = &buffer->fHead->fBlock;
-        fRemaining = buffer->fUsed;
+        fRemaining = buffer->fAvailable;
     } else {
         fBlock = nullptr;
         fRemaining = 0;
@@ -153,7 +161,7 @@ size_t SkROBuffer::Iter::size() const {
     if (!fBlock) {
         return 0;
     }
-    return SkTMin(fBlock->fUsed, fRemaining);
+    return SkTMin(fBlock->fCapacity, fRemaining);
 }
 
 bool SkROBuffer::Iter::next() {
@@ -164,6 +172,8 @@ bool SkROBuffer::Iter::next() {
     return fRemaining != 0;
 }
 
+///////////////////////////////////////////////////////////////////////////////////////////////////
+
 SkRWBuffer::SkRWBuffer(size_t initialCapacity) : fHead(nullptr), fTail(nullptr), fTotalUsed(0) {}
 
 SkRWBuffer::~SkRWBuffer() {
@@ -173,6 +183,10 @@ SkRWBuffer::~SkRWBuffer() {
     }
 }
 
+// It is important that we always completely fill the current block before spilling over to the
+// next, since our reader will be using fCapacity (min'd against its total available) to know how
+// many bytes to read from a given block.
+//
 void SkRWBuffer::append(const void* src, size_t length) {
     this->validate();
     if (0 == length) {
@@ -201,28 +215,6 @@ void SkRWBuffer::append(const void* src, size_t length) {
     this->validate();
 }
 
-void* SkRWBuffer::append(size_t length) {
-    this->validate();
-    if (0 == length) {
-        return nullptr;
-    }
-
-    fTotalUsed += length;
-
-    if (nullptr == fHead) {
-        fHead = SkBufferHead::Alloc(length);
-        fTail = &fHead->fBlock;
-    } else if (fTail->avail() < length) {
-        SkBufferBlock* block = SkBufferBlock::Alloc(length);
-        fTail->fNext = block;
-        fTail = block;
-    }
-
-    fTail->fUsed += length;
-    this->validate();
-    return (char*)fTail->availData() - length;
-}
-
 #ifdef SK_DEBUG
 void SkRWBuffer::validate() const {
     if (fHead) {