Sharing SharedBuffer between WebCore and ImageIO is racy and crash prone
authorpsolanki@apple.com <psolanki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 Jul 2014 21:37:43 +0000 (21:37 +0000)
committerpsolanki@apple.com <psolanki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 Jul 2014 21:37:43 +0000 (21:37 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@171526 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/loader/cache/CachedResource.cpp
Source/WebCore/platform/SharedBuffer.cpp
Source/WebCore/platform/SharedBuffer.h
Source/WebCore/platform/cf/SharedBufferCF.cpp
Source/WebCore/platform/mac/SharedBufferMac.mm

index e67e638..41c7e65 100644 (file)
@@ -1,3 +1,63 @@
+2014-07-24  Pratik Solanki  <psolanki@apple.com>
+
+        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.
+
 2014-07-24  peavo@outlook.com  <peavo@outlook.com>
 
         [Curl] Enable file logging.
index b0c277d..3be5fc5 100644 (file)
@@ -811,10 +811,6 @@ bool CachedResource::makePurgeable(bool purgeable)
         if (!m_data)
             return false;
         
-        // Should not make buffer purgeable if it has refs other than this since we don't want two copies.
-        if (!m_data->hasOneRef())
-            return false;
-
         m_data->createPurgeableBuffer();
         if (!m_data->hasPurgeableBuffer())
             return false;
index 692bc59..bf6c9cf 100644 (file)
@@ -65,6 +65,7 @@ static inline void freeSegment(char* p)
 
 SharedBuffer::SharedBuffer()
     : m_size(0)
+    , m_buffer(adoptRef(new DataBuffer))
     , m_shouldUsePurgeableMemory(false)
 #if ENABLE(DISK_IMAGE_CACHE)
     , m_isMemoryMapped(false)
@@ -77,7 +78,7 @@ SharedBuffer::SharedBuffer()
 
 SharedBuffer::SharedBuffer(unsigned size)
     : m_size(size)
-    , m_buffer(size)
+    , m_buffer(adoptRef(new DataBuffer))
     , m_shouldUsePurgeableMemory(false)
 #if ENABLE(DISK_IMAGE_CACHE)
     , m_isMemoryMapped(false)
@@ -90,6 +91,7 @@ SharedBuffer::SharedBuffer(unsigned size)
 
 SharedBuffer::SharedBuffer(const char* data, unsigned size)
     : m_size(0)
+    , m_buffer(adoptRef(new DataBuffer))
     , m_shouldUsePurgeableMemory(false)
 #if ENABLE(DISK_IMAGE_CACHE)
     , m_isMemoryMapped(false)
@@ -103,6 +105,7 @@ SharedBuffer::SharedBuffer(const char* data, unsigned size)
 
 SharedBuffer::SharedBuffer(const unsigned char* data, unsigned size)
     : m_size(0)
+    , m_buffer(adoptRef(new DataBuffer))
     , m_shouldUsePurgeableMemory(false)
 #if ENABLE(DISK_IMAGE_CACHE)
     , m_isMemoryMapped(false)
@@ -129,8 +132,8 @@ SharedBuffer::~SharedBuffer()
 PassRefPtr<SharedBuffer> SharedBuffer::adoptVector(Vector<char>& vector)
 {
     RefPtr<SharedBuffer> buffer = create();
-    buffer->m_buffer.swap(vector);
-    buffer->m_size = buffer->m_buffer.size();
+    buffer->m_buffer->data.swap(vector);
+    buffer->m_size = buffer->m_buffer->data.size();
     return buffer.release();
 }
 
@@ -232,7 +235,7 @@ void SharedBuffer::createPurgeableBuffer() const
         return;
 #endif
 
-    if (!hasOneRef())
+    if (!m_buffer->hasOneRef())
         return;
 
     if (!m_shouldUsePurgeableMemory)
@@ -242,11 +245,11 @@ void SharedBuffer::createPurgeableBuffer() const
     m_purgeableBuffer = PurgeableBuffer::createUninitialized(m_size, destination);
     if (!m_purgeableBuffer)
         return;
-    unsigned bufferSize = m_buffer.size();
+    unsigned bufferSize = m_buffer->data.size();
     if (bufferSize) {
-        memcpy(destination, m_buffer.data(), bufferSize);
+        memcpy(destination, m_buffer->data.data(), bufferSize);
         destination += bufferSize;
-        m_buffer.clear();
+        (const_cast<SharedBuffer*>(this))->clearDataBuffer();
     }
     copyBufferAndClear(destination, m_size - bufferSize);
 }
