- Revision
- 209407
- Author
- ander...@apple.com
- Date
- 2016-12-06 11:20:19 -0800 (Tue, 06 Dec 2016)
Log Message
Don't memcpy out of line data
https://bugs.webkit.org/show_bug.cgi?id=165434
Reviewed by Sam Weinig.
Change the Decoder constructor to take a buffer deallocator parameter. If the buffer deallocator is null, the
data will be copied as before. Otherwise, the memory will be adopted by the Decoder object, and will be deallocated
by invoking the data deallocator.
* Platform/IPC/Decoder.cpp:
(IPC::copyBuffer):
Add a new helper.
(IPC::Decoder::Decoder):
Copy the buffer if the deallocator is null.
(IPC::Decoder::~Decoder):
Invoke the deallocator or call fastFree if it is null.
(IPC::Decoder::unwrapForTesting):
Update constructor.
(IPC::roundUpToAlignment):
(IPC::Decoder::alignBufferPosition):
(IPC::Decoder::decodeVariableLengthByteArray):
(IPC::decodeValueFromBuffer):
Change all these to deal with const pointers.
* Platform/IPC/Decoder.h:
Add new members.
* Platform/IPC/mac/ConnectionMac.mm:
(IPC::createMessageDecoder):
When we have out of line data, pass a deallocator that calls vm_deallocate, instead of copying the data
and then immediately throwing the original away.
Modified Paths
Diff
Modified: trunk/Source/WebKit2/ChangeLog (209406 => 209407)
--- trunk/Source/WebKit2/ChangeLog 2016-12-06 19:07:56 UTC (rev 209406)
+++ trunk/Source/WebKit2/ChangeLog 2016-12-06 19:20:19 UTC (rev 209407)
@@ -1,3 +1,41 @@
+2016-12-05 Anders Carlsson <ander...@apple.com>
+
+ Don't memcpy out of line data
+ https://bugs.webkit.org/show_bug.cgi?id=165434
+
+ Reviewed by Sam Weinig.
+
+ Change the Decoder constructor to take a buffer deallocator parameter. If the buffer deallocator is null, the
+ data will be copied as before. Otherwise, the memory will be adopted by the Decoder object, and will be deallocated
+ by invoking the data deallocator.
+
+ * Platform/IPC/Decoder.cpp:
+ (IPC::copyBuffer):
+ Add a new helper.
+
+ (IPC::Decoder::Decoder):
+ Copy the buffer if the deallocator is null.
+
+ (IPC::Decoder::~Decoder):
+ Invoke the deallocator or call fastFree if it is null.
+
+ (IPC::Decoder::unwrapForTesting):
+ Update constructor.
+
+ (IPC::roundUpToAlignment):
+ (IPC::Decoder::alignBufferPosition):
+ (IPC::Decoder::decodeVariableLengthByteArray):
+ (IPC::decodeValueFromBuffer):
+ Change all these to deal with const pointers.
+
+ * Platform/IPC/Decoder.h:
+ Add new members.
+
+ * Platform/IPC/mac/ConnectionMac.mm:
+ (IPC::createMessageDecoder):
+ When we have out of line data, pass a deallocator that calls vm_deallocate, instead of copying the data
+ and then immediately throwing the original away.
+
2016-12-06 Tim Horton <timothy_hor...@apple.com>
Almost half-second stall scrolling apple.com because of synchronous getPositionInformation
Modified: trunk/Source/WebKit2/Platform/IPC/Decoder.cpp (209406 => 209407)
--- trunk/Source/WebKit2/Platform/IPC/Decoder.cpp 2016-12-06 19:07:56 UTC (rev 209406)
+++ trunk/Source/WebKit2/Platform/IPC/Decoder.cpp 2016-12-06 19:20:19 UTC (rev 209407)
@@ -36,12 +36,23 @@
namespace IPC {
-Decoder::Decoder(const DataReference& buffer, Vector<Attachment> attachments)
+static const uint8_t* copyBuffer(const uint8_t* buffer, size_t bufferSize)
{
- initialize(buffer.data(), buffer.size());
+ auto bufferCopy = static_cast<uint8_t*>(fastMalloc(bufferSize));
+ memcpy(bufferCopy, buffer, bufferSize);
- m_attachments = WTFMove(attachments);
+ return bufferCopy;
+}
+Decoder::Decoder(const uint8_t* buffer, size_t bufferSize, void (*bufferDeallocator)(const uint8_t*, size_t), Vector<Attachment> attachments)
+ : m_buffer { bufferDeallocator ? buffer : copyBuffer(buffer, bufferSize) }
+ , m_bufferPos { m_buffer }
+ , m_bufferEnd { m_buffer + bufferSize }
+ , m_bufferDeallocator { bufferDeallocator }
+ , m_attachments { WTFMove(attachments) }
+{
+ ASSERT(!(reinterpret_cast<uintptr_t>(m_buffer) % alignof(uint64_t)));
+
if (!decode(m_messageFlags))
return;
@@ -58,7 +69,12 @@
Decoder::~Decoder()
{
ASSERT(m_buffer);
- fastFree(m_buffer);
+
+ if (m_bufferDeallocator)
+ m_bufferDeallocator(m_buffer, m_bufferEnd - m_buffer);
+ else
+ fastFree(const_cast<uint8_t*>(m_buffer));
+
// FIXME: We need to dispose of the mach ports in cases of failure.
#if HAVE(QOS_CLASSES)
@@ -103,10 +119,10 @@
if (!decoder.decode(wrappedMessage))
return nullptr;
- return std::make_unique<Decoder>(wrappedMessage, WTFMove(attachments));
+ return std::make_unique<Decoder>(wrappedMessage.data(), wrappedMessage.size(), nullptr, WTFMove(attachments));
}
-static inline uint8_t* roundUpToAlignment(uint8_t* ptr, unsigned alignment)
+static inline const uint8_t* roundUpToAlignment(const uint8_t* ptr, unsigned alignment)
{
// Assert that the alignment is a power of 2.
ASSERT(alignment && !(alignment & (alignment - 1)));
@@ -115,17 +131,6 @@
return reinterpret_cast<uint8_t*>((reinterpret_cast<uintptr_t>(ptr) + alignmentMask) & ~alignmentMask);
}
-void Decoder::initialize(const uint8_t* buffer, size_t bufferSize)
-{
- m_buffer = static_cast<uint8_t*>(fastMalloc(bufferSize));
-
- ASSERT(!(reinterpret_cast<uintptr_t>(m_buffer) % alignof(uint64_t)));
-
- m_bufferPos = m_buffer;
- m_bufferEnd = m_buffer + bufferSize;
- memcpy(m_buffer, buffer, bufferSize);
-}
-
static inline bool alignedBufferIsLargeEnoughToContain(const uint8_t* alignedPosition, const uint8_t* bufferEnd, size_t size)
{
return bufferEnd >= alignedPosition && static_cast<size_t>(bufferEnd - alignedPosition) >= size;
@@ -133,7 +138,7 @@
bool Decoder::alignBufferPosition(unsigned alignment, size_t size)
{
- uint8_t* alignedPosition = roundUpToAlignment(m_bufferPos, alignment);
+ const uint8_t* alignedPosition = roundUpToAlignment(m_bufferPos, alignment);
if (!alignedBufferIsLargeEnoughToContain(alignedPosition, m_bufferEnd, size)) {
// We've walked off the end of this buffer.
markInvalid();
@@ -169,7 +174,7 @@
if (!alignBufferPosition(1, size))
return false;
- uint8_t* data = ""
+ const uint8_t* data = ""
m_bufferPos += size;
dataReference = DataReference(data, size);
@@ -177,7 +182,7 @@
}
template<typename Type>
-static void decodeValueFromBuffer(Type& value, uint8_t*& bufferPosition)
+static void decodeValueFromBuffer(Type& value, const uint8_t*& bufferPosition)
{
memcpy(&value, bufferPosition, sizeof(value));
bufferPosition += sizeof(Type);
Modified: trunk/Source/WebKit2/Platform/IPC/Decoder.h (209406 => 209407)
--- trunk/Source/WebKit2/Platform/IPC/Decoder.h 2016-12-06 19:07:56 UTC (rev 209406)
+++ trunk/Source/WebKit2/Platform/IPC/Decoder.h 2016-12-06 19:20:19 UTC (rev 209407)
@@ -43,9 +43,12 @@
class Decoder {
WTF_MAKE_FAST_ALLOCATED;
public:
- Decoder(const DataReference& buffer, Vector<Attachment>);
+ Decoder(const uint8_t* buffer, size_t bufferSize, void (*bufferDeallocator)(const uint8_t*, size_t), Vector<Attachment>);
~Decoder();
+ Decoder(const Decoder&) = delete;
+ Decoder(Decoder&&) = delete;
+
StringReference messageReceiverName() const { return m_messageReceiverName; }
StringReference messageName() const { return m_messageName; }
uint64_t destinationID() const { return m_destinationID; }
@@ -130,16 +133,14 @@
static const bool isIPCDecoder = true;
-protected:
- void initialize(const uint8_t* buffer, size_t bufferSize);
-
+private:
bool alignBufferPosition(unsigned alignment, size_t);
bool bufferIsLargeEnoughToContain(unsigned alignment, size_t) const;
-private:
- uint8_t* m_buffer;
- uint8_t* m_bufferPos;
- uint8_t* m_bufferEnd;
+ const uint8_t* m_buffer;
+ const uint8_t* m_bufferPos;
+ const uint8_t* m_bufferEnd;
+ void (*m_bufferDeallocator)(const uint8_t*, size_t);
Vector<Attachment> m_attachments;
@@ -149,7 +150,6 @@
uint64_t m_destinationID;
-
#if PLATFORM(MAC)
std::unique_ptr<ImportanceAssertion> m_importanceAssertion;
#endif
Modified: trunk/Source/WebKit2/Platform/IPC/mac/ConnectionMac.mm (209406 => 209407)
--- trunk/Source/WebKit2/Platform/IPC/mac/ConnectionMac.mm 2016-12-06 19:07:56 UTC (rev 209406)
+++ trunk/Source/WebKit2/Platform/IPC/mac/ConnectionMac.mm 2016-12-06 19:20:19 UTC (rev 209407)
@@ -400,7 +400,7 @@
uint8_t* body = reinterpret_cast<uint8_t*>(header + 1);
size_t bodySize = header->msgh_size - sizeof(mach_msg_header_t);
- return std::make_unique<Decoder>(DataReference(body, bodySize), Vector<Attachment>());
+ return std::make_unique<Decoder>(body, bodySize, nullptr, Vector<Attachment> { });
}
bool messageBodyIsOOL = header->msgh_id & MessageBodyIsOutOfLine;
@@ -439,10 +439,10 @@
uint8_t* messageBody = static_cast<uint8_t*>(descriptor->out_of_line.address);
size_t messageBodySize = descriptor->out_of_line.size;
- auto decoder = std::make_unique<Decoder>(DataReference(messageBody, messageBodySize), WTFMove(attachments));
+ auto decoder = std::make_unique<Decoder>(messageBody, messageBodySize, [](const uint8_t* buffer, size_t length) {
+ vm_deallocate(mach_task_self(), reinterpret_cast<vm_address_t>(buffer), length);
+ }, WTFMove(attachments));
- vm_deallocate(mach_task_self(), reinterpret_cast<vm_address_t>(descriptor->out_of_line.address), descriptor->out_of_line.size);
-
return decoder;
}
@@ -449,7 +449,7 @@
uint8_t* messageBody = descriptorData;
size_t messageBodySize = header->msgh_size - (descriptorData - reinterpret_cast<uint8_t*>(header));
- return std::make_unique<Decoder>(DataReference(messageBody, messageBodySize), attachments);
+ return std::make_unique<Decoder>(messageBody, messageBodySize, nullptr, attachments);
}
// The receive buffer size should always include the maximum trailer size.
