Title: [244721] trunk/Source/WebKit
- Revision
- 244721
- Author
- [email protected]
- Date
- 2019-04-27 10:09:24 -0700 (Sat, 27 Apr 2019)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (244720 => 244721)
--- trunk/Source/WebKit/ChangeLog 2019-04-27 16:17:09 UTC (rev 244720)
+++ trunk/Source/WebKit/ChangeLog 2019-04-27 17:09:24 UTC (rev 244721)
@@ -1,3 +1,50 @@
+2019-04-27 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Rename m_LayerTreeFreezeReasons to m_layerTreeFreezeReasons
Modified: trunk/Source/WebKit/Platform/IPC/mac/ConnectionMac.mm (244720 => 244721)
--- trunk/Source/WebKit/Platform/IPC/mac/ConnectionMac.mm 2019-04-27 16:17:09 UTC (rev 244720)
+++ trunk/Source/WebKit/Platform/IPC/mac/ConnectionMac.mm 2019-04-27 17:09:24 UTC (rev 244721)
@@ -306,9 +306,7 @@
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);
@@ -315,7 +313,7 @@
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 @@
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 @@
}
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;
Modified: trunk/Source/WebKit/Platform/IPC/mac/MachMessage.cpp (244720 => 244721)
--- trunk/Source/WebKit/Platform/IPC/mac/MachMessage.cpp 2019-04-27 16:17:09 UTC (rev 244720)
+++ trunk/Source/WebKit/Platform/IPC/mac/MachMessage.cpp 2019-04-27 17:09:24 UTC (rev 244721)
@@ -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 @@
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;
Modified: trunk/Source/WebKit/Platform/IPC/mac/MachMessage.h (244720 => 244721)
--- trunk/Source/WebKit/Platform/IPC/mac/MachMessage.h 2019-04-27 16:17:09 UTC (rev 244720)
+++ trunk/Source/WebKit/Platform/IPC/mac/MachMessage.h 2019-04-27 17:09:24 UTC (rev 244721)
@@ -36,30 +36,27 @@
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[];
};
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
