Changeset 171526 in webkit


Ignore:
Timestamp:
Jul 24, 2014, 2:37:43 PM (11 years ago)
Author:
psolanki@apple.com
Message:

Sharing SharedBuffer between WebCore and ImageIO is racy and crash prone
​https://bugs.webkit.org/show_bug.cgi?id=135069
<rdar://problem/17470655>

Reviewed by Simon Fraser.

When passing image data to ImageIO for decoding, we pass an NSData subclass that is a wraper
around SharedBuffer. This can be a problem when ImageIO tries to access the data on the CA
thread. End result is data corruption on large image loads and potential crashes. The fix is
to have SharedBuffer create a copy of its data if the data has been passed to ImageIO and
might be accessed concurrently.

Since Vector is not refcounted, we do this by having a new refcounted object in SharedBuffer
that contains the buffer and we pass that in our NSData subclass WebCoreSharedBufferData.
Code that would result in the Vector memory moving e.g. append(), resize(), now checks to
see if the buffer was shared and if so, will create a new copy of the vector. This ensures
that the main thread does not end up invalidating the vector memory that we have passed it
to ImageIO.

No new tests because no functional changes.

  • loader/cache/CachedResource.cpp:

(WebCore::CachedResource::makePurgeable):

Remove early return - createPurgeableMemory() has the correct check now.

  • platform/SharedBuffer.cpp:

(WebCore::SharedBuffer::SharedBuffer):
(WebCore::SharedBuffer::adoptVector):
(WebCore::SharedBuffer::createPurgeableBuffer):

Don't create purgeable buffer if we are sharing the buffer.

(WebCore::SharedBuffer::append):
(WebCore::SharedBuffer::clear):
(WebCore::SharedBuffer::copy):
(WebCore::SharedBuffer::duplicateDataBufferIfNecessary): Added.

Create a new copy of the data if we have shared the buffer and if appending to it would
exceed the capacity of the vector resulting in memmove.

(WebCore::SharedBuffer::appendToInternalBuffer): Added.
(WebCore::SharedBuffer::clearInternalBuffer): Added.
(WebCore::SharedBuffer::buffer):

Create a new copy of the buffer if we have shared it.

(WebCore::SharedBuffer::getSomeData):

  • platform/SharedBuffer.h:
  • platform/cf/SharedBufferCF.cpp:

(WebCore::SharedBuffer::SharedBuffer):
(WebCore::SharedBuffer::singleDataArrayBuffer):
(WebCore::SharedBuffer::maybeAppendDataArray):

  • platform/mac/SharedBufferMac.mm:

Pass the InternalBuffer object to WebCoreSharedBufferData

(-[WebCoreSharedBufferData dealloc]):
(-[WebCoreSharedBufferData initWithSharedBufferInternalBuffer:]):
(-[WebCoreSharedBufferData length]):
(-[WebCoreSharedBufferData bytes]):
(WebCore::SharedBuffer::createNSData):

Call createCFData() instead of duplicating code.

(WebCore::SharedBuffer::createCFData):

If the data is in purgeable memory, make a copy of it since m_buffer was cleared when
creating the purgeable buffer.

(-[WebCoreSharedBufferData initWithSharedBuffer:]): Deleted.

