Improve safety of MachMessage class
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 27 Apr 2019 17:09:24 +0000 (17:09 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 27 Apr 2019 17:09:24 +0000 (17:09 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197323
<rdar://problem/44291920>

Reviewed by Darin Adler.

Improve safety of MachMessage class and clean things up a bit.

* Platform/IPC/mac/ConnectionMac.mm:
(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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244721 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/Platform/IPC/mac/ConnectionMac.mm
Source/WebKit/Platform/IPC/mac/MachMessage.cpp
Source/WebKit/Platform/IPC/mac/MachMessage.h

index 02b2bab..3433e25 100644 (file)
@@ -1,3 +1,50 @@
+2019-04-27  Chris Dumez  <cdumez@apple.com>
+
+        Improve safety of MachMessage class
+        https://bugs.webkit.org/show_bug.cgi?id=197323
+        <rdar://problem/44291920>
+
+        Reviewed by Darin Adler.
+
+        Improve safety of MachMessage class and clean things up a bit.
+
+        * Platform/IPC/mac/ConnectionMac.mm:
+        (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-26  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         Rename m_LayerTreeFreezeReasons to m_layerTreeFreezeReasons
index d94193e..dc5f11c 100644 (file)
@@ -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;
index 64ea6b9..ec84c96 100644 (file)
 
 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;
index 9a4115e..d0131c7 100644 (file)
@@ -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[];
 };
 
 }