Fix race condition in SkROBuffer.

SkBufferBlock::fUsed may be updated by the writer while a reader is
attempting to read it.

BUG=chromium:601578
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1872853002

Review URL: https://codereview.chromium.org/1872853002

include/core/SkRWBuffer.h

@@ -25,7 +25,7 @@ public:
      *  Return the logical length of the data owned/shared by this buffer. It may be stored in
      *  multiple contiguous blocks, accessible via the iterator.
      */
-    size_t size() const { return fUsed; }
+    size_t size() const { return fAvailable; }
 
     class SK_API Iter {
     public:
@@ -56,11 +56,11 @@ public:
     };
 
 private:
-    SkROBuffer(const SkBufferHead* head, size_t used);
+    SkROBuffer(const SkBufferHead* head, size_t available);
     virtual ~SkROBuffer();
 
     const SkBufferHead* fHead;
-    const size_t        fUsed;
+    const size_t        fAvailable;
 
     friend class SkRWBuffer;
 };
@@ -77,7 +77,6 @@ public:
 
     size_t size() const { return fTotalUsed; }
     void append(const void* buffer, size_t length);
-    void* append(size_t length);
 
     SkROBuffer* newRBufferSnapshot() const;
     SkStreamAsset* newStreamSnapshot() const;

src/core/SkRWBuffer.cpp

@@ -13,9 +13,11 @@
 static const size_t kMinAllocSize = 4096;
 
 struct SkBufferBlock {
-    SkBufferBlock*  fNext;
+    SkBufferBlock*  fNext;      // updated by the writer
-    size_t          fUsed;
+    size_t          fUsed;      // updated by the writer
-    size_t          fCapacity;
+    const size_t    fCapacity;
+
+    SkBufferBlock(size_t capacity) : fNext(nullptr), fUsed(0), fCapacity(capacity) {}
 
     const void* startData() const { return this + 1; };
 
@@ -24,14 +26,13 @@ struct SkBufferBlock {
 
     static SkBufferBlock* Alloc(size_t length) {
         size_t capacity = LengthToCapacity(length);
-        SkBufferBlock* block = (SkBufferBlock*)sk_malloc_throw(sizeof(SkBufferBlock) + capacity);
+        void* buffer = sk_malloc_throw(sizeof(SkBufferBlock) + capacity);
-        block->fNext = nullptr;
+        return new (buffer) SkBufferBlock(capacity);
-        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);
@@ -59,6 +60,8 @@ 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);
@@ -67,12 +70,8 @@ struct SkBufferHead {
     static SkBufferHead* Alloc(size_t length) {
         size_t capacity = LengthToCapacity(length);
         size_t size = sizeof(SkBufferHead) + capacity;
-        SkBufferHead* head = (SkBufferHead*)sk_malloc_throw(size);
+        void* buffer = sk_malloc_throw(size);
-        head->fRefCnt = 1;
+        return new (buffer) SkBufferHead(capacity);
-        head->fBlock.fNext = nullptr;
-        head->fBlock.fUsed = 0;
-        head->fBlock.fCapacity = capacity;
-        return head;
     }
 
     void ref() const {
@@ -115,19 +114,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);
+        SkASSERT(available > 0);
-        head->validate(used);
+        head->validate(available);
     } else {
-        SkASSERT(0 == used);
+        SkASSERT(0 == available);
     }
 }
 
 SkROBuffer::~SkROBuffer() {
     if (fHead) {
-        fHead->validate(fUsed);
+        fHead->validate(fAvailable);
         fHead->unref();
     }
 }
@@ -139,7 +144,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;
@@ -154,7 +159,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() {
@@ -165,6 +170,8 @@ bool SkROBuffer::Iter::next() {
     return fRemaining != 0;
 }
 
+///////////////////////////////////////////////////////////////////////////////////////////////////
+
 SkRWBuffer::SkRWBuffer(size_t initialCapacity) : fHead(nullptr), fTail(nullptr), fTotalUsed(0) {}
 
 SkRWBuffer::~SkRWBuffer() {
@@ -174,6 +181,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) {
@@ -202,28 +213,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) {

tests/DataRefTest.cpp

@@ -294,11 +294,7 @@ DEF_TEST(RWBuffer, reporter) {
     {
         SkRWBuffer buffer;
         for (int i = 0; i < N; ++i) {
-            if (0 == (i & 1)) {
+            buffer.append(gABC, 26);
-                buffer.append(gABC, 26);
-            } else {
-                memcpy(buffer.append(26), gABC, 26);
-            }
             readers[i] = buffer.newRBufferSnapshot();
             streams[i] = buffer.newStreamSnapshot();
         }