Skip to content
Permalink
Browse files
Avoid flattening a SharedBuffer when reading reading it through Share…
…dBufferChunkReader.

https://bugs.webkit.org/show_bug.cgi?id=233481
rdar://problem/85733358

Source/WebCore:

Reviewed by Cameron McCormack

SharedBufferChunkReader required the SharedBuffer to be flattened which
would mutate the underlying SharedBuffer.
The peek method was also incorrect if the data read overlapped multiple
segments (it would return too many characters) and some members were used
without being initialised.

Additionally, add a similar read method in SharedBuffer that will be used
in bug 233030.

Covered in existing tests, API tests added.

* Headers.cmake:
* WebCore.xcodeproj/project.pbxproj:
* platform/SharedBuffer.cpp:
(WebCore::SharedBuffer::getSomeData const):
(WebCore::SharedBuffer::getSegmentForPosition const):
(WebCore::SharedBuffer::read const):
* platform/SharedBuffer.h:
* platform/SharedBufferChunkReader.cpp:
(WebCore::SharedBufferChunkReader::SharedBufferChunkReader):
(WebCore::SharedBufferChunkReader::nextChunk):
(WebCore::SharedBufferChunkReader::peek):
* platform/SharedBufferChunkReader.h:

Tools:

Reviewed by Cameron McCormack.

Add API tests.
API tests original data come from the original bug that added
SharedBufferChunkReader (bug 59946) and then expanded.

* TestWebKitAPI/Tests/WebCore/SharedBuffer.cpp:
(TestWebKitAPI::TEST_F):
(TestWebKitAPI::readAllChunks):
(TestWebKitAPI::checkChunks):
(TestWebKitAPI::checkDataInRange):
* TestWebKitAPI/Tests/WebCore/SharedBufferTest.h:



Canonical link: https://commits.webkit.org/244554@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@286171 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
jyavenard committed Nov 26, 2021
1 parent e95560d commit 52e6d91c3c34a16adc0ded116ebf8548c6b249cb
Showing 10 changed files with 448 additions and 48 deletions.
@@ -1,3 +1,35 @@
2021-11-25 Jean-Yves Avenard <jya@apple.com>

Avoid flattening a SharedBuffer when reading reading it through SharedBufferChunkReader.
https://bugs.webkit.org/show_bug.cgi?id=233481
rdar://problem/85733358

Reviewed by Cameron McCormack

SharedBufferChunkReader required the SharedBuffer to be flattened which
would mutate the underlying SharedBuffer.
The peek method was also incorrect if the data read overlapped multiple
segments (it would return too many characters) and some members were used
without being initialised.

Additionally, add a similar read method in SharedBuffer that will be used
in bug 233030.

Covered in existing tests, API tests added.

* Headers.cmake:
* WebCore.xcodeproj/project.pbxproj:
* platform/SharedBuffer.cpp:
(WebCore::SharedBuffer::getSomeData const):
(WebCore::SharedBuffer::getSegmentForPosition const):
(WebCore::SharedBuffer::read const):
* platform/SharedBuffer.h:
* platform/SharedBufferChunkReader.cpp:
(WebCore::SharedBufferChunkReader::SharedBufferChunkReader):
(WebCore::SharedBufferChunkReader::nextChunk):
(WebCore::SharedBufferChunkReader::peek):
* platform/SharedBufferChunkReader.h:

2021-11-25 Antti Koivisto <antti@apple.com>