@@ -323,14 +326,14 @@ void SharedBuffer::append(const char* data, unsigned length)
     maybeTransferPlatformData();
 
 #if !USE(NETWORK_CFDATA_ARRAY_CALLBACK)
-    unsigned positionInSegment = offsetInSegment(m_size - m_buffer.size());
+    unsigned positionInSegment = offsetInSegment(m_size - m_buffer->data.size());
     m_size += length;
 
     if (m_size <= segmentSize) {
         // No need to use segments for small resource data
-        if (m_buffer.isEmpty())
-            m_buffer.reserveInitialCapacity(length);
-        m_buffer.append(data, length);
+        if (m_buffer->data.isEmpty())
+            m_buffer->data.reserveInitialCapacity(length);
+        appendToDataBuffer(data, length);
         return;
     }
 
@@ -356,10 +359,10 @@ void SharedBuffer::append(const char* data, unsigned length)
         bytesToCopy = std::min(length, segmentSize);
     }
 #else
-    if (m_buffer.isEmpty())
-        m_buffer.reserveInitialCapacity(length);
-    m_buffer.append(data, length);
     m_size += length;
+    if (m_buffer->data.isEmpty())
+        m_buffer->data.reserveInitialCapacity(length);
+    appendToDataBuffer(data, length);
 #endif
 }
 
@@ -382,7 +385,7 @@ void SharedBuffer::clear()
 #endif
 
     m_size = 0;
-    m_buffer.clear();
+    clearDataBuffer();
     m_purgeableBuffer.clear();
 }
 
@@ -395,11 +398,11 @@ PassRefPtr<SharedBuffer> SharedBuffer::copy() const
     }
 
     clone->m_size = m_size;
-    clone->m_buffer.reserveCapacity(m_size);
-    clone->m_buffer.append(m_buffer.data(), m_buffer.size());
+    clone->m_buffer->data.reserveCapacity(m_size);
+    clone->m_buffer->data.append(m_buffer->data.data(), m_buffer->data.size());
 #if !USE(NETWORK_CFDATA_ARRAY_CALLBACK)
     for (unsigned i = 0; i < m_segments.size(); ++i)
-        clone->m_buffer.append(m_segments[i], segmentSize);
+        clone->m_buffer->data.append(m_segments[i], segmentSize);
 #else
     for (unsigned i = 0; i < m_dataArray.size(); ++i)
         clone->append(m_dataArray[i].get());
@@ -413,6 +416,31 @@ PassOwnPtr<PurgeableBuffer> SharedBuffer::releasePurgeableBuffer()
     return m_purgeableBuffer.release(); 
 }
 
