- Revision
- 171526
- Author
- psola...@apple.com
- Date
- 2014-07-24 14:37:43 -0700 (Thu, 24 Jul 2014)
Log 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.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (171525 => 171526)
--- trunk/Source/WebCore/ChangeLog 2014-07-24 21:12:53 UTC (rev 171525)
+++ trunk/Source/WebCore/ChangeLog 2014-07-24 21:37:43 UTC (rev 171526)
@@ -1,3 +1,63 @@
+2014-07-24 Pratik Solanki <psola...@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 pe...@outlook.com <pe...@outlook.com>
[Curl] Enable file logging.
Modified: trunk/Source/WebCore/loader/cache/CachedResource.cpp (171525 => 171526)
--- trunk/Source/WebCore/loader/cache/CachedResource.cpp 2014-07-24 21:12:53 UTC (rev 171525)
+++ trunk/Source/WebCore/loader/cache/CachedResource.cpp 2014-07-24 21:37:43 UTC (rev 171526)
@@ -811,10 +811,6 @@
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;
Modified: trunk/Source/WebCore/platform/SharedBuffer.cpp (171525 => 171526)
--- trunk/Source/WebCore/platform/SharedBuffer.cpp 2014-07-24 21:12:53 UTC (rev 171525)
+++ trunk/Source/WebCore/platform/SharedBuffer.cpp 2014-07-24 21:37:43 UTC (rev 171526)
@@ -65,6 +65,7 @@
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(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(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 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 @@
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 @@
return;
#endif
- if (!hasOneRef())
+ if (!m_buffer->hasOneRef())
return;
if (!m_shouldUsePurgeableMemory)
@@ -242,11 +245,11 @@
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 @@
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 @@
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 @@
#endif
m_size = 0;
- m_buffer.clear();
+ clearDataBuffer();
m_purgeableBuffer.clear();
}
@@ -395,11 +398,11 @@
}
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 @@
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 = 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 @@
#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 @@
}
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;
}
Modified: trunk/Source/WebCore/platform/SharedBuffer.h (171525 => 171526)
--- trunk/Source/WebCore/platform/SharedBuffer.h 2014-07-24 21:12:53 UTC (rev 171525)
+++ trunk/Source/WebCore/platform/SharedBuffer.h 2014-07-24 21:37:43 UTC (rev 171526)
@@ -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 @@
void tryReplaceContentsWithPlatformBuffer(SharedBuffer*);
bool hasPlatformData() const;
+ struct DataBuffer : public ThreadSafeRefCounted<DataBuffer> {
+ Vector<char> data;
+ };
+
private:
SharedBuffer();
explicit SharedBuffer(unsigned);
@@ -177,8 +182,13 @@
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)
Modified: trunk/Source/WebCore/platform/cf/SharedBufferCF.cpp (171525 => 171526)
--- trunk/Source/WebCore/platform/cf/SharedBufferCF.cpp 2014-07-24 21:12:53 UTC (rev 171525)
+++ trunk/Source/WebCore/platform/cf/SharedBufferCF.cpp 2014-07-24 21:37:43 UTC (rev 171526)
@@ -39,6 +39,7 @@
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 @@
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 @@
{
// 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 @@
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();
Modified: trunk/Source/WebCore/platform/mac/SharedBufferMac.mm (171525 => 171526)
--- trunk/Source/WebCore/platform/mac/SharedBufferMac.mm 2014-07-24 21:12:53 UTC (rev 171525)
+++ trunk/Source/WebCore/platform/mac/SharedBufferMac.mm 2014-07-24 21:37:43 UTC (rev 171526)
@@ -32,15 +32,14 @@
#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 @@
{
if (WebCoreObjCScheduleDeallocateOnMainThread([WebCoreSharedBufferData class], self))
return;
-
+
[super dealloc];
}
@@ -67,24 +66,24 @@
[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 @@
RetainPtr<NSData> SharedBuffer::createNSData()
{
- return adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBuffer:this]);
+ return adoptNS((NSData *)createCFData().leakRef());
}
RetainPtr<CFDataRef> SharedBuffer::createCFData()
@@ -106,7 +105,14 @@
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)
