Attachment 368346 Details for Bug 197323 – Patch

[patch] Patch

bug-197323-20190426135516.patch (text/plain), 9.86 KB, created by Chris Dumez on 2019-04-26 13:55:17 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-04-26 13:55:17 PDT

Size: 9.86 KB

patch

obsolete

>Subversion Revision: 244685
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 0c67f75b22d6a11e78580f19f57e3d3164a12038..ca55301c4e13ecdc7041ca0aace53f0dfa7bc3de 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,52 @@
>+2019-04-26  Chris Dumez  <cdumez@apple.com>
>+
>+        Improve safety of MachMessage class
>+        https://bugs.webkit.org/show_bug.cgi?id=197323
>+        <rdar://problem/44291920>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Improve safety of MachMessage class and clean things up a bit.
>+
>+        * Platform/IPC/mac/ConnectionMac.mm:
>+        Set inlineMessageMaxSize to PAGE_SIZE instead of hardcoding 4096.
>+
>+        (IPC::Connection::sendOutgoingMessage):
>+        - Pass MessageReceiverName / MessageName when constructing the MachMessage rather
>+          than setting them afterwards since they never change for a given MachMessage.
>+        - Set header->msgh_id to the right value right away instead of setting it first
>+          to inlineBodyMessageID and then later fixing it to be outOfLineBodyMessageID
>+          when the body is out of line.
>+        - When messageBodyIsOOL was true, we would call getDescriptorAndSkip() which
>+          would advance the pointer by sizeof(mach_msg_port_descriptor_t), even though
>+          the descriptor type is mach_msg_ool_descriptor_t. This would not matter in
>+          the end because we would not use the messageData pointer after this but
>+          still.
>+
>+        * Platform/IPC/mac/MachMessage.cpp:
>+        (IPC::MachMessage::create):
>+        Use fastZeroedMalloc() instead of fastMalloc() for safety, given that this class
>+        has a mach_msg_header_t flexible array member. This is what is recommended by the
>+        mach documentation. It is much safer because it otherwize relies on the user
>+        (Connection::sendOutgoingMessage()) to initialize ALL the message members
>+        correctly. I suspect this was the cause of <rdar://problem/44291920> because
>+        Connection::sendOutgoingMessage() would fail to initialize header->msgh_voucher_port
>+        and the MachMessage destructor would then call mach_msg_destroy(header()), which
>+        would mach_msg_destroy_port(header->msgh_voucher_port).
>+
>+        (IPC::MachMessage::MachMessage):
>+        Pass MessageReceiverName / MessageName when constructing the MachMessage rather
>+        than setting them afterwards since they never change for a given MachMessage.
>+
>+        (IPC::MachMessage::messageSize):
>+        Drop if checks for portDescriptorCount and memoryDescriptorCount since the logic
>+        will do the right thing even if they are 0.
>+
>+        * Platform/IPC/mac/MachMessage.h:
>+        (IPC::MachMessage::header):
>+        (IPC::MachMessage::messageReceiverName const):
>+        (IPC::MachMessage::messageName const):
>+
> 2019-04-25  Myles C. Maxfield  <mmaxfield@apple.com>
> 
>         [iOS] Add internal setting to force -webkit-text-size-adjust to "auto"
>diff --git a/Source/WebKit/Platform/IPC/mac/ConnectionMac.mm b/Source/WebKit/Platform/IPC/mac/ConnectionMac.mm
>index d94193e59a99f1e14c1c277fb322f26647733d9d..44af74fe13451f692e9f9be1ae49f2a5aabc0037 100644
>--- a/Source/WebKit/Platform/IPC/mac/ConnectionMac.mm
>+++ b/Source/WebKit/Platform/IPC/mac/ConnectionMac.mm
>@@ -70,7 +70,7 @@ extern "C" AXError _AXUIElementNotifyProcessSuspendStatus(AXSuspendStatus);
> 
> namespace IPC {
> 
>-static const size_t inlineMessageMaxSize = 4096;
>+static const size_t inlineMessageMaxSize = PAGE_SIZE;
> 
> // Arbitrary message IDs that do not collide with Mach notification messages (used my initials).
> constexpr mach_msg_id_t inlineBodyMessageID = 0xdba0dba;
>@@ -306,16 +306,14 @@ bool Connection::sendOutgoingMessage(std::unique_ptr<Encoder> encoder)
>         messageSize = MachMessage::messageSize(0, numberOfPortDescriptors, messageBodyIsOOL);
>     }
> 
>-    auto message = MachMessage::create(messageSize);
>-    message->setMessageReceiverName(encoder->messageReceiverName().toString());
>-    message->setMessageName(encoder->messageName().toString());
>+    auto message = MachMessage::create(encoder->messageReceiverName().toString(), encoder->messageName().toString(), messageSize);
> 
>     auto* header = message->header();
>     header->msgh_bits = MACH_MSGH_BITS(MACH_MSG_TYPE_COPY_SEND, 0);
>     header->msgh_size = messageSize;
>     header->msgh_remote_port = m_sendPort;
>     header->msgh_local_port = MACH_PORT_NULL;
>-    header->msgh_id = inlineBodyMessageID;
>+    header->msgh_id = messageBodyIsOOL ? outOfLineBodyMessageID : inlineBodyMessageID;
> 
>     auto* messageData = reinterpret_cast<uint8_t*>(header + 1);
> 
>@@ -327,14 +325,14 @@ bool Connection::sendOutgoingMessage(std::unique_ptr<Encoder> encoder)
>         body->msgh_descriptor_count = numberOfPortDescriptors + messageBodyIsOOL;
>         messageData = reinterpret_cast<uint8_t*>(body + 1);
> 
>-        auto getDescriptorAndSkip = [](uint8_t*& data) {
>-            return reinterpret_cast<mach_msg_descriptor_t*>(std::exchange(data, data + sizeof(mach_msg_port_descriptor_t)));
>+        auto getDescriptorAndAdvance = [](uint8_t*& data, std::size_t toAdvance) {
>+            return reinterpret_cast<mach_msg_descriptor_t*>(std::exchange(data, data + toAdvance));
>         };
> 
>         for (auto& attachment : attachments) {
>             ASSERT(attachment.type() == Attachment::MachPortType);
>             if (attachment.type() == Attachment::MachPortType) {
>-                auto* descriptor = getDescriptorAndSkip(messageData);
>+                auto* descriptor = getDescriptorAndAdvance(messageData, sizeof(mach_msg_port_descriptor_t));
>                 descriptor->port.name = attachment.port();
>                 descriptor->port.disposition = attachment.disposition();
>                 descriptor->port.type = MACH_MSG_PORT_DESCRIPTOR;
>@@ -342,9 +340,7 @@ bool Connection::sendOutgoingMessage(std::unique_ptr<Encoder> encoder)
>         }
> 
>         if (messageBodyIsOOL) {
>-            header->msgh_id = outOfLineBodyMessageID;
>-
>-            auto* descriptor = getDescriptorAndSkip(messageData);
>+            auto* descriptor = getDescriptorAndAdvance(messageData, sizeof(mach_msg_ool_descriptor_t));
>             descriptor->out_of_line.address = encoder->buffer();
>             descriptor->out_of_line.size = encoder->bufferSize();
>             descriptor->out_of_line.copy = MACH_MSG_VIRTUAL_COPY;
>diff --git a/Source/WebKit/Platform/IPC/mac/MachMessage.cpp b/Source/WebKit/Platform/IPC/mac/MachMessage.cpp
>index 64ea6b96de38282c2a79257469b36f2a8081002b..ec84c96a139af805f13f07ac7cbe024b0f53da23 100644
>--- a/Source/WebKit/Platform/IPC/mac/MachMessage.cpp
>+++ b/Source/WebKit/Platform/IPC/mac/MachMessage.cpp
>@@ -32,15 +32,16 @@
> 
> namespace IPC {
> 
>-std::unique_ptr<MachMessage> MachMessage::create(size_t size)
>+std::unique_ptr<MachMessage> MachMessage::create(CString&& messageReceiverName, CString&& messageName, size_t size)
> {
>-    void* memory = WTF::fastMalloc(sizeof(MachMessage) + size);
>-    return std::unique_ptr<MachMessage> { new (NotNull, memory) MachMessage { size } };
>+    void* memory = WTF::fastZeroedMalloc(sizeof(MachMessage) + size);
>+    return std::unique_ptr<MachMessage> { new (NotNull, memory) MachMessage { WTFMove(messageReceiverName), WTFMove(messageName), size } };
> }
> 
>-MachMessage::MachMessage(size_t size)
>-    : m_size { size }
>-    , m_shouldFreeDescriptors { true }
>+MachMessage::MachMessage(CString&& messageReceiverName, CString&& messageName, size_t size)
>+    : m_messageReceiverName(WTFMove(messageReceiverName))
>+    , m_messageName(WTFMove(messageName))
>+    , m_size { size }
> {
> }
> 
>@@ -56,21 +57,13 @@ size_t MachMessage::messageSize(size_t bodySize, size_t portDescriptorCount, siz
> 
>     if (portDescriptorCount || memoryDescriptorCount) {
>         messageSize += sizeof(mach_msg_body_t);
>-
>-        if (portDescriptorCount)
>-            messageSize += (portDescriptorCount * sizeof(mach_msg_port_descriptor_t));
>-        if (memoryDescriptorCount)
>-            messageSize += (memoryDescriptorCount * sizeof(mach_msg_ool_descriptor_t));
>+        messageSize += (portDescriptorCount * sizeof(mach_msg_port_descriptor_t));
>+        messageSize += (memoryDescriptorCount * sizeof(mach_msg_ool_descriptor_t));
>     }
> 
>     return round_msg(messageSize);
> }
> 
>-mach_msg_header_t* MachMessage::header()
>-{
>-    return m_messageHeader;
>-}
>-
> void MachMessage::leakDescriptors()
> {
>     m_shouldFreeDescriptors = false;
>diff --git a/Source/WebKit/Platform/IPC/mac/MachMessage.h b/Source/WebKit/Platform/IPC/mac/MachMessage.h
>index 9a4115eeb0f4f3834eb842c38dd39648e4311482..d0131c7f0d3efa38af4054e276d4fc286c93442c 100644
>--- a/Source/WebKit/Platform/IPC/mac/MachMessage.h
>+++ b/Source/WebKit/Platform/IPC/mac/MachMessage.h
>@@ -36,30 +36,27 @@ namespace IPC {
> class MachMessage {
>     WTF_MAKE_FAST_ALLOCATED;
> public:
>-    static std::unique_ptr<MachMessage> create(size_t);
>+    static std::unique_ptr<MachMessage> create(CString&& messageReceiverName, CString&& messageName, size_t);
>     ~MachMessage();
> 
>     static size_t messageSize(size_t bodySize, size_t portDescriptorCount, size_t memoryDescriptorCount);
> 
>     size_t size() const { return m_size; }
>-    mach_msg_header_t* header();
>+    mach_msg_header_t* header() { return m_messageHeader; }
> 
>     void leakDescriptors();
> 
>     const CString& messageReceiverName() const { return m_messageReceiverName; }
>-    void setMessageReceiverName(CString&& messageReceiverName) { m_messageReceiverName = WTFMove(messageReceiverName); }
>-
>     const CString& messageName() const { return m_messageName; }
>-    void setMessageName(CString&& messageName) { m_messageName = WTFMove(messageName); }
> 
> private:
>-    explicit MachMessage(size_t);
>+    MachMessage(CString&& messageReceiverName, CString&& messageName, size_t);
> 
>     CString m_messageReceiverName;
>     CString m_messageName;
>     size_t m_size;
>-    bool m_shouldFreeDescriptors;
>-    mach_msg_header_t m_messageHeader[0];
>+    bool m_shouldFreeDescriptors { true };
>+    mach_msg_header_t m_messageHeader[];
> };
> 
> }

Actions: View | Formatted Diff | Diff

Attachments on bug 197323: 368346 | 368353