Diff
Modified: branches/safari-600.1-branch/Source/WebCore/ChangeLog (171545 => 171546)
--- branches/safari-600.1-branch/Source/WebCore/ChangeLog 2014-07-24 23:01:43 UTC (rev 171545)
+++ branches/safari-600.1-branch/Source/WebCore/ChangeLog 2014-07-24 23:11:47 UTC (rev 171546)
@@ -1,5 +1,69 @@
2014-07-24 Lucas Forschler <lforsch...@apple.com>
+ Merge r171526
+
+ 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 Lucas Forschler <lforsch...@apple.com>
+
Merge r171497
2014-07-23 Pratik Solanki <psola...@apple.com>
Modified: branches/safari-600.1-branch/Source/WebCore/loader/cache/CachedResource.cpp (171545 => 171546)
--- branches/safari-600.1-branch/Source/WebCore/loader/cache/CachedResource.cpp 2014-07-24 23:01:43 UTC (rev 171545)
+++ branches/safari-600.1-branch/Source/WebCore/loader/cache/CachedResource.cpp 2014-07-24 23:11:47 UTC (rev 171546)
@@ -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: branches/safari-600.1-branch/Source/WebCore/platform/SharedBuffer.cpp (171545 => 171546)
--- branches/safari-600.1-branch/Source/WebCore/platform/SharedBuffer.cpp 2014-07-24 23:01:43 UTC (rev 171545)
+++ branches/safari-600.1-branch/Source/WebCore/platform/SharedBuffer.cpp 2014-07-24 23:11:47 UTC (rev 171546)
@@ -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: branches/safari-600.1-branch/Source/WebCore/platform/SharedBuffer.h (171545 => 171546)
--- branches/safari-600.1-branch/Source/WebCore/platform/SharedBuffer.h 2014-07-24 23:01:43 UTC (rev 171545)
+++ branches/safari-600.1-branch/Source/WebCore/platform/SharedBuffer.h 2014-07-24 23:11:47 UTC (rev 171546)
@@ -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: branches/safari-600.1-branch/Source/WebCore/platform/cf/SharedBufferCF.cpp (171545 => 171546)
--- branches/safari-600.1-branch/Source/WebCore/platform/cf/SharedBufferCF.cpp 2014-07-24 23:01:43 UTC (rev 171545)
+++ branches/safari-600.1-branch/Source/WebCore/platform/cf/SharedBufferCF.cpp 2014-07-24 23:11:47 UTC (rev 171546)
@@ -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: branches/safari-600.1-branch/Source/WebCore/platform/mac/SharedBufferMac.mm (171545 => 171546)
--- branches/safari-600.1-branch/Source/WebCore/platform/mac/SharedBufferMac.mm 2014-07-24 23:01:43 UTC (rev 171545)
+++ branches/safari-600.1-branch/Source/WebCore/platform/mac/SharedBufferMac.mm 2014-07-24 23:11:47 UTC (rev 171546)
@@ -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)
