Changeset 244721 in webkit


Ignore:
Timestamp:
Apr 27, 2019 10:09:24 AM (5 years ago)
Author:
Chris Dumez
Message:

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):

Location:
trunk/Source/WebKit
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r244717 r244721  
     12019-04-27  Chris Dumez  <cdumez@apple.com>
     2
     3        Improve safety of MachMessage class
     4        https://bugs.webkit.org/show_bug.cgi?id=197323
     5        <rdar://problem/44291920>
     6
     7        Reviewed by Darin Adler.
     8
     9        Improve safety of MachMessage class and clean things up a bit.
     10
     11        * Platform/IPC/mac/ConnectionMac.mm:
     12        (IPC::Connection::sendOutgoingMessage):
     13        - Pass MessageReceiverName / MessageName when constructing the MachMessage rather
     14          than setting them afterwards since they never change for a given MachMessage.
     15        - Set header->msgh_id to the right value right away instead of setting it first
     16          to inlineBodyMessageID and then later fixing it to be outOfLineBodyMessageID
     17          when the body is out of line.
     18        - When messageBodyIsOOL was true, we would call getDescriptorAndSkip() which
     19          would advance the pointer by sizeof(mach_msg_port_descriptor_t), even though
     20          the descriptor type is mach_msg_ool_descriptor_t. This would not matter in
     21          the end because we would not use the messageData pointer after this but
     22          still.
     23
     24        * Platform/IPC/mac/MachMessage.cpp:
     25        (IPC::MachMessage::create):
     26        Use fastZeroedMalloc() instead of fastMalloc() for safety, given that this class
     27        has a mach_msg_header_t flexible array member. This is what is recommended by the
     28        mach documentation. It is much safer because it otherwize relies on the user
     29        (Connection::sendOutgoingMessage()) to initialize ALL the message members
     30        correctly. I suspect this was the cause of <rdar://problem/44291920> because
     31        Connection::sendOutgoingMessage() would fail to initialize header->msgh_voucher_port
     32        and the MachMessage destructor would then call mach_msg_destroy(header()), which
     33        would mach_msg_destroy_port(header->msgh_voucher_port).
     34
     35        (IPC::MachMessage::MachMessage):
     36        Pass MessageReceiverName / MessageName when constructing the MachMessage rather
     37        than setting them afterwards since they never change for a given MachMessage.
     38
     39        (IPC::MachMessage::messageSize):
     40        Drop if checks for portDescriptorCount and memoryDescriptorCount since the logic
     41        will do the right thing even if they are 0.
     42
     43        * Platform/IPC/mac/MachMessage.h:
     44        (IPC::MachMessage::header):
     45        (IPC::MachMessage::messageReceiverName const):
     46        (IPC::MachMessage::messageName const):
     47
    1482019-04-26  Wenson Hsieh  <wenson_hsieh@apple.com>
    249

  • trunk/Source/WebKit/Platform/IPC/mac/ConnectionMac.mm

    r242610 r244721  
    307307    }
    308308
    309     auto message = MachMessage::create(messageSize);
    310     message->setMessageReceiverName(encoder->messageReceiverName().toString());
    311     message->setMessageName(encoder->messageName().toString());
     309    auto message = MachMessage::create(encoder->messageReceiverName().toString(), encoder->messageName().toString(), messageSize);
    312310
    313311    auto* header = message->header();
    … …  
    316314    header->msgh_remote_port = m_sendPort;
    317315    header->msgh_local_port = MACH_PORT_NULL;
    318     header->msgh_id = inlineBodyMessageID;
     316    header->msgh_id = messageBodyIsOOL ? outOfLineBodyMessageID : inlineBodyMessageID;
    319317
    320318    auto* messageData = reinterpret_cast<uint8_t*>(header + 1);
    … …  
    328326        messageData = reinterpret_cast<uint8_t*>(body + 1);
    329327
    330         auto getDescriptorAndSkip = [](uint8_t*& data) {
    331             return reinterpret_cast<mach_msg_descriptor_t*>(std::exchange(data, data + sizeof(mach_msg_port_descriptor_t)));
     328        auto getDescriptorAndAdvance = [](uint8_t*& data, std::size_t toAdvance) {
     329            return reinterpret_cast<mach_msg_descriptor_t*>(std::exchange(data, data + toAdvance));
    332330        };
    333331
    … …  
    335333            ASSERT(attachment.type() == Attachment::MachPortType);
    336334            if (attachment.type() == Attachment::MachPortType) {
    337                 auto* descriptor = getDescriptorAndSkip(messageData);
     335                auto* descriptor = getDescriptorAndAdvance(messageData, sizeof(mach_msg_port_descriptor_t));
    338336                descriptor->port.name = attachment.port();
    339337                descriptor->port.disposition = attachment.disposition();
    … …  
    343341
    344342        if (messageBodyIsOOL) {
    345             header->msgh_id = outOfLineBodyMessageID;
    346 
    347             auto* descriptor = getDescriptorAndSkip(messageData);
     343            auto* descriptor = getDescriptorAndAdvance(messageData, sizeof(mach_msg_ool_descriptor_t));
    348344            descriptor->out_of_line.address = encoder->buffer();
    349345            descriptor->out_of_line.size = encoder->bufferSize();

  • trunk/Source/WebKit/Platform/IPC/mac/MachMessage.cpp

    r224779 r244721  
    3333namespace IPC {
    3434
    35 std::unique_ptr<MachMessage> MachMessage::create(size_t size)
     35std::unique_ptr<MachMessage> MachMessage::create(CString&& messageReceiverName, CString&& messageName, size_t size)
    3636{
    37     void* memory = WTF::fastMalloc(sizeof(MachMessage) + size);
    38     return std::unique_ptr<MachMessage> { new (NotNull, memory) MachMessage { size } };
     37    void* memory = WTF::fastZeroedMalloc(sizeof(MachMessage) + size);
     38    return std::unique_ptr<MachMessage> { new (NotNull, memory) MachMessage { WTFMove(messageReceiverName), WTFMove(messageName), size } };
    3939}
    4040
    41 MachMessage::MachMessage(size_t size)
    42     : m_size { size }
    43     , m_shouldFreeDescriptors { true }
     41MachMessage::MachMessage(CString&& messageReceiverName, CString&& messageName, size_t size)
     42    : m_messageReceiverName(WTFMove(messageReceiverName))
     43    , m_messageName(WTFMove(messageName))
     44    , m_size { size }
    4445{
    4546}
    … …  
    5758    if (portDescriptorCount || memoryDescriptorCount) {
    5859        messageSize += sizeof(mach_msg_body_t);
    59 
    60         if (portDescriptorCount)
    61             messageSize += (portDescriptorCount * sizeof(mach_msg_port_descriptor_t));
    62         if (memoryDescriptorCount)
    63             messageSize += (memoryDescriptorCount * sizeof(mach_msg_ool_descriptor_t));
     60        messageSize += (portDescriptorCount * sizeof(mach_msg_port_descriptor_t));
     61        messageSize += (memoryDescriptorCount * sizeof(mach_msg_ool_descriptor_t));
    6462    }
    6563
    6664    return round_msg(messageSize);
    67 }
    68 
    69 mach_msg_header_t* MachMessage::header()
    70 {
    71     return m_messageHeader;
    7265}
    7366

  • trunk/Source/WebKit/Platform/IPC/mac/MachMessage.h

    r224782 r244721  
    3737    WTF_MAKE_FAST_ALLOCATED;
    3838public:
    39     static std::unique_ptr<MachMessage> create(size_t);
     39    static std::unique_ptr<MachMessage> create(CString&& messageReceiverName, CString&& messageName, size_t);
    4040    ~MachMessage();
    4141
    … …  
    4343
    4444    size_t size() const { return m_size; }
    45     mach_msg_header_t* header();
     45    mach_msg_header_t* header() { return m_messageHeader; }
    4646
    4747    void leakDescriptors();
    4848
    4949    const CString& messageReceiverName() const { return m_messageReceiverName; }
    50     void setMessageReceiverName(CString&& messageReceiverName) { m_messageReceiverName = WTFMove(messageReceiverName); }
    51 
    5250    const CString& messageName() const { return m_messageName; }
    53     void setMessageName(CString&& messageName) { m_messageName = WTFMove(messageName); }
    5451
    5552private:
    56     explicit MachMessage(size_t);
     53    MachMessage(CString&& messageReceiverName, CString&& messageName, size_t);
    5754
    5855    CString m_messageReceiverName;
    5956    CString m_messageName;
    6057    size_t m_size;
    61     bool m_shouldFreeDescriptors;
    62     mach_msg_header_t m_messageHeader[0];
     58    bool m_shouldFreeDescriptors { true };
     59    mach_msg_header_t m_messageHeader[];
    6360};
    6461
Note: See TracChangeset for help on using the changeset viewer.