Make IPC::Attachment moveable on DARWIN

https://bugs.webkit.org/show_bug.cgi?id=241660 This makes the DARWIN implementation of IPC::Attachment use MachSendPort, since this proper move handling, and lifetime management of the underlying mach_port_t. This also removes the MachPort class, since this was largely just used as an intermediate and is stricly worse than MachSendPort. The primary bug fixed here is a case where the WebProcess sent a port to the UIProcess, and then we failed to forward it to the GPUProcess since it had crashed. Previously we'd just leak the port (and thus never notify the WebContent process that we'd failed), whereas now it gets released correctly. Reviewed by Kimmo Kinnunen. * Source/WebKit/GPUProcess/GPUProcess.cpp: (WebKit::asConnectionIdentifier): (WebKit::GPUProcess::createGPUConnectionToWebProcess): * Source/WebKit/Platform/IPC/Attachment.cpp: (IPC::Attachment::Attachment): (IPC::Attachment::release): Deleted. * Source/WebKit/Platform/IPC/Attachment.h: (IPC::Attachment::Attachment::customWriter const): (IPC::Attachment::Attachment): Deleted. (IPC::Attachment::type const): Deleted. (IPC::Attachment::isNull const): Deleted. (IPC::Attachment::size const): Deleted. (IPC::Attachment::fd const): Deleted. (IPC::Attachment::release): Deleted. (IPC::Attachment::customWriter const): Deleted. (IPC::Attachment::port const): Deleted. (IPC::Attachment::disposition const): Deleted. (IPC::Attachment::handle const): Deleted. * Source/WebKit/Platform/IPC/StreamServerConnection.cpp: (IPC::StreamServerConnection::createWithDedicatedConnection): * Source/WebKit/Platform/IPC/cocoa/MachPort.h: Removed. * Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp: (WebKit::SharedMemory::IPCHandle::encode const): (WebKit::SharedMemory::IPCHandle::decode): * Source/WebKit/Scripts/webkit/parser_unittest.py: * Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp: (IPC::messageArgumentDescriptions): * Source/WebKit/Scripts/webkit/tests/TestWithLegacyReceiver.messages.in: * Source/WebKit/Scripts/webkit/tests/TestWithLegacyReceiverMessageReceiver.cpp: * Source/WebKit/Scripts/webkit/tests/TestWithLegacyReceiverMessages.h: (Messages::TestWithLegacyReceiver::DidCreateWebProcessConnection::DidCreateWebProcessConnection): * Source/WebKit/Scripts/webkit/tests/TestWithoutAttributes.messages.in: * Source/WebKit/Scripts/webkit/tests/TestWithoutAttributesMessageReceiver.cpp: * Source/WebKit/Scripts/webkit/tests/TestWithoutAttributesMessages.h: (Messages::TestWithoutAttributes::DidCreateWebProcessConnection::DidCreateWebProcessConnection): * Source/WebKit/Shared/IPCConnectionTester.cpp: (WebKit::asIdentifier): * Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm: (IPC::ArgumentCoder<MachSendRight>::encode): (IPC::ArgumentCoder<MachSendRight>::decode): * Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp: * Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp: (WebKit::NetworkProcessProxy::getNetworkProcessConnection): * Source/WebKit/WebKit.xcodeproj/project.pbxproj: * Source/WebKit/WebProcess/Inspector/WebInspector.cpp: (WebKit::WebInspector::setFrontendConnection): * Source/WebKit/WebProcess/Network/NetworkProcessConnectionInfo.h: (WebKit::NetworkProcessConnectionInfo::identifier const): (WebKit::NetworkProcessConnectionInfo::releaseIdentifier): Converts IPC::Attachment to use MachSendPort instead of a port/disposition pair, and changes all callsites to handle that. * Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm: (IPC::Connection::open): (IPC::Connection::sendOutgoingMessage): (IPC::createMessageDecoder): (IPC::Connection::receiveSourceEventHandler): (IPC::Connection::createConnectionIdentifierPair): Make serialization/deserialization handle IPC::Connection being a MachSendPort. Also makes sure we explicity allocate a send right at the two places we allocate a receive right, rather than relying on this happening during serialization (with the MAKE_SEND disposition). This means if the caller ends up not sending the IPC::Connection due to an error, we'll still end up deallocating a send right, and trigger the associated notifications. * Source/WebKit/Platform/IPC/cocoa/SharedFileHandleCocoa.cpp: (IPC::SharedFileHandle::encode const): (IPC::SharedFileHandle::decode): Fixes a potential leak where we previously just passed our port to fileport_makefd without then releasing our reference to it (which is now handled via the MachSendPort destructor). * Source/WTF/wtf/cocoa/MachSendRight.cpp: (WTF::MachSendRight::operator=): Fixes a potential leak, where we were just overwriting the old m_port contents without releasing it. Canonical link: https://commits.webkit.org/251671@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295666 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jun 20, 2022 · 16972b8 · 16972b8
1 parent a440dc2
commit 16972b8
Show file tree

