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
+    const size_t    fCapacity;
+
+    SkBufferBlock(size_t capacity) : fNext(nullptr), fUsed(0), fCapacity(capacity) {}
 
     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;
+        void* buffer = sk_malloc_throw(sizeof(SkBufferBlock) + capacity);
+        return new (buffer) SkBufferBlock(capacity);
     }
 
-    // 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 {
 #ifdef SK_DEBUG
         SkASSERT(fCapacity > 0);
         SkASSERT(fUsed <= fCapacity);
 #endif
     }
 
 private:
     static size_t LengthToCapacity(size_t length) {
         const size_t minSize = kMinAllocSize - sizeof(SkBufferBlock);
         return SkTMax(length, minSize);
     }
 };
 
 struct SkBufferHead {
     mutable int32_t fRefCnt;
     SkBufferBlock   fBlock;
 
+    SkBufferHead(size_t capacity) : fRefCnt(1), fBlock(capacity) {}
+
     static size_t LengthToCapacity(size_t length) {
         const size_t minSize = kMinAllocSize - sizeof(SkBufferHead);
         return SkTMax(length, minSize);
     }
 
     static SkBufferHead* Alloc(size_t length) {
         size_t capacity = LengthToCapacity(length);
         size_t size = sizeof(SkBufferHead) + capacity;
-        SkBufferHead* head = (SkBufferHead*)sk_malloc_throw(size);
-        head->fRefCnt = 1;
-        head->fBlock.fNext = nullptr;
-        head->fBlock.fUsed = 0;
-        head->fBlock.fCapacity = capacity;
-        return head;
+        void* buffer = sk_malloc_throw(size);
+        return new (buffer) SkBufferHead(capacity);
     }
 
     void ref() const {
         SkASSERT(fRefCnt > 0);
         sk_atomic_inc(&fRefCnt);
     }
 
     void unref() const {
         SkASSERT(fRefCnt > 0);
         // A release here acts in place of all releases we "should" have been doing in ref().
@@ skipping 22 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);
 }