diff options
author | 2016-04-08 12:47:14 -0700 | |
---|---|---|
committer | 2016-04-08 12:47:14 -0700 | |
commit | 377add74267e5e0c94521858fb1f9ac5cf299667 (patch) | |
tree | d30129fe90043125775bdc4aabce258dcbdaea1f /src/core | |
parent | 0576aa9c0722bba358a27f80cc134ea2cd4ca2c9 (diff) |
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
Diffstat (limited to 'src/core')
-rw-r--r-- | src/core/SkRWBuffer.cpp | 77 |
1 files changed, 33 insertions, 44 deletions
diff --git a/src/core/SkRWBuffer.cpp b/src/core/SkRWBuffer.cpp index c0c801adea..f0b565edf3 100644 --- a/src/core/SkRWBuffer.cpp +++ b/src/core/SkRWBuffer.cpp @@ -13,9 +13,11 @@ 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; }; @@ -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); - 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); @@ -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); - 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 { @@ -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); - 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(); } } @@ -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) { |