+void SharedBuffer::duplicateDataBufferIfNecessary() const
+{
+    if (m_buffer->hasOneRef() || m_size <= m_buffer->data.capacity())
+        return;
+
+    RefPtr<DataBuffer> newBuffer = adoptRef(new DataBuffer);
+    newBuffer->data.reserveInitialCapacity(m_size);
+    newBuffer->data = m_buffer->data;
+    m_buffer = newBuffer.release();
+}
+
+void SharedBuffer::appendToDataBuffer(const char *data, unsigned length) const
+{
+    duplicateDataBufferIfNecessary();
+    m_buffer->data.append(data, length);
+}
+
+void SharedBuffer::clearDataBuffer()
+{
+    if (!m_buffer->hasOneRef())
+        m_buffer = adoptRef(new DataBuffer);
+    else
+        m_buffer->data.clear();
+}
+
 #if !USE(NETWORK_CFDATA_ARRAY_CALLBACK)
 void SharedBuffer::copyBufferAndClear(char* destination, unsigned bytesToCopy) const
 {
@@ -432,12 +460,13 @@ const Vector<char>& SharedBuffer::buffer() const
 #if ENABLE(DISK_IMAGE_CACHE)
     ASSERT(!isMemoryMapped());
 #endif
-    unsigned bufferSize = m_buffer.size();
+    unsigned bufferSize = m_buffer->data.size();
     if (m_size > bufferSize) {
-        m_buffer.resize(m_size);
-        copyBufferAndClear(m_buffer.data() + bufferSize, m_size - bufferSize);
+        duplicateDataBufferIfNecessary();
+        m_buffer->data.resize(m_size);
+        copyBufferAndClear(m_buffer->data.data() + bufferSize, m_size - bufferSize);
     }
-    return m_buffer;
+    return m_buffer->data;
 }
 
 unsigned SharedBuffer::getSomeData(const char*& someData, unsigned position) const
@@ -464,9 +493,9 @@ unsigned SharedBuffer::getSomeData(const char*& someData, unsigned position) con
     }
 
     ASSERT_WITH_SECURITY_IMPLICATION(position < m_size);
-    unsigned consecutiveSize = m_buffer.size();
+    unsigned consecutiveSize = m_buffer->data.size();
     if (position < consecutiveSize) {
-        someData = m_buffer.data() + position;
+        someData = m_buffer->data.data() + position;
         return consecutiveSize - position;
     }
  
index 6a48c56..f4db960 100644 (file)
@@ -31,6 +31,7 @@
 #include <wtf/Forward.h>
 #include <wtf/OwnPtr.h>
 #include <wtf/RefCounted.h>
+#include <wtf/ThreadSafeRefCounted.h>
 #include <wtf/Vector.h>
 #include <wtf/text/WTFString.h>
 
@@ -158,6 +159,10 @@ public:
     void tryReplaceContentsWithPlatformBuffer(SharedBuffer*);
     bool hasPlatformData() const;
 
+    struct DataBuffer : public ThreadSafeRefCounted<DataBuffer> {
+        Vector<char> data;
+    };
+
 private:
     SharedBuffer();
     explicit SharedBuffer(unsigned);
@@ -177,8 +182,13 @@ private:
 
     void copyBufferAndClear(char* destination, unsigned bytesToCopy) const;
 
+    void appendToDataBuffer(const char *, unsigned) const;
+    void duplicateDataBufferIfNecessary() const;
+    void clearDataBuffer();
+
     unsigned m_size;
-    mutable Vector<char> m_buffer;
+    mutable RefPtr<DataBuffer> m_buffer;
+
     bool m_shouldUsePurgeableMemory;
     mutable OwnPtr<PurgeableBuffer> m_purgeableBuffer;
 #if USE(NETWORK_CFDATA_ARRAY_CALLBACK)