[:has() pseudo-class] Invalidation support for adding and removing nodes
@@ -1171,6 +1171,7 @@ set(WebCore_PRIVATE_FRAMEWORK_HEADERS
platform/SerializedPlatformDataCue.h
platform/SerializedPlatformDataCueValue.h
platform/SharedBuffer.h
platform/SharedBufferChunkReader.h
platform/SharedStringHash.h
platform/SleepDisabler.h
platform/SleepDisablerClient.h
@@ -488,6 +488,7 @@
1A494EDF0A123F4C00FDAFC1 /* JSDocumentFragment.h in Headers */ = {isa = PBXBuildFile; fileRef = 1A494EDD0A123F4C00FDAFC1 /* JSDocumentFragment.h */; };
1A4A2DF00A1B852A00C807F8 /* JSHTMLAnchorElement.h in Headers */ = {isa = PBXBuildFile; fileRef = 1A4A2DEC0A1B852A00C807F8 /* JSHTMLAnchorElement.h */; };
1A4A954E0B4EDCCB002D8C3C /* SharedBuffer.h in Headers */ = {isa = PBXBuildFile; fileRef = 1A4A954C0B4EDCCB002D8C3C /* SharedBuffer.h */; settings = {ATTRIBUTES = (Private, ); }; };
51FA2EDF27506FDE0011C15D /* SharedBufferChunkReader.h in Headers */ = {isa = PBXBuildFile; fileRef = 51FA2ED6274F78370011C15D /* SharedBufferChunkReader.h */; settings = {ATTRIBUTES = (Private, ); }; };
1A4DA4221CDD3A8300F4473C /* LinkIconCollector.h in Headers */ = {isa = PBXBuildFile; fileRef = 1A4DA4201CDD3A8300F4473C /* LinkIconCollector.h */; settings = {ATTRIBUTES = (Private, ); }; };
1A569D120D7E2B82007C3983 /* objc_class.h in Headers */ = {isa = PBXBuildFile; fileRef = 1A569CE30D7E2B82007C3983 /* objc_class.h */; };
1A569D130D7E2B82007C3983 /* objc_class.mm in Sources */ = {isa = PBXBuildFile; fileRef = 1A569CE40D7E2B82007C3983 /* objc_class.mm */; };
@@ -9606,6 +9607,8 @@
51F798EC1BE880D3008AE491 /* IDBIndexInfo.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = IDBIndexInfo.h; sourceTree = "<group>"; };
51F886BE1F32920700C193EF /* JSNavigatorServiceWorker.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSNavigatorServiceWorker.cpp; sourceTree = "<group>"; };
51F886BF1F32920700C193EF /* JSNavigatorServiceWorker.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSNavigatorServiceWorker.h; sourceTree = "<group>"; };
51FA2ED6274F78370011C15D /* SharedBufferChunkReader.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SharedBufferChunkReader.h; sourceTree = "<group>"; };
51FA2ED7274F78370011C15D /* SharedBufferChunkReader.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = SharedBufferChunkReader.cpp; sourceTree = "<group>"; };
51FB5502113E3E9100821176 /* JSCloseEvent.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSCloseEvent.h; sourceTree = "<group>"; };
51FB5503113E3E9100821176 /* JSCloseEvent.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSCloseEvent.cpp; sourceTree = "<group>"; };
51FB67D91AE6B5E400D06C5A /* ContentExtensionStyleSheet.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ContentExtensionStyleSheet.cpp; sourceTree = "<group>"; };
@@ -29213,6 +29216,8 @@
076D1C1D23F6D9D800D95B06 /* SerializedPlatformDataCueValue.h */,
1A4A954B0B4EDCCB002D8C3C /* SharedBuffer.cpp */,
1A4A954C0B4EDCCB002D8C3C /* SharedBuffer.h */,
51FA2ED7274F78370011C15D /* SharedBufferChunkReader.cpp */,
51FA2ED6274F78370011C15D /* SharedBufferChunkReader.h */,
834DFACE1F7DAE5700C2725B /* SharedStringHash.cpp */,
834DFACC1F7DAE5600C2725B /* SharedStringHash.h */,
93309EA0099EB78C0056E581 /* SharedTimer.h */,
@@ -36222,6 +36227,7 @@
FD1AF1501656F15100C6D4F7 /* ShapeValue.h in Headers */,
1D9F0FC12122029B005D8FD4 /* ShareData.h in Headers */,
1A4A954E0B4EDCCB002D8C3C /* SharedBuffer.h in Headers */,
51FA2EDF27506FDE0011C15D /* SharedBufferChunkReader.h in Headers */,
510A91DC24CF46FE00BFD89C /* SharedGamepadValue.h in Headers */,
CD36C16B260A65CC00C8C529 /* SharedRoutingArbitrator.h in Headers */,
834DFAD01F7DAE5D00C2725B /* SharedStringHash.h in Headers */,
@@ -155,14 +155,20 @@ Vector<uint8_t> SharedBuffer::takeData()
}

SharedBufferDataView SharedBuffer::getSomeData(size_t position) const
{
const DataSegmentVectorEntry* element = getSegmentForPosition(position);
return { element->segment.copyRef(), position - element->beginPosition };
}

const SharedBuffer::DataSegmentVectorEntry* SharedBuffer::getSegmentForPosition(size_t position) const
{
RELEASE_ASSERT(position < m_size);
auto comparator = [](const size_t& position, const DataSegmentVectorEntry& entry) {
return position < entry.beginPosition;
};
const DataSegmentVectorEntry* element = std::upper_bound(m_segments.begin(), m_segments.end(), position, comparator);
element--; // std::upper_bound gives a pointer to the element that is greater than position. We want the element just before that.
return { element->segment.copyRef(), position - element->beginPosition };
return element;
}