Location:
trunk/Source/WebCore
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r171513 r171526  
     12014-07-24  Pratik Solanki  <psolanki@apple.com>
     2
     3        Sharing SharedBuffer between WebCore and ImageIO is racy and crash prone
     4        https://bugs.webkit.org/show_bug.cgi?id=135069
     5        <rdar://problem/17470655>
     6
     7        Reviewed by Simon Fraser.
     8
     9        When passing image data to ImageIO for decoding, we pass an NSData subclass that is a wraper
     10        around SharedBuffer. This can be a problem when ImageIO tries to access the data on the CA
     11        thread. End result is data corruption on large image loads and potential crashes. The fix is
     12        to have SharedBuffer create a copy of its data if the data has been passed to ImageIO and
     13        might be accessed concurrently.
     14
     15        Since Vector is not refcounted, we do this by having a new refcounted object in SharedBuffer
     16        that contains the buffer and we pass that in our NSData subclass WebCoreSharedBufferData.
     17        Code that would result in the Vector memory moving e.g. append(), resize(), now checks to
     18        see if the buffer was shared and if so, will create a new copy of the vector. This ensures
     19        that the main thread does not end up invalidating the vector memory that we have passed it
     20        to ImageIO.
     21
     22        No new tests because no functional changes.
     23
     24        * loader/cache/CachedResource.cpp:
     25        (WebCore::CachedResource::makePurgeable):
     26            Remove early return - createPurgeableMemory() has the correct check now.
     27        * platform/SharedBuffer.cpp:
     28        (WebCore::SharedBuffer::SharedBuffer):
     29        (WebCore::SharedBuffer::adoptVector):
     30        (WebCore::SharedBuffer::createPurgeableBuffer):
     31            Don't create purgeable buffer if we are sharing the buffer.
     32        (WebCore::SharedBuffer::append):
     33        (WebCore::SharedBuffer::clear):
     34        (WebCore::SharedBuffer::copy):
     35        (WebCore::SharedBuffer::duplicateDataBufferIfNecessary): Added.
     36            Create a new copy of the data if we have shared the buffer and if appending to it would
     37            exceed the capacity of the vector resulting in memmove.
     38        (WebCore::SharedBuffer::appendToInternalBuffer): Added.
     39        (WebCore::SharedBuffer::clearInternalBuffer): Added.
     40        (WebCore::SharedBuffer::buffer):
     41            Create a new copy of the buffer if we have shared it.
     42        (WebCore::SharedBuffer::getSomeData):
     43        * platform/SharedBuffer.h:
     44        * platform/cf/SharedBufferCF.cpp:
     45        (WebCore::SharedBuffer::SharedBuffer):
     46        (WebCore::SharedBuffer::singleDataArrayBuffer):
     47        (WebCore::SharedBuffer::maybeAppendDataArray):
     48        * platform/mac/SharedBufferMac.mm:
     49            Pass the InternalBuffer object to WebCoreSharedBufferData
     50        (-[WebCoreSharedBufferData dealloc]):
     51        (-[WebCoreSharedBufferData initWithSharedBufferInternalBuffer:]):
     52        (-[WebCoreSharedBufferData length]):
     53        (-[WebCoreSharedBufferData bytes]):
     54        (WebCore::SharedBuffer::createNSData):
     55            Call createCFData() instead of duplicating code.
     56        (WebCore::SharedBuffer::createCFData):
     57            If the data is in purgeable memory, make a copy of it since m_buffer was cleared when
     58            creating the purgeable buffer.
     59        (-[WebCoreSharedBufferData initWithSharedBuffer:]): Deleted.
     60
    1612014-07-24  peavo@outlook.com  <peavo@outlook.com>
    262

  • trunk/Source/WebCore/loader/cache/CachedResource.cpp

    r171036 r171526  
    812812            return false;
    813813       
    814         // Should not make buffer purgeable if it has refs other than this since we don't want two copies.
    815         if (!m_data->hasOneRef())
    816             return false;
    817 
    818814        m_data->createPurgeableBuffer();
    819815        if (!m_data->hasPurgeableBuffer())

  • trunk/Source/WebCore/platform/SharedBuffer.cpp

    r171289 r171526  
    6666SharedBuffer::SharedBuffer()
    6767    : m_size(0)
     68    , m_buffer(adoptRef(new DataBuffer))
    6869    , m_shouldUsePurgeableMemory(false)
    6970#if ENABLE(DISK_IMAGE_CACHE)
    … …  
    7879SharedBuffer::SharedBuffer(unsigned size)
    7980    : m_size(size)
    80     , m_buffer(size)
     81    , m_buffer(adoptRef(new DataBuffer))
    8182    , m_shouldUsePurgeableMemory(false)
    8283#if ENABLE(DISK_IMAGE_CACHE)
    … …  
    9192SharedBuffer::SharedBuffer(const char* data, unsigned size)
    9293    : m_size(0)
     94    , m_buffer(adoptRef(new DataBuffer))
    9395    , m_shouldUsePurgeableMemory(false)
    9496#if ENABLE(DISK_IMAGE_CACHE)
    … …  
    104106SharedBuffer::SharedBuffer(const unsigned char* data, unsigned size)
    105107    : m_size(0)
     108    , m_buffer(adoptRef(new DataBuffer))
    106109    , m_shouldUsePurgeableMemory(false)
    107110#if ENABLE(DISK_IMAGE_CACHE)
    … …  
    130133{
    131134    RefPtr<SharedBuffer> buffer = create();
    132     buffer->m_buffer.swap(vector);
    133     buffer->m_size = buffer->m_buffer.size();
     135    buffer->m_buffer->data.swap(vector);
     136    buffer->m_size = buffer->m_buffer->data.size();
    134137    return buffer.release();
    135138}
    … …  
    233236#endif
    234237
    235     if (!hasOneRef())
     238    if (!m_buffer->hasOneRef())
    236239        return;
    237240
    … …  
    243246    if (!m_purgeableBuffer)
    244247        return;
    245     unsigned bufferSize = m_buffer.size();
     248    unsigned bufferSize = m_buffer->data.size();
    246249    if (bufferSize) {
    247         memcpy(destination, m_buffer.data(), bufferSize);
     250        memcpy(destination, m_buffer->data.data(), bufferSize);
    248251        destination += bufferSize;
    249         m_buffer.clear();
     252        (const_cast<SharedBuffer*>(this))->clearDataBuffer();
    250253    }
    251254    copyBufferAndClear(destination, m_size - bufferSize);
    … …  
    324327
    325328#if !USE(NETWORK_CFDATA_ARRAY_CALLBACK)
    326     unsigned positionInSegment = offsetInSegment(m_size - m_buffer.size());
     329    unsigned positionInSegment = offsetInSegment(m_size - m_buffer->data.size());
    327330    m_size += length;
    328331
    329332    if (m_size <= segmentSize) {
    330333        // No need to use segments for small resource data
    331         if (m_buffer.isEmpty())
    332             m_buffer.reserveInitialCapacity(length);
    333         m_buffer.append(data, length);
     334        if (m_buffer->data.isEmpty())
     335            m_buffer->data.reserveInitialCapacity(length);
     336        appendToDataBuffer(data, length);
    334337        return;
    335338    }
    … …  
    357360    }
    358361#else
    359     if (m_buffer.isEmpty())
    360         m_buffer.reserveInitialCapacity(length);
    361     m_buffer.append(data, length);
    362362    m_size += length;
     363    if (m_buffer->data.isEmpty())
     364        m_buffer->data.reserveInitialCapacity(length);
     365    appendToDataBuffer(data, length);
    363366#endif
    364367}
    … …  
    383386
    384387    m_size = 0;
    385     m_buffer.clear();
     388    clearDataBuffer();
    386389    m_purgeableBuffer.clear();
    387390}
    … …  
    396399
    397400    clone->m_size = m_size;
    398     clone->m_buffer.reserveCapacity(m_size);
    399     clone->m_buffer.append(m_buffer.data(), m_buffer.size());
     401    clone->m_buffer->data.reserveCapacity(m_size);
     402    clone->m_buffer->data.append(m_buffer->data.data(), m_buffer->data.size());
    400403#if !USE(NETWORK_CFDATA_ARRAY_CALLBACK)
    401404    for (unsigned i = 0; i < m_segments.size(); ++i)
    402         clone->m_buffer.append(m_segments[i], segmentSize);
     405        clone->m_buffer->data.append(m_segments[i], segmentSize);
    403406#else
    404407    for (unsigned i = 0; i < m_dataArray.size(); ++i)
    … …  
    412415    ASSERT(hasOneRef());
    413416    return m_purgeableBuffer.release();
     417}
     418
     419void SharedBuffer::duplicateDataBufferIfNecessary() const
     420{
     421    if (m_buffer->hasOneRef() || m_size <= m_buffer->data.capacity())
     422        return;
     423
     424    RefPtr<DataBuffer> newBuffer = adoptRef(new DataBuffer);
     425    newBuffer->data.reserveInitialCapacity(m_size);
     426    newBuffer->data = m_buffer->data;
     427    m_buffer = newBuffer.release();
     428}
     429
     430void SharedBuffer::appendToDataBuffer(const char *data, unsigned length) const
     431{
     432    duplicateDataBufferIfNecessary();
     433    m_buffer->data.append(data, length);
     434}
     435
     436void SharedBuffer::clearDataBuffer()
     437{
     438    if (!m_buffer->hasOneRef())
     439        m_buffer = adoptRef(new DataBuffer);
     440    else
     441        m_buffer->data.clear();
    414442}
    415443
    … …  
    433461    ASSERT(!isMemoryMapped());
    434462#endif
    435     unsigned bufferSize = m_buffer.size();
     463    unsigned bufferSize = m_buffer->data.size();
    436464    if (m_size > bufferSize) {
    437         m_buffer.resize(m_size);
    438         copyBufferAndClear(m_buffer.data() + bufferSize, m_size - bufferSize);
    439     }
    440     return m_buffer;
     465        duplicateDataBufferIfNecessary();
     466        m_buffer->data.resize(m_size);
     467        copyBufferAndClear(m_buffer->data.data() + bufferSize, m_size - bufferSize);
     468    }
     469    return m_buffer->data;
    441470}
    442471
    … …  
    465494
    466495    ASSERT_WITH_SECURITY_IMPLICATION(position < m_size);
    467     unsigned consecutiveSize = m_buffer.size();
     496    unsigned consecutiveSize = m_buffer->data.size();
    468497    if (position < consecutiveSize) {
    469         someData = m_buffer.data() + position;
     498        someData = m_buffer->data.data() + position;
    470499        return consecutiveSize - position;
    471500    }

  • trunk/Source/WebCore/platform/SharedBuffer.h

    r171497 r171526  
    3232#include <wtf/OwnPtr.h>
    3333#include <wtf/RefCounted.h>
     34#include <wtf/ThreadSafeRefCounted.h>
    3435#include <wtf/Vector.h>
    3536#include <wtf/text/WTFString.h>
    … …  
    159160    bool hasPlatformData() const;
    160161
     162    struct DataBuffer : public ThreadSafeRefCounted<DataBuffer> {
     163        Vector<char> data;
     164    };
     165
    161166private:
    162167    SharedBuffer();
    … …  
    178183    void copyBufferAndClear(char* destination, unsigned bytesToCopy) const;
    179184
     185    void appendToDataBuffer(const char *, unsigned) const;
     186    void duplicateDataBufferIfNecessary() const;
     187    void clearDataBuffer();
     188
    180189    unsigned m_size;
    181     mutable Vector<char> m_buffer;
     190    mutable RefPtr<DataBuffer> m_buffer;
     191
    182192    bool m_shouldUsePurgeableMemory;
    183193    mutable OwnPtr<PurgeableBuffer> m_purgeableBuffer;

  • trunk/Source/WebCore/platform/cf/SharedBufferCF.cpp

    r170975 r171526  
    4040SharedBuffer::SharedBuffer(CFDataRef cfData)
    4141    : m_size(0)
     42    , m_buffer(adoptRef(new DataBuffer))
    4243    , m_shouldUsePurgeableMemory(false)
    4344#if ENABLE(DISK_IMAGE_CACHE)
    … …  
    129130SharedBuffer::SharedBuffer(CFArrayRef cfDataArray)
    130131    : m_size(0)
     132    , m_buffer(adoptRef(new DataBuffer))
    131133    , m_shouldUsePurgeableMemory(false)
    132134#if ENABLE(DISK_IMAGE_CACHE)
    … …  
    188190    // If we had previously copied data into m_buffer in copyDataArrayAndClear() or some other
    189191    // function, then we can't return a pointer to the CFDataRef buffer.
    190     if (m_buffer.size())
     192    if (m_buffer->data.size())
    191193        return 0;
    192194
    … …  
    199201bool SharedBuffer::maybeAppendDataArray(SharedBuffer* data)
    200202{
    201     if (m_buffer.size() || m_cfData || !data->m_dataArray.size())
     203    if (m_buffer->data.size() || m_cfData || !data->m_dataArray.size())
    202204        return false;
    203205#if !ASSERT_DISABLED

  • trunk/Source/WebCore/platform/mac/SharedBufferMac.mm

    r171497 r171526  
    3333#include <wtf/PassRefPtr.h>
    3434
    35 
    3635using namespace WebCore;
    3736
    3837@interface WebCoreSharedBufferData : NSData
    3938{
    40     RefPtr<SharedBuffer> sharedBuffer;
     39    RefPtr<SharedBuffer::DataBuffer> buffer;
    4140}
    4241
    43 - (id)initWithSharedBuffer:(SharedBuffer*)buffer;
     42- (id)initWithSharedBufferDataBuffer:(SharedBuffer::DataBuffer*)dataBuffer;
    4443@end
    4544
    … …  
    5958    if (WebCoreObjCScheduleDeallocateOnMainThread([WebCoreSharedBufferData class], self))
    6059        return;
    61    
     60
    6261    [super dealloc];
    6362}
    … …  
    6867}
    6968
    70 - (id)initWithSharedBuffer:(SharedBuffer*)buffer
     69- (id)initWithSharedBufferDataBuffer:(SharedBuffer::DataBuffer*)dataBuffer
    7170{
    7271    self = [super init];
    7372   
    7473    if (self)
    75         sharedBuffer = buffer;
    76    
     74        buffer = dataBuffer;
     75
    7776    return self;
    7877}
    … …  
    8079- (NSUInteger)length
    8180{
    82     return sharedBuffer->size();
     81    return buffer->data.size();
    8382}
    8483
    8584- (const void *)bytes
    8685{
    87     return reinterpret_cast<const void*>(sharedBuffer->data());
     86    return reinterpret_cast<const void*>(buffer->data.data());
    8887}
    8988
    … …  
    9998RetainPtr<NSData> SharedBuffer::createNSData()
    10099{
    101     return adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this]);
     100    return adoptNS((NSData *)createCFData().leakRef());
    102101}
    103102
    … …  
    107106        return m_cfData;
    108107
    109     return adoptCF((CFDataRef)adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this]).leakRef());
     108    data(); // Force data into m_buffer from segments or data array.
     109    if (hasPurgeableBuffer()) {
     110        RefPtr<SharedBuffer::DataBuffer> copiedBuffer = adoptRef(new DataBuffer);
     111        copiedBuffer->data.append(data(), size());
     112        return adoptCF(reinterpret_cast<CFDataRef>([[WebCoreSharedBufferData alloc] initWithSharedBufferDataBuffer:copiedBuffer.get()]));
     113    }
     114
     115    return adoptCF(reinterpret_cast<CFDataRef>([[WebCoreSharedBufferData alloc] initWithSharedBufferDataBuffer:m_buffer.get()]));
    110116}
    111117
Note: See TracChangeset for help on using the changeset viewer.