Hide file tree

Showing 26 changed files with 261 additions and 141 deletions.
diff --git a/Source/WTF/wtf/cocoa/MachSendRight.cpp b/Source/WTF/wtf/cocoa/MachSendRight.cpp
@@ -72,8 +72,26 @@ void deallocateSendRightSafely(mach_port_t port)
         CRASH();
 }
 
+static void assertSendRight(mach_port_t port)
+{
+    if (port == MACH_PORT_NULL)
+        return;
+
+    unsigned count = 0;
+    auto kr = mach_port_get_refs(mach_task_self(), port, MACH_PORT_RIGHT_SEND, &count);
+    if (kr == KERN_SUCCESS && !count)
+        kr = mach_port_get_refs(mach_task_self(), port, MACH_PORT_RIGHT_DEAD_NAME, &count);
+
+    if (kr == KERN_SUCCESS && count > 0)
+        return;
+
+    RELEASE_LOG_ERROR(Process, "mach_port_get_refs error for port %d: %{private}s (%#x)", port, mach_error_string(kr), kr);
+    CRASH();
+}
+
 MachSendRight MachSendRight::adopt(mach_port_t port)
 {
+    assertSendRight(port);
     return MachSendRight(port);
 }
 
@@ -118,6 +136,7 @@ MachSendRight& MachSendRight::operator=(MachSendRight&& other)
 MachSendRight& MachSendRight::operator=(const MachSendRight& other)
 {
     if (this != &other) {
+        releaseSendRight(m_port);
         m_port = other.sendRight();
         retainSendRight(m_port);
     }

diff --git a/Source/WebKit/GPUProcess/GPUProcess.cpp b/Source/WebKit/GPUProcess/GPUProcess.cpp
@@ -121,7 +121,7 @@ static IPC::Connection::Identifier asConnectionIdentifier(IPC::Attachment&& conn
 #if USE(UNIX_DOMAIN_SOCKETS)
     return IPC::Connection::Identifier { connectionHandle.release().release() };
 #elif OS(DARWIN)
-    return IPC::Connection::Identifier { connectionHandle.port() };
+    return IPC::Connection::Identifier { connectionHandle.leakSendRight() };
 #elif OS(WINDOWS)
     return IPC::Connection::Identifier { connectionHandle.handle() };
 #else

diff --git a/Source/WebKit/Platform/IPC/Attachment.cpp b/Source/WebKit/Platform/IPC/Attachment.cpp
@@ -37,16 +37,16 @@ Attachment::Attachment()
 }
 
 #if OS(DARWIN) && !USE(UNIX_DOMAIN_SOCKETS)
-Attachment::Attachment(mach_port_name_t port, mach_msg_type_name_t disposition)
-    : m_type(MachPortType)
-    , m_port(port)
-    , m_disposition(disposition)
+Attachment::Attachment(MachSendRight&& right)
+    : MachSendRight(WTFMove(right))
+    , m_type(MachPortType)
 {
 }
 
-void Attachment::release()
+Attachment::Attachment(const MachSendRight& right)
+    : MachSendRight(right)
+    , m_type(MachPortType)
 {
-    m_type = Uninitialized;
 }
 #endif
 

diff --git a/Source/WebKit/Platform/IPC/Attachment.h b/Source/WebKit/Platform/IPC/Attachment.h
@@ -31,6 +31,7 @@
 #if OS(DARWIN) && !USE(UNIX_DOMAIN_SOCKETS)
 #include <mach/mach_init.h>
 #include <mach/mach_traps.h>
+#include <wtf/MachSendRight.h>
 #endif
 
 #if OS(WINDOWS)
@@ -48,7 +49,11 @@ namespace IPC {
 class Decoder;
 class Encoder;
 
+#if OS(DARWIN)
+class Attachment : public MachSendRight {
+#else
 class Attachment {
+#endif
 public:
     Attachment();
 
@@ -76,7 +81,8 @@ class Attachment {
     using CustomWriter = std::variant<CustomWriterFunc, SocketDescriptor>;
     Attachment(CustomWriter&&);
 #elif OS(DARWIN)
-    Attachment(mach_port_name_t, mach_msg_type_name_t disposition);
+    Attachment(MachSendRight&&);
+    Attachment(const MachSendRight&);
 #elif OS(WINDOWS)
     Attachment(HANDLE handle)
         : m_handle(handle)
@@ -93,12 +99,6 @@ class Attachment {
     UnixFileDescriptor release() { return std::exchange(m_fd, UnixFileDescriptor { }); }
 
     const CustomWriter& customWriter() const { return m_customWriter; }
-#elif OS(DARWIN)
-    void release();
-
-    // MachPortType
-    mach_port_name_t port() const { return m_port; }
-    mach_msg_type_name_t disposition() const { return m_disposition; }
 #elif OS(WINDOWS)
     HANDLE handle() const { return m_handle; }
 #endif
@@ -113,9 +113,6 @@ class Attachment {
     UnixFileDescriptor m_fd;
     size_t m_size;
     CustomWriter m_customWriter;
-#elif OS(DARWIN)
-    mach_port_name_t m_port { 0 };
-    mach_msg_type_name_t m_disposition { 0 };
 #elif OS(WINDOWS)
     HANDLE m_handle { INVALID_HANDLE_VALUE };
 #endif

diff --git a/Source/WebKit/Platform/IPC/StreamServerConnection.cpp b/Source/WebKit/Platform/IPC/StreamServerConnection.cpp
@@ -58,7 +58,7 @@ Ref<StreamServerConnection> StreamServerConnection::createWithDedicatedConnectio
 #if USE(UNIX_DOMAIN_SOCKETS)
     IPC::Connection::Identifier connectionHandle { connectionIdentifier.release().release() };
 #elif OS(DARWIN)
-    IPC::Connection::Identifier connectionHandle { connectionIdentifier.port() };
+    IPC::Connection::Identifier connectionHandle { connectionIdentifier.leakSendRight() };
 #elif OS(WINDOWS)
     IPC::Connection::Identifier connectionHandle { connectionIdentifier.handle() };
 #else

diff --git a/Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm b/Source/WebKit/Platform/IPC/cocoa/ConnectionCocoa.mm
@@ -31,7 +31,6 @@
 #import "ImportanceAssertion.h"
 #import "Logging.h"
 #import "MachMessage.h"
-#import "MachPort.h"
 #import "MachUtilities.h"
 #import "ReasonSPI.h"
 #import "WKCrashReporter.h"
@@ -217,7 +216,10 @@ void watchdogTimerFired()
 
         // Send the initialize message, which contains a send right for the server to use.
         auto encoder = makeUniqueRef<Encoder>(MessageName::InitializeConnection, 0);
-        encoder.get() << MachPort(m_receivePort, MACH_MSG_TYPE_MAKE_SEND);
+
+        mach_port_insert_right(mach_task_self(), m_receivePort, m_receivePort, MACH_MSG_TYPE_MAKE_SEND);
+        MachSendRight right = MachSendRight::adopt(m_receivePort);
+        encoder.get() << Attachment { WTFMove(right) };
 
         initializeSendSource();
 
@@ -338,8 +340,8 @@ void watchdogTimerFired()
             ASSERT(attachment.type() == Attachment::MachPortType);
             if (attachment.type() == Attachment::MachPortType) {
                 auto* descriptor = getDescriptorAndAdvance(messageData, sizeof(mach_msg_port_descriptor_t));
-                descriptor->port.name = attachment.port();
-                descriptor->port.disposition = attachment.disposition();
+                descriptor->port.name = attachment.leakSendRight();
+                descriptor->port.disposition = MACH_MSG_TYPE_MOVE_SEND;
                 descriptor->port.type = MACH_MSG_PORT_DESCRIPTOR;
             }
         }
@@ -459,8 +461,10 @@ void watchdogTimerFired()
         ASSERT(descriptor->type.type == MACH_MSG_PORT_DESCRIPTOR);
         if (descriptor->type.type != MACH_MSG_PORT_DESCRIPTOR)
             return nullptr;
+        ASSERT(descriptor->port.disposition == MACH_MSG_TYPE_PORT_SEND);
+        MachSendRight right = MachSendRight::adopt(descriptor->port.name);
 
-        attachments[numberOfAttachments - i - 1] = Attachment { descriptor->port.name, descriptor->port.disposition };
+        attachments[numberOfAttachments - i - 1] = Attachment { WTFMove(right) };
         descriptorData += sizeof(mach_msg_port_descriptor_t);
     }
 
@@ -569,13 +573,13 @@ void watchdogTimerFired()
             return;
         }
 
-        MachPort port;
-        if (!decoder->decode(port)) {
+        Attachment attachment;
+        if (!decoder->decode(attachment)) {
             // FIXME: Disconnect.
             return;
         }
 
-        m_sendPort = port.port();
+        m_sendPort = attachment.leakSendRight();
 
         if (m_sendPort) {
             ASSERT(MACH_PORT_VALID(m_receivePort));
@@ -676,6 +680,9 @@ void AccessibilityProcessSuspendedNotification(bool suspended)
         RELEASE_LOG_ERROR(Process, "Connection::createConnectionIdentifierPair: Could not allocate mach port, returned port was invalid");
         return std::nullopt;
     }
-    return ConnectionIdentifierPair { Connection::Identifier { listeningPort }, Attachment { listeningPort, MACH_MSG_TYPE_MAKE_SEND } };
+    mach_port_insert_right(mach_task_self(), listeningPort, listeningPort, MACH_MSG_TYPE_MAKE_SEND);
+    MachSendRight right = MachSendRight::adopt(listeningPort);
+
+    return ConnectionIdentifierPair { Connection::Identifier { listeningPort }, Attachment { WTFMove(right) } };
 }
 } // namespace IPC
diff --git a/Source/WebKit/Platform/IPC/cocoa/MachPort.h b/Source/WebKit/Platform/IPC/cocoa/MachPort.h
diff --git a/Source/WebKit/Platform/IPC/cocoa/SharedFileHandleCocoa.cpp b/Source/WebKit/Platform/IPC/cocoa/SharedFileHandleCocoa.cpp
@@ -24,9 +24,14 @@
  */
 
 #include "config.h"
+#include "ArgumentCoders.h"
+#include "Attachment.h"
+#include "Decoder.h"
+#include "Encoder.h"
 #include "SharedFileHandle.h"
+#include "WebCoreArgumentCoders.h"
+#include <wtf/MachSendRight.h>
 
-#include "MachPort.h"
 #include <pal/spi/cocoa/FilePortSPI.h>
 
 namespace IPC {
@@ -40,20 +45,19 @@ void SharedFileHandle::encode(Encoder& encoder) const
 {
     mach_port_name_t fileport = MACH_PORT_NULL;
     if (fileport_makeport(m_handle.handle(), &fileport) == -1) {
-        encoder << MachPort();
         return;
     }
 
-    encoder << MachPort(fileport, MACH_MSG_TYPE_MOVE_SEND);
+    encoder << MachSendRight::adopt(fileport);
 }
 
 std::optional<SharedFileHandle> SharedFileHandle::decode(Decoder& decoder)
 {
-    MachPort machPort;
-    if (!decoder.decode(machPort))
+    auto fileport = decoder.decode<MachSendRight>();
+    if (UNLIKELY(!decoder.isValid()))
         return std::nullopt;
 
-    int fd = fileport_makefd(machPort.port());
+    int fd = fileport_makefd(fileport->sendRight());
     if (fd == -1)
         return SharedFileHandle { };
 

diff --git a/Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp b/Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp
@@ -28,7 +28,6 @@
 
 #include "ArgumentCoders.h"
 #include "Logging.h"
-#include "MachPort.h"
 #include <WebCore/SharedBuffer.h>
 #include <mach/mach_error.h>
 #include <mach/mach_port.h>
@@ -128,7 +127,7 @@ void SharedMemory::IPCHandle::encode(IPC::Encoder& encoder) const
 {
     encoder << static_cast<uint64_t>(handle.m_size);
     encoder << dataSize;
-    encoder << IPC::MachPort(handle.m_port, MACH_MSG_TYPE_MOVE_SEND);
+    encoder << MachSendRight::adopt(handle.m_port);
     handle.m_port = MACH_PORT_NULL;
 }
 
@@ -151,12 +150,12 @@ bool SharedMemory::IPCHandle::decode(IPC::Decoder& decoder, IPCHandle& ipcHandle
     if (dataLength > bufferSize)
         return false;
 
-    IPC::MachPort machPort;
-    if (!decoder.decode(machPort))
+    auto sendRight = decoder.decode<MachSendRight>();
+    if (UNLIKELY(!decoder.isValid()))
         return false;
 
     handle.m_size = bufferSize;
-    handle.m_port = machPort.port();
+    handle.m_port = sendRight->leakSendRight();
     ipcHandle.handle = WTFMove(handle);
     ipcHandle.dataSize = dataLength;
     return true;

diff --git a/Source/WebKit/Scripts/webkit/parser_unittest.py b/Source/WebKit/Scripts/webkit/parser_unittest.py
@@ -191,7 +191,7 @@
         {
             'name': 'DidCreateWebProcessConnection',
             'parameters': (
-                ('IPC::MachPort', 'connectionIdentifier'),
+                ('MachSendRight', 'connectionIdentifier'),
                 ('OptionSet<WebKit::SelectionFlags>', 'flags'),
             ),
             'conditions': ('PLATFORM(MAC)'),

diff --git a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
@@ -417,7 +417,7 @@ std::optional<Vector<ArgumentDescription>> messageArgumentDescriptions(MessageNa
 #if PLATFORM(MAC)
     case MessageName::TestWithLegacyReceiver_DidCreateWebProcessConnection:
         return Vector<ArgumentDescription> {
-            { "connectionIdentifier", "IPC::MachPort", nullptr, false },
+            { "connectionIdentifier", "MachSendRight", nullptr, false },
             { "flags", "OptionSet<WebKit::SelectionFlags>", nullptr, false },
         };
     case MessageName::TestWithLegacyReceiver_InterpretKeyEvent:
@@ -526,7 +526,7 @@ std::optional<Vector<ArgumentDescription>> messageArgumentDescriptions(MessageNa
 #if PLATFORM(MAC)
     case MessageName::TestWithoutAttributes_DidCreateWebProcessConnection:
         return Vector<ArgumentDescription> {
-            { "connectionIdentifier", "IPC::MachPort", nullptr, false },
+            { "connectionIdentifier", "MachSendRight", nullptr, false },
             { "flags", "OptionSet<WebKit::SelectionFlags>", nullptr, false },
         };
     case MessageName::TestWithoutAttributes_InterpretKeyEvent:

diff --git a/Source/WebKit/Scripts/webkit/tests/TestWithLegacyReceiver.messages.in b/Source/WebKit/Scripts/webkit/tests/TestWithLegacyReceiver.messages.in
@@ -56,7 +56,7 @@ messages -> TestWithLegacyReceiver LegacyReceiver {
     SetVideoLayerID(WebCore::GraphicsLayer::PlatformLayerID videoLayerID)
 
 #if PLATFORM(MAC)
-    DidCreateWebProcessConnection(IPC::MachPort connectionIdentifier, OptionSet<WebKit::SelectionFlags> flags)
+    DidCreateWebProcessConnection(MachSendRight connectionIdentifier, OptionSet<WebKit::SelectionFlags> flags)
 #endif
 
 #if PLATFORM(MAC)