String SharedBuffer::toHexString() const
@@ -277,6 +283,33 @@ bool SharedBuffer::startsWith(const Span<const uint8_t>& prefix) const
return false;
}

Vector<uint8_t> SharedBuffer::read(size_t offset, size_t length) const
{
Vector<uint8_t> data;
if (offset >= size())
return data;
auto remaining = std::min(length, size() - offset);
if (!remaining)
return data;

data.reserveInitialCapacity(remaining);
auto* currentSegment = getSegmentForPosition(offset);
size_t offsetInSegment = offset - currentSegment->beginPosition;
size_t availableInSegment = std::min(currentSegment->segment->size() - offsetInSegment, remaining);
data.append(currentSegment->segment->data() + offsetInSegment, availableInSegment);

remaining -= availableInSegment;

auto* afterLastSegment = end();

while (remaining && ++currentSegment != afterLastSegment) {
size_t lengthInSegment = std::min(currentSegment->segment->size(), remaining);
data.append(currentSegment->segment->data(), lengthInSegment);
remaining -= lengthInSegment;
}
return data;
}

void SharedBuffer::copyTo(void* destination, size_t length) const
{
return copyTo(destination, 0, length);
@@ -102,6 +102,7 @@ class WEBCORE_EXPORT SharedBuffer : public ThreadSafeRefCounted<SharedBuffer> {
const uint8_t* data() const;
const char* dataAsCharPtr() const { return reinterpret_cast<const char*>(data()); }
Vector<uint8_t> copyData() const;
Vector<uint8_t> read(size_t offset, size_t length) const;

// 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();
@@ -244,7 +245,9 @@ class WEBCORE_EXPORT SharedBuffer : public ThreadSafeRefCounted<SharedBuffer> {

// Combines all the segments into a Vector and returns that vector after clearing the SharedBuffer.
Vector<uint8_t> takeData();


const DataSegmentVectorEntry* getSegmentForPosition(size_t positition) const;

static RefPtr<SharedBuffer> createFromReadingFile(const String& filePath);

size_t m_size { 0 };
@@ -1,5 +1,6 @@
/*
* Copyright (C) 2011 Google Inc. All rights reserved.
* Copyright (C) 2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -31,23 +32,22 @@
#include "config.h"
#include "SharedBufferChunkReader.h"

#if ENABLE(MHTML)

// FIXME: This class is overkill. Remove this class and just iterate the segments of a SharedBuffer
// using the cool new SharedBuffer::begin() and SharedBuffer::end() instead of using this class.

#include "SharedBuffer.h"

namespace WebCore {

#if ENABLE(MHTML)

SharedBufferChunkReader::SharedBufferChunkReader(SharedBuffer* buffer, const Vector<char>& separator)
: m_buffer(buffer)
: m_iteratorCurrent(buffer->begin())
, m_iteratorEnd(buffer->end())
, m_segment(m_iteratorCurrent != m_iteratorEnd ? m_iteratorCurrent->segment->data() : nullptr)
, m_separator(separator)
{
}

SharedBufferChunkReader::SharedBufferChunkReader(SharedBuffer* buffer, const char* separator)
: m_buffer(buffer)
: m_iteratorCurrent(buffer->begin())
, m_iteratorEnd(buffer->end())
, m_segment(m_iteratorCurrent != m_iteratorEnd ? m_iteratorCurrent->segment->data() : nullptr)
{
setSeparator(separator);
}
@@ -65,12 +65,13 @@ void SharedBufferChunkReader::setSeparator(const char* separator)

bool SharedBufferChunkReader::nextChunk(Vector<uint8_t>& chunk, bool includeSeparator)
{
if (m_reachedEndOfFile)
if (m_iteratorCurrent == m_iteratorEnd)
return false;

chunk.clear();
while (true) {
while (m_segmentIndex < m_segmentLength) {
while (m_segmentIndex < m_iteratorCurrent->segment->size()) {
// FIXME: The existing code to check for separators doesn't work correctly with arbitrary separator strings.
auto currentCharacter = m_segment[m_segmentIndex++];
if (currentCharacter != m_separator[m_separatorIndex]) {
if (m_separatorIndex > 0) {
@@ -92,18 +93,15 @@ bool SharedBufferChunkReader::nextChunk(Vector<uint8_t>& chunk, bool includeSepa

// Read the next segment.
m_segmentIndex = 0;
m_bufferPosition += m_segmentLength;
// Let's pretend all the data is in one block.
// FIXME: This class should be removed in favor of just iterating the segments of the SharedBuffer.
m_segment = m_buffer->data() + m_bufferPosition;
m_segmentLength = m_buffer->size() - m_bufferPosition;
if (!m_segmentLength) {
m_reachedEndOfFile = true;
if (++m_iteratorCurrent == m_iteratorEnd) {
m_segment = nullptr;
if (m_separatorIndex > 0)
chunk.append(reinterpret_cast<const uint8_t*>(m_separator.data()), m_separatorIndex);
return !chunk.isEmpty();
}
m_segment = m_iteratorCurrent->segment->data();
}

ASSERT_NOT_REACHED();
return false;
}
@@ -120,31 +118,28 @@ String SharedBufferChunkReader::nextChunkAsUTF8StringWithLatin1Fallback(bool inc
size_t SharedBufferChunkReader::peek(Vector<uint8_t>& data, size_t requestedSize)
{
data.clear();
if (requestedSize <= m_segmentLength - m_segmentIndex) {
data.append(m_segment + m_segmentIndex, requestedSize);
return requestedSize;
}
if (m_iteratorCurrent == m_iteratorEnd)
return 0;

size_t availableInSegment = std::min(m_iteratorCurrent->segment->size() - m_segmentIndex, requestedSize);
data.append(m_segment + m_segmentIndex, availableInSegment);

size_t readBytesCount = availableInSegment;
requestedSize -= readBytesCount;

auto currentSegment = m_iteratorCurrent;

size_t readBytesCount = m_segmentLength - m_segmentIndex;
data.append(m_segment + m_segmentIndex, readBytesCount);

size_t bufferPosition = m_bufferPosition + m_segmentLength;
const uint8_t* segment = nullptr;

// Let's pretend all the data is in one block.
// FIXME: This class should be removed in favor of just iterating the segments of the SharedBuffer.
if (bufferPosition != m_buffer->size()) {
segment = m_buffer->data() + bufferPosition;
size_t segmentLength = m_buffer->size() - bufferPosition;
if (segmentLength > requestedSize)
segmentLength = requestedSize;
data.append(segment, segmentLength);
readBytesCount += segmentLength;
bufferPosition += segmentLength;
while (requestedSize && ++currentSegment != m_iteratorEnd) {
const uint8_t* segment = currentSegment->segment->data();
size_t lengthInSegment = std::min(currentSegment->segment->size(), requestedSize);
data.append(segment, lengthInSegment);
readBytesCount += lengthInSegment;
requestedSize -= lengthInSegment;
}
return readBytesCount;
}

#endif

}

#endif
@@ -1,5 +1,6 @@
/*
* Copyright (C) 2011 Google Inc. All rights reserved.
* Copyright (C) 2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -32,14 +33,13 @@

#if ENABLE(MHTML)

#include "SharedBuffer.h"
#include <wtf/Vector.h>
#include <wtf/text/WTFString.h>

namespace WebCore {

class SharedBuffer;

class SharedBufferChunkReader {
class WEBCORE_EXPORT SharedBufferChunkReader {
public:
SharedBufferChunkReader(SharedBuffer*, const Vector<char>& separator);
SharedBufferChunkReader(SharedBuffer*, const char* separator);
@@ -58,12 +58,10 @@ class SharedBufferChunkReader {
size_t peek(Vector<uint8_t>&, size_t);

private:
SharedBuffer* m_buffer;
size_t m_bufferPosition { 0 };
SharedBuffer::DataSegmentVector::const_iterator m_iteratorCurrent;
const SharedBuffer::DataSegmentVector::const_iterator m_iteratorEnd;
const uint8_t* m_segment { nullptr };
size_t m_segmentLength { 0 };
size_t m_segmentIndex { 0 };
bool m_reachedEndOfFile;
Vector<char> m_separator { false };
size_t m_separatorIndex { 0 };
};
@@ -1,3 +1,22 @@
2021-11-25 Jean-Yves Avenard <jya@apple.com>

Avoid flattening a SharedBuffer when reading reading it through SharedBufferChunkReader.
https://bugs.webkit.org/show_bug.cgi?id=233481
rdar://problem/85733358

Reviewed by Cameron McCormack.

Add API tests.
API tests original data come from the original bug that added
SharedBufferChunkReader (bug 59946) and then expanded.

* TestWebKitAPI/Tests/WebCore/SharedBuffer.cpp:
(TestWebKitAPI::TEST_F):
(TestWebKitAPI::readAllChunks):
(TestWebKitAPI::checkChunks):
(TestWebKitAPI::checkDataInRange):
* TestWebKitAPI/Tests/WebCore/SharedBufferTest.h:

2021-11-25 Lauro Moura <lmoura@igalia.com>

[GTK] Gardening API test authentication-success flaky timeout

0 comments on commit 52e6d91

Please sign in to comment.