Diff
Modified: trunk/Source/WTF/ChangeLog (280259 => 280260)
--- trunk/Source/WTF/ChangeLog 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WTF/ChangeLog 2021-07-23 22:05:03 UTC (rev 280260)
@@ -1,3 +1,16 @@
+2021-07-23 Chris Dumez <[email protected]>
+
+ SharedBuffer::takeData() is a bit dangerous
+ https://bugs.webkit.org/show_bug.cgi?id=228161
+
+ Reviewed by Darin Adler.
+
+ Add Vector::uncheckedAppend() overload that takes in a Span.
+
+ * wtf/Vector.h:
+ (WTF::Vector::uncheckedAppend):
+ (WTF::Malloc>::uncheckedAppend):
+
2021-07-23 Robert Jenner <[email protected]>
Unreviewed, reverting r280205.
Modified: trunk/Source/WTF/wtf/Vector.h (280259 => 280260)
--- trunk/Source/WTF/wtf/Vector.h 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WTF/wtf/Vector.h 2021-07-23 22:05:03 UTC (rev 280260)
@@ -790,6 +790,7 @@
template<typename U> ALWAYS_INLINE void append(const U* u, size_t size) { append<FailureAction::Crash>(u, size); }
template<typename U> ALWAYS_INLINE bool tryAppend(const U* u, size_t size) { return append<FailureAction::Report>(u, size); }
template<typename U> ALWAYS_INLINE void append(Span<const U> span) { append(span.data(), span.size()); }
+ template<typename U> ALWAYS_INLINE void uncheckedAppend(Span<const U> span) { uncheckedAppend<FailureAction::Crash>(span.data(), span.size()); }
template<typename U, size_t otherCapacity, typename OtherOverflowHandler, size_t otherMinCapacity, typename OtherMalloc> void appendVector(const Vector<U, otherCapacity, OtherOverflowHandler, otherMinCapacity, OtherMalloc>&);
template<typename U, size_t otherCapacity, typename OtherOverflowHandler, size_t otherMinCapacity, typename OtherMalloc> void appendVector(Vector<U, otherCapacity, OtherOverflowHandler, otherMinCapacity, OtherMalloc>&&);
@@ -858,6 +859,7 @@
template<FailureAction, typename U> bool append(U&&);
template<FailureAction, typename U> bool append(const U*, size_t);
+ template<FailureAction, typename U> bool uncheckedAppend(const U*, size_t);
template<size_t position, typename U, typename... Items>
void uncheckedInitialize(U&& item, Items&&... items)
@@ -1302,6 +1304,24 @@
template<typename T, size_t inlineCapacity, typename OverflowHandler, size_t minCapacity, typename Malloc>
template<FailureAction action, typename U>
+ALWAYS_INLINE bool Vector<T, inlineCapacity, OverflowHandler, minCapacity, Malloc>::uncheckedAppend(const U* data, size_t dataSize)
+{
+ static_assert(action == FailureAction::Crash || action == FailureAction::Report);
+ if (!dataSize)
+ return true;
+
+ ASSERT(size() < capacity());
+
+ size_t newSize = m_size + dataSize;
+ asanBufferSizeWillChangeTo(newSize);
+ T* dest = end();
+ VectorCopier<std::is_trivial<T>::value, U>::uninitializedCopy(data, std::addressof(data[dataSize]), dest);
+ m_size = newSize;
+ return true;
+}
+
+template<typename T, size_t inlineCapacity, typename OverflowHandler, size_t minCapacity, typename Malloc>
+template<FailureAction action, typename U>
ALWAYS_INLINE bool Vector<T, inlineCapacity, OverflowHandler, minCapacity, Malloc>::append(U&& value)
{
if (size() != capacity()) {
Modified: trunk/Source/WebCore/ChangeLog (280259 => 280260)
--- trunk/Source/WebCore/ChangeLog 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WebCore/ChangeLog 2021-07-23 22:05:03 UTC (rev 280260)
@@ -1,3 +1,38 @@
+2021-07-23 Chris Dumez <[email protected]>
+
+ SharedBuffer::takeData() is a bit dangerous
+ https://bugs.webkit.org/show_bug.cgi?id=228161
+
+ Reviewed by Darin Adler.
+
+ SharedBuffer::takeData() is a bit dangerous since SharedBuffer is RefCounted and several object may be sharing ownership
+ of the buffer. Having one owner call takeData() in case ownership is shared leads to bugs such as Bug 228096.
+
+ To address the issue, I made SharedBuffer::takeData() private and introduced a new SharedBuffer::extractData() member
+ function which calls takeData() only if the SharedBuffer is not shared (RefCount is 1) and falls back to calling copyData()
+ otherwise. I also optimized copyData() a bit by iterating over the segments to build the vector, instead of calling the
+ potentially very slow SharedBuffer::data().
+
+ * Modules/fetch/FetchBodyConsumer.cpp:
+ (WebCore::FetchBodyConsumer::takeAsBlob):
+ * Modules/mediarecorder/MediaRecorder.cpp:
+ (WebCore::createDataAvailableEvent):
+ * editing/WebCorePasteboardFileReader.cpp:
+ (WebCore::WebCorePasteboardFileReader::readBuffer):
+ * editing/cocoa/WebContentReaderCocoa.mm:
+ (WebCore::createFragmentForImageAttachment):
+ (WebCore::WebContentReader::readImage):
+ * editing/gtk/WebContentReaderGtk.cpp:
+ (WebCore::WebContentReader::readImage):
+ * html/HTMLAttachmentElement.cpp:
+ (WebCore::HTMLAttachmentElement::updateEnclosingImageWithData):
+ * platform/SharedBuffer.cpp:
+ (WebCore::SharedBuffer::copyData):
+ * platform/SharedBuffer.h:
+ (WebCore::SharedBuffer::extractData):
+ * xml/XMLHttpRequest.cpp:
+ (WebCore::XMLHttpRequest::createResponseBlob):
+
2021-07-23 Alexey Shvayka <[email protected]>
[JSC] Call custom accessors / values with their holder's global object
Modified: trunk/Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp (280259 => 280260)
--- trunk/Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp 2021-07-23 22:05:03 UTC (rev 280260)
@@ -351,15 +351,8 @@
if (!m_buffer)
return Blob::create(context, Vector<uint8_t>(), Blob::normalizedContentType(m_contentType));
- Vector<uint8_t> data;
auto buffer = std::exchange(m_buffer, nullptr);
- if (buffer->hasOneRef())
- data = "" // Avoid copying the data in the case where the buffer isn't shared.
- else {
- buffer->forEachSegment([&data](auto& span) {
- data.append(span);
- });
- }
+ auto data = ""
return blobFromData(context, WTFMove(data), m_contentType);
}
Modified: trunk/Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp (280259 => 280260)
--- trunk/Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp 2021-07-23 22:05:03 UTC (rev 280260)
@@ -198,7 +198,7 @@
static inline Ref<BlobEvent> createDataAvailableEvent(ScriptExecutionContext* context, RefPtr<SharedBuffer>&& buffer, const String& mimeType, double timeCode)
{
- auto blob = buffer ? Blob::create(context, buffer->takeData(), mimeType) : Blob::create(context);
+ auto blob = buffer ? Blob::create(context, buffer->extractData(), mimeType) : Blob::create(context);
return BlobEvent::create(eventNames().dataavailableEvent, BlobEvent::Init { { false, false, false }, WTFMove(blob), timeCode }, BlobEvent::IsTrusted::Yes);
}
Modified: trunk/Source/WebCore/editing/WebCorePasteboardFileReader.cpp (280259 => 280260)
--- trunk/Source/WebCore/editing/WebCorePasteboardFileReader.cpp 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WebCore/editing/WebCorePasteboardFileReader.cpp 2021-07-23 22:05:03 UTC (rev 280260)
@@ -41,7 +41,7 @@
void WebCorePasteboardFileReader::readBuffer(const String& filename, const String& type, Ref<SharedBuffer>&& buffer)
{
- files.append(File::create(context.get(), Blob::create(context.get(), buffer->takeData(), type), filename));
+ files.append(File::create(context.get(), Blob::create(context.get(), buffer->extractData(), type), filename));
}
}
Modified: trunk/Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm (280259 => 280260)
--- trunk/Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm 2021-07-23 22:05:03 UTC (rev 280260)
@@ -253,10 +253,10 @@
auto fragment = document.createDocumentFragment();
if (supportsClientSideAttachmentData(frame)) {
- frame.editor().registerAttachmentIdentifier(attachment->ensureUniqueIdentifier(), contentType, defaultImageAttachmentName, WTFMove(buffer));
+ frame.editor().registerAttachmentIdentifier(attachment->ensureUniqueIdentifier(), contentType, defaultImageAttachmentName, buffer.copyRef());
if (contentTypeIsSuitableForInlineImageRepresentation(contentType)) {
auto image = HTMLImageElement::create(document);
- image->setAttributeWithoutSynchronization(HTMLNames::srcAttr, DOMURL::createObjectURL(document, Blob::create(&document, buffer->takeData(), contentType)));
+ image->setAttributeWithoutSynchronization(HTMLNames::srcAttr, DOMURL::createObjectURL(document, Blob::create(&document, buffer->extractData(), contentType)));
image->setAttachmentElement(WTFMove(attachment));
if (preferredSize.width)
image->setAttributeWithoutSynchronization(HTMLNames::widthAttr, AtomString::number(*preferredSize.width));
@@ -268,7 +268,7 @@
fragment->appendChild(WTFMove(attachment));
}
} else {
- attachment->setFile(File::create(&document, Blob::create(&document, buffer->takeData(), contentType), defaultImageAttachmentName), HTMLAttachmentElement::UpdateDisplayAttributes::Yes);
+ attachment->setFile(File::create(&document, Blob::create(&document, buffer->extractData(), contentType), defaultImageAttachmentName), HTMLAttachmentElement::UpdateDisplayAttributes::Yes);
fragment->appendChild(WTFMove(attachment));
}
return fragment;
@@ -685,7 +685,7 @@
if (shouldReplaceRichContentWithAttachments())
addFragment(createFragmentForImageAttachment(frame, document, WTFMove(buffer), type, preferredPresentationSize));
else
- addFragment(createFragmentForImageAndURL(document, DOMURL::createObjectURL(document, Blob::create(&document, buffer->takeData(), type)), preferredPresentationSize));
+ addFragment(createFragmentForImageAndURL(document, DOMURL::createObjectURL(document, Blob::create(&document, buffer->extractData(), type)), preferredPresentationSize));
return fragment;
}
Modified: trunk/Source/WebCore/editing/gtk/WebContentReaderGtk.cpp (280259 => 280260)
--- trunk/Source/WebCore/editing/gtk/WebContentReaderGtk.cpp 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WebCore/editing/gtk/WebContentReaderGtk.cpp 2021-07-23 22:05:03 UTC (rev 280260)
@@ -75,7 +75,7 @@
{
ASSERT(frame.document());
auto& document = *frame.document();
- addFragment(createFragmentForImageAndURL(document, DOMURL::createObjectURL(document, Blob::create(&document, buffer->takeData(), type)), preferredPresentationSize));
+ addFragment(createFragmentForImageAndURL(document, DOMURL::createObjectURL(document, Blob::create(&document, buffer->extractData(), type)), preferredPresentationSize));
return fragment;
}
Modified: trunk/Source/WebCore/html/HTMLAttachmentElement.cpp (280259 => 280260)
--- trunk/Source/WebCore/html/HTMLAttachmentElement.cpp 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WebCore/html/HTMLAttachmentElement.cpp 2021-07-23 22:05:03 UTC (rev 280260)
@@ -241,7 +241,7 @@
if (!mimeTypeIsSuitableForInlineImageAttachment(mimeType))
return;
- hostElement->setAttributeWithoutSynchronization(HTMLNames::srcAttr, DOMURL::createObjectURL(document(), Blob::create(&document(), buffer->takeData(), mimeType)));
+ hostElement->setAttributeWithoutSynchronization(HTMLNames::srcAttr, DOMURL::createObjectURL(document(), Blob::create(&document(), buffer->extractData(), mimeType)));
}
void HTMLAttachmentElement::updateThumbnail(const RefPtr<Image>& thumbnail)
Modified: trunk/Source/WebCore/platform/SharedBuffer.cpp (280259 => 280260)
--- trunk/Source/WebCore/platform/SharedBuffer.cpp 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WebCore/platform/SharedBuffer.cpp 2021-07-23 22:05:03 UTC (rev 280260)
@@ -122,6 +122,16 @@
return m_segments[0].segment->data();
}
+Vector<uint8_t> SharedBuffer::copyData() const
+{
+ Vector<uint8_t> data;
+ data.reserveInitialCapacity(size());
+ forEachSegment([&data](auto& span) {
+ data.uncheckedAppend(span);
+ });
+ return data;
+}
+
Vector<uint8_t> SharedBuffer::takeData()
{
if (m_segments.isEmpty())
Modified: trunk/Source/WebCore/platform/SharedBuffer.h (280259 => 280260)
--- trunk/Source/WebCore/platform/SharedBuffer.h 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WebCore/platform/SharedBuffer.h 2021-07-23 22:05:03 UTC (rev 280260)
@@ -101,10 +101,10 @@
// FIXME: Audit the call sites of this function and replace them with iteration if possible.
const uint8_t* data() const;
const char* dataAsCharPtr() const { return reinterpret_cast<const char*>(data()); }
- Vector<uint8_t> copyData() { return { data(), size() }; }
+ Vector<uint8_t> copyData() const;
- // Combines all the segments into a Vector and returns that vector after clearing the SharedBuffer.
- Vector<uint8_t> takeData();
+ // Similar to copyData() but avoids copying and will take the data instead when it is safe (The SharedBuffer is not shared).
+ Vector<uint8_t> extractData();
// Creates an ArrayBuffer and copies this SharedBuffer's contents to that
// ArrayBuffer without merging segmented buffers into a flat buffer.
@@ -227,6 +227,9 @@
#endif
void combineIntoOneSegment() const;
+
+ // Combines all the segments into a Vector and returns that vector after clearing the SharedBuffer.
+ Vector<uint8_t> takeData();
static RefPtr<SharedBuffer> createFromReadingFile(const String& filePath);
@@ -239,6 +242,13 @@
#endif
};
+inline Vector<uint8_t> SharedBuffer::extractData()
+{
+ if (hasOneRef())
+ return takeData();
+ return copyData();
+}
+
class WEBCORE_EXPORT SharedBufferDataView {
public:
SharedBufferDataView(Ref<SharedBuffer::DataSegment>&&, size_t);
Modified: trunk/Source/WebCore/xml/XMLHttpRequest.cpp (280259 => 280260)
--- trunk/Source/WebCore/xml/XMLHttpRequest.cpp 2021-07-23 21:43:37 UTC (rev 280259)
+++ trunk/Source/WebCore/xml/XMLHttpRequest.cpp 2021-07-23 22:05:03 UTC (rev 280260)
@@ -208,7 +208,7 @@
// FIXME: We just received the data from NetworkProcess, and are sending it back. This is inefficient.
Vector<uint8_t> data;
if (m_binaryResponseBuilder)
- data = "" nullptr)->takeData();
+ data = "" nullptr)->extractData();
String normalizedContentType = Blob::normalizedContentType(responseMIMEType(FinalMIMEType::Yes)); // responseMIMEType defaults to text/xml which may be incorrect.
return Blob::create(scriptExecutionContext(), WTFMove(data), normalizedContentType);
}
