Fix race condition in SkROBuffer.

src/core/SkRWBuffer.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkRWBuffer.h"
 #include "SkStream.h"
 
 // Force small chunks to be a page's worth
 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; };
 
     size_t avail() const { return fCapacity - fUsed; }
     void* availData() { return (char*)this->startData() + fUsed; }
 
     static SkBufferBlock* Alloc(size_t length) {
         size_t capacity = LengthToCapacity(length);
         SkBufferBlock* block = (SkBufferBlock*)sk_malloc_throw(sizeof(SkBufferBlock) + capacity);
         block->fNext = nullptr;
         block->fUsed = 0;
         block->fCapacity = capacity;
         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);
         memcpy(this->availData(), src, amount);
         fUsed += amount;
         this->validate();
         return amount;
     }
 
     void validate() const {
@@ skipping 63 matching lines @@
             block = block->fNext;
         }
         SkASSERT(minUsed <= totalUsed);
         if (tail) {
             SkASSERT(tail == lastBlock);
         }
 #endif
     }
 };
 
-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();
     }
 }
 
 SkROBuffer::Iter::Iter(const SkROBuffer* buffer) {
     this->reset(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;
     }
 }
 
 const void* SkROBuffer::Iter::data() const {
     return fRemaining ? fBlock->startData() : nullptr;
 }
 
 size_t SkROBuffer::Iter::size() const {
     if (!fBlock) {
         return 0;
     }
-    return SkTMin(fBlock->fUsed, fRemaining);
+    return SkTMin(fBlock->fCapacity, fRemaining);
 }
 
 bool SkROBuffer::Iter::next() {
     if (fRemaining) {
         fRemaining -= this->size();
         fBlock = fBlock->fNext;
     }
     return fRemaining != 0;
 }
 
+///////////////////////////////////////////////////////////////////////////////////////////////////
+
 SkRWBuffer::SkRWBuffer(size_t initialCapacity) : fHead(nullptr), fTail(nullptr), fTotalUsed(0) {}
 
 SkRWBuffer::~SkRWBuffer() {
     this->validate();
     if (fHead) {
         fHead->unref();
     }
 }
 
+// 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) {
         return;
     }
 
     fTotalUsed += length;
 
     if (nullptr == fHead) {
         fHead = SkBufferHead::Alloc(length);
         fTail = &fHead->fBlock;
     }
 
     size_t written = fTail->append(src, length);
     SkASSERT(written <= length);
     src = (const char*)src + written;
     length -= written;
 
     if (length) {
         SkBufferBlock* block = SkBufferBlock::Alloc(length);
         fTail->fNext = block;
         fTail = block;
         written = fTail->append(src, length);
         SkASSERT(written == 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) {
         fHead->validate(fTotalUsed, fTail);
     } else {
         SkASSERT(nullptr == fTail);
         SkASSERT(0 == fTotalUsed);
     }
 }
 #endif
@@ skipping 109 matching lines @@
     const SkROBuffer*   fBuffer;
     SkROBuffer::Iter    fIter;
     size_t              fLocalOffset;
     size_t              fGlobalOffset;
 };
 
 SkStreamAsset* SkRWBuffer::newStreamSnapshot() const {
     SkAutoTUnref<SkROBuffer> buffer(this->newRBufferSnapshot());
     return new SkROBufferStreamAsset(buffer);
 }