index f75613b..de9a762 100644 (file)
@@ -39,6 +39,7 @@ namespace WebCore {
 
 SharedBuffer::SharedBuffer(CFDataRef cfData)
     : m_size(0)
+    , m_buffer(adoptRef(new DataBuffer))
     , m_shouldUsePurgeableMemory(false)
 #if ENABLE(DISK_IMAGE_CACHE)
     , m_isMemoryMapped(false)
@@ -128,6 +129,7 @@ PassRefPtr<SharedBuffer> SharedBuffer::wrapCFDataArray(CFArrayRef cfDataArray)
 
 SharedBuffer::SharedBuffer(CFArrayRef cfDataArray)
     : m_size(0)
+    , m_buffer(adoptRef(new DataBuffer))
     , m_shouldUsePurgeableMemory(false)
 #if ENABLE(DISK_IMAGE_CACHE)
     , m_isMemoryMapped(false)
@@ -187,7 +189,7 @@ const char *SharedBuffer::singleDataArrayBuffer() const
 {
     // If we had previously copied data into m_buffer in copyDataArrayAndClear() or some other
     // function, then we can't return a pointer to the CFDataRef buffer.
-    if (m_buffer.size())
+    if (m_buffer->data.size())
         return 0;
 
     if (m_dataArray.size() != 1)
@@ -198,7 +200,7 @@ const char *SharedBuffer::singleDataArrayBuffer() const
 
 bool SharedBuffer::maybeAppendDataArray(SharedBuffer* data)
 {
-    if (m_buffer.size() || m_cfData || !data->m_dataArray.size())
+    if (m_buffer->data.size() || m_cfData || !data->m_dataArray.size())
         return false;
 #if !ASSERT_DISABLED
     unsigned originalSize = size();
index 2215881..f6e7aea 100644 (file)
 #include <wtf/MainThread.h>
 #include <wtf/PassRefPtr.h>
 
-
 using namespace WebCore;
 
 @interface WebCoreSharedBufferData : NSData
 {
-    RefPtr<SharedBuffer> sharedBuffer;
+    RefPtr<SharedBuffer::DataBuffer> buffer;
 }
 
-- (id)initWithSharedBuffer:(SharedBuffer*)buffer;
+- (id)initWithSharedBufferDataBuffer:(SharedBuffer::DataBuffer*)dataBuffer;
 @end
 
 @implementation WebCoreSharedBufferData
@@ -58,7 +57,7 @@ using namespace WebCore;
 {
     if (WebCoreObjCScheduleDeallocateOnMainThread([WebCoreSharedBufferData class], self))
         return;
-    
+
     [super dealloc];
 }
 
@@ -67,24 +66,24 @@ using namespace WebCore;
     [super finalize];
 }
 
-- (id)initWithSharedBuffer:(SharedBuffer*)buffer
+- (id)initWithSharedBufferDataBuffer:(SharedBuffer::DataBuffer*)dataBuffer
 {
     self = [super init];
     
     if (self)
-        sharedBuffer = buffer;
-    
+        buffer = dataBuffer;
+
     return self;
 }
 
 - (NSUInteger)length
 {
-    return sharedBuffer->size();
+    return buffer->data.size();
 }
 
 - (const void *)bytes
 {
-    return reinterpret_cast<const void*>(sharedBuffer->data());
+    return reinterpret_cast<const void*>(buffer->data.data());
 }
 
 @end
@@ -98,7 +97,7 @@ PassRefPtr<SharedBuffer> SharedBuffer::wrapNSData(NSData *nsData)
 
 RetainPtr<NSData> SharedBuffer::createNSData()
 {
-    return adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this]);
+    return adoptNS((NSData *)createCFData().leakRef());
 }
 
 RetainPtr<CFDataRef> SharedBuffer::createCFData()
@@ -106,7 +105,14 @@ RetainPtr<CFDataRef> SharedBuffer::createCFData()
     if (m_cfData)
         return m_cfData;
 
-    return adoptCF((CFDataRef)adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this]).leakRef());
+    data(); // Force data into m_buffer from segments or data array.
+    if (hasPurgeableBuffer()) {
+        RefPtr<SharedBuffer::DataBuffer> copiedBuffer = adoptRef(new DataBuffer);
+        copiedBuffer->data.append(data(), size());
+        return adoptCF(reinterpret_cast<CFDataRef>([[WebCoreSharedBufferData alloc] initWithSharedBufferDataBuffer:copiedBuffer.get()]));
+    }
+
+    return adoptCF(reinterpret_cast<CFDataRef>([[WebCoreSharedBufferData alloc] initWithSharedBufferDataBuffer:m_buffer.get()]));
 }
 
 PassRefPtr<SharedBuffer> SharedBuffer::createWithContentsOfFile(const String& filePath)