Changeset 247322 in webkit

Timestamp:

Jul 10, 2019 1:10:49 PM (5 years ago)

Author:

Chris Dumez

Message:

Crash under IPC::Connection::waitForMessage()
​https://bugs.webkit.org/show_bug.cgi?id=199680
<rdar://problem/52500561>

Reviewed by Tim Horton.

IPC::Connection::waitForMessage() is crashing due to a null defererence of
m_waitingForMessage. Since m_waitingForMessage is only ever set to null in
waitForMessage(), this seems to imply we've re-entered waitForMessage().
This is in theory possible since the loop inside waitForMessage() calls
SyncMessageState::singleton().dispatchMessages() on every iteration to
process incoming synchronous IPC messages. In theory, one of these sync
IPC messages could run code which ends up calling waitForAndDispatchImmediately()
(and thus waitForMessage()).

We had a debug assertion to try and catch re-entrancy with a comment stating
"We don't support having multiple clients waiting for messages." but we
would not see those in release and we would crash with a null dereference
instead.

To address the crashes in release, return early in case of re-entrancy
(we would still hit an assertion in debug).

• Platform/IPC/Connection.cpp:

(IPC::Connection::Connection):
(IPC::Connection::waitForMessage):

• Platform/IPC/Connection.h:

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• Platform/IPC/Connection.cpp (modified) (3 diffs)
• Platform/IPC/Connection.h (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-07-10  Chris Dumez  <cdumez@apple.com>
+
+        Crash under IPC::Connection::waitForMessage()
+        https://bugs.webkit.org/show_bug.cgi?id=199680
+        <rdar://problem/52500561>
+
+        Reviewed by Tim Horton.
+
+        IPC::Connection::waitForMessage() is crashing due to a null defererence of
+        m_waitingForMessage. Since m_waitingForMessage is only ever set to null in
+        waitForMessage(), this seems to imply we've re-entered waitForMessage().
+        This is in theory possible since the loop inside waitForMessage() calls
+        SyncMessageState::singleton().dispatchMessages() on every iteration to
+        process incoming synchronous IPC messages. In theory, one of these sync
+        IPC messages could run code which ends up calling waitForAndDispatchImmediately()
+        (and thus waitForMessage()).
+
+        We had a debug assertion to try and catch re-entrancy with a comment stating
+        "We don't support having multiple clients waiting for messages." but we
+        would not see those in release and we would crash with a null dereference
+        instead.
+
+        To address the crashes in release, return early in case of re-entrancy
+        (we would still hit an assertion in debug).
+
+        * Platform/IPC/Connection.cpp:
+        (IPC::Connection::Connection):
+        (IPC::Connection::waitForMessage):
+        * Platform/IPC/Connection.h:
+
 2019-07-10  Tim Horton  <timothy_horton@apple.com>
 

trunk/Source/WebKit/Platform/IPC/Connection.cpp

@@ -261 +261 @@
     , m_inDispatchMessageMarkedDispatchWhenWaitingForSyncReplyCount(0)
     , m_didReceiveInvalidMessage(false)
-    , m_waitingForMessage(nullptr)
     , m_shouldWaitForSyncReplies(true)
 {
@@ -469 +468 @@
 {
     ASSERT(RunLoop::isMain());
+    auto protectedThis = makeRef(*this);
 
     timeout = timeoutRespectingIgnoreTimeoutsForTesting(timeout);
@@ -495 +495 @@
     // Don't even start waiting if we have InterruptWaitingIfSyncMessageArrives and there's a sync message already in the queue.
     if (hasIncomingSynchronousMessage && waitForOptions.contains(WaitForOption::InterruptWaitingIfSyncMessageArrives)) {
-        m_waitingForMessage = nullptr;
-        return nullptr;
-    }
-
-    WaitForMessageState waitingForMessage(messageReceiverName, messageName, destinationID, waitForOptions);
-
-    {
+#if !ASSERT_DISABLED
         std::lock_guard<Lock> lock(m_waitForMessageMutex);
-
         // We don't support having multiple clients waiting for messages.
         ASSERT(!m_waitingForMessage);
+#endif
+        return nullptr;
+    }
+
+    WaitForMessageState waitingForMessage(messageReceiverName, messageName, destinationID, waitForOptions);
+
+    {
+        std::lock_guard<Lock> lock(m_waitForMessageMutex);
+
+        // We don't support having multiple clients waiting for messages.
+        ASSERT(!m_waitingForMessage);
+        if (m_waitingForMessage)
+            return nullptr;
 
         m_waitingForMessage = &waitingForMessage;

trunk/Source/WebKit/Platform/IPC/Connection.h

@@ -354 +354 @@
 
     struct WaitForMessageState;
-    WaitForMessageState* m_waitingForMessage;
+    WaitForMessageState* m_waitingForMessage { nullptr };
 
     class SyncMessageState;