Attachment #373858 for bug #199680

View Details Raw Unified Return to Bug 199680



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 NOBODY (OOPS!).

        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  Youenn Fablet  <youenn@apple.com>

        Disable speculative loading if cache is not to be used for the load

Lines 260-266 Connection::Connection(Identifier identifier, bool isServer, Client& client) a/Source/WebKit/Platform/IPC/Connection.cpp_sec1
- a/Source/WebKit/Platform/IPC/Connection.cpp -2 / +8 lines
260	, m_inDispatchMessageCount(0)	260	, m_inDispatchMessageCount(0)
261	, m_inDispatchMessageMarkedDispatchWhenWaitingForSyncReplyCount(0)	261	, m_inDispatchMessageMarkedDispatchWhenWaitingForSyncReplyCount(0)
262	, m_didReceiveInvalidMessage(false)	262	, m_didReceiveInvalidMessage(false)
263	, m_waitingForMessage(nullptr)
264	, m_shouldWaitForSyncReplies(true)	263	, m_shouldWaitForSyncReplies(true)
265	{	264	{
266	ASSERT(RunLoop::isMain());	265	ASSERT(RunLoop::isMain());
Lines 468-473 Seconds Connection::timeoutRespectingIgnoreTimeoutsForTesting(Seconds timeout) c a/Source/WebKit/Platform/IPC/Connection.cpp_sec2
468	std::unique_ptr<Decoder> Connection::waitForMessage(StringReference messageReceiverName, StringReference messageName, uint64_t destinationID, Seconds timeout, OptionSet<WaitForOption> waitForOptions)	467	std::unique_ptr<Decoder> Connection::waitForMessage(StringReference messageReceiverName, StringReference messageName, uint64_t destinationID, Seconds timeout, OptionSet<WaitForOption> waitForOptions)
469	{	468	{
470	ASSERT(RunLoop::isMain());	469	ASSERT(RunLoop::isMain());
		470	auto protectedThis = makeRef(*this);
471		471
472	timeout = timeoutRespectingIgnoreTimeoutsForTesting(timeout);	472	timeout = timeoutRespectingIgnoreTimeoutsForTesting(timeout);
473		473
Lines 494-500 std::unique_ptr<Decoder> Connection::waitForMessage(StringReference messageRecei a/Source/WebKit/Platform/IPC/Connection.cpp_sec3
494		494
495	// Don't even start waiting if we have InterruptWaitingIfSyncMessageArrives and there's a sync message already in the queue.	495	// Don't even start waiting if we have InterruptWaitingIfSyncMessageArrives and there's a sync message already in the queue.
496	if (hasIncomingSynchronousMessage && waitForOptions.contains(WaitForOption::InterruptWaitingIfSyncMessageArrives)) {	496	if (hasIncomingSynchronousMessage && waitForOptions.contains(WaitForOption::InterruptWaitingIfSyncMessageArrives)) {
497	m_waitingForMessage = nullptr;	497	#if !ASSERT_DISABLED
		498	std::lock_guard<Lock> lock(m_waitForMessageMutex);
		499	// We don't support having multiple clients waiting for messages.
		500	ASSERT(!m_waitingForMessage);
		501	#endif
498	return nullptr;	502	return nullptr;
499	}	503	}
500		504
Lines 505-510 std::unique_ptr<Decoder> Connection::waitForMessage(StringReference messageRecei a/Source/WebKit/Platform/IPC/Connection.cpp_sec4
505		509
506	// We don't support having multiple clients waiting for messages.	510	// We don't support having multiple clients waiting for messages.
507	ASSERT(!m_waitingForMessage);	511	ASSERT(!m_waitingForMessage);
		512	if (m_waitingForMessage)
		513	return nullptr;
508		514
509	m_waitingForMessage = &waitingForMessage;	515	m_waitingForMessage = &waitingForMessage;
510	}	516	}

Lines 353-359 private: a/Source/WebKit/Platform/IPC/Connection.h_sec1
- a/Source/WebKit/Platform/IPC/Connection.h -1 / +1 lines
353	HashMap<uint64_t, ReplyHandler> m_replyHandlers;	353	HashMap<uint64_t, ReplyHandler> m_replyHandlers;
354		354
355	struct WaitForMessageState;	355	struct WaitForMessageState;
356	WaitForMessageState* m_waitingForMessage;	356	WaitForMessageState* m_waitingForMessage { nullptr };
357		357
358	class SyncMessageState;	358	class SyncMessageState;
359		359

Return to Bug 199680

Lines 1-3 a/Source/WebKit/ChangeLog_sec1
- a/Source/WebKit/ChangeLog +30 lines
		1	2019-07-10 Chris Dumez <cdumez@apple.com>
		2
		3	Crash under IPC::Connection::waitForMessage()
		4	https://bugs.webkit.org/show_bug.cgi?id=199680
		5	<rdar://problem/52500561>
		6
		7	Reviewed by NOBODY (OOPS!).
		8
		9	IPC::Connection::waitForMessage() is crashing due to a null defererence of
		10	m_waitingForMessage. Since m_waitingForMessage is only ever set to null in
		11	waitForMessage(), this seems to imply we've re-entered waitForMessage().
		12	This is in theory possible since the loop inside waitForMessage() calls
		13	SyncMessageState::singleton().dispatchMessages() on every iteration to
		14	process incoming synchronous IPC messages. In theory, one of these sync
		15	IPC messages could run code which ends up calling waitForAndDispatchImmediately()
		16	(and thus waitForMessage()).
		17
		18	We had a debug assertion to try and catch re-entrancy with a comment stating
		19	"We don't support having multiple clients waiting for messages." but we
		20	would not see those in release and we would crash with a null dereference
		21	instead.
		22
		23	To address the crashes in release, return early in case of re-entrancy
		24	(we would still hit an assertion in debug).
		25
		26	* Platform/IPC/Connection.cpp:
		27	(IPC::Connection::Connection):
		28	(IPC::Connection::waitForMessage):
		29	* Platform/IPC/Connection.h:
		30
1	2019-07-10 Youenn Fablet <youenn@apple.com>	31	2019-07-10 Youenn Fablet <youenn@apple.com>
2		32
3	Disable speculative loading if cache is not to be used for the load	33	Disable speculative loading if cache is not to be used for the load