From c551a8094a0a771bc245fca0d9881d6270215532 Mon Sep 17 00:00:00 2001 From: sideshowbarker Date: Sun, 24 May 2026 09:54:23 +0900 Subject: [PATCH] =?UTF-8?q?LibIPC:=20Don=E2=80=99t=20dispatch=20queued=20m?= =?UTF-8?q?essages=20when=20sync=20IPC=20peer=20disconnects?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: When the browser is closed during startup, we crash inside PageHost::attach_compositor_ui_client() — on what looks like a normal null check, but which is actually reading uninitialized memory. Cause: A sync allocate_compositor_context_id IPC call is issued by the PageHost constructor before ConnectionFromClient::m_page_host has been assigned. The sync call runs inside the initializer that’s still computing m_page_host’s value. If a peer disconnects before responding (e.g., during shutdown), wait_for_specific_endpoint_message_impl’s failure path drains any still-queued messages via handle_messages(). One of those is the initial ConnectToCompositor, which dispatches to m_page_host->attach_compositor_ui_client() — but m_page_host is uninitialized garbage at that point, so the call segfaults. Fix: On peer EOF, stop draining queued messages from the failure path. Re-entering arbitrary handlers from any sync IPC wait is unsafe — since that wait can be reached from a constructor whose members are still being initialized. Instead, call shutdown() to close the transport and invoke die(). That exits processes cleanly via _exit(0) — achieving the same effect the queued close_server message would’ve had. Fixes https://github.com/LadybirdBrowser/ladybird/issues/9582 --- Libraries/LibIPC/Connection.cpp | 19 +++-- Tests/LibIPC/CMakeLists.txt | 1 + Tests/LibIPC/TestConnection.cpp | 122 ++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 6 deletions(-) create mode 100644 Tests/LibIPC/TestConnection.cpp diff --git a/Libraries/LibIPC/Connection.cpp b/Libraries/LibIPC/Connection.cpp index 66eeb45608..b2bbad4e56 100644 --- a/Libraries/LibIPC/Connection.cpp +++ b/Libraries/LibIPC/Connection.cpp @@ -128,6 +128,8 @@ ConnectionBase::PeerEOF ConnectionBase::drain_messages_from_peer() OwnPtr ConnectionBase::wait_for_specific_endpoint_message_impl(u32 endpoint_magic, int message_id) { + bool peer_disconnected_during_wait = false; + for (;;) { // Double check we don't already have the event waiting for us. // Otherwise we might end up blocked for a while for no reason. @@ -143,24 +145,29 @@ OwnPtr ConnectionBase::wait_for_specific_endpoint_message_impl(u32 break; wait_for_transport_to_become_readable(); - if (drain_messages_from_peer() == PeerEOF::Yes) + if (drain_messages_from_peer() == PeerEOF::Yes) { + peer_disconnected_during_wait = true; break; + } } dbgln("Failed to receive message_id: {}", message_id); if (!m_unprocessed_messages.is_empty()) { - m_transport->close(); - dbgln("Transport shutdown with unprocessed messages left: {}", m_unprocessed_messages.size()); for (size_t i = 0; i < m_unprocessed_messages.size(); ++i) { auto& message = m_unprocessed_messages[i]; dbgln(" Message {:03} is: {:2}-{}", i, message->message_id(), message->message_name()); } + } - dbgln("Handling remaining messages before returning to caller"); - handle_messages(); - dbgln("Messages handled, returning to caller"); + if (peer_disconnected_during_wait) { + // Don't dispatch any remaining queued messages here. wait_for_specific_endpoint_message_impl can be entered + // from any sync IPC call — including from inside a constructor whose members are still being initialized. (See + // issue #9582. PageHost's constructor issues a sync IPC before ConnectionFromClient::m_page_host has been + // assigned.) Re-entering arbitrary handlers from here can hit uninitialized state and crash. shutdown() closes + // the transport and calls die(). That exits processes cleanly — the same as queued close_server message would. + shutdown(); } return {}; diff --git a/Tests/LibIPC/CMakeLists.txt b/Tests/LibIPC/CMakeLists.txt index 7dfdac668a..055caced70 100644 --- a/Tests/LibIPC/CMakeLists.txt +++ b/Tests/LibIPC/CMakeLists.txt @@ -1,3 +1,4 @@ if (UNIX AND NOT APPLE) ladybird_test("TestTransportSocket.cpp" LibIPC LIBS LibIPC) + ladybird_test("TestConnection.cpp" LibIPC LIBS LibIPC) endif() diff --git a/Tests/LibIPC/TestConnection.cpp b/Tests/LibIPC/TestConnection.cpp new file mode 100644 index 0000000000..c7d5ec8a34 --- /dev/null +++ b/Tests/LibIPC/TestConnection.cpp @@ -0,0 +1,122 @@ +/* + * Copyright (c) 2026, The Ladybird developers + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +constexpr u32 TEST_MAGIC = 0xCAFEF00D; +constexpr int TARGET_MESSAGE_ID = 1; +constexpr int OTHER_MESSAGE_ID = 2; + +class TestMessage final : public IPC::Message { +public: + explicit TestMessage(int id) + : m_id(id) + { + } + + u32 endpoint_magic() const override { return TEST_MAGIC; } + int message_id() const override { return m_id; } + StringView message_name() const override { return "TestMessage"sv; } + + ErrorOr encode() const override + { + return IPC::MessageBuffer {}; + } + +private: + int m_id { 0 }; +}; + +class CountingStub final : public IPC::Stub { +public: + u32 magic() const override { return TEST_MAGIC; } + ByteString name() const override { return "CountingStub"; } + + ErrorOr> handle(NonnullOwnPtr) override + { + ++m_handle_count; + return OwnPtr {}; + } + + size_t handle_count() const { return m_handle_count; } + +private: + size_t m_handle_count { 0 }; +}; + +class TestConnection final : public IPC::ConnectionBase { + C_OBJECT(TestConnection); + +public: + void inject_unprocessed_message(NonnullOwnPtr message) + { + m_unprocessed_messages.append(move(message)); + } + + OwnPtr call_wait_for_specific_endpoint_message_impl(u32 endpoint_magic, int message_id) + { + return wait_for_specific_endpoint_message_impl(endpoint_magic, message_id); + } + +protected: + OwnPtr try_parse_message(ReadonlyBytes, Queue&) override + { + return nullptr; + } + +private: + TestConnection(IPC::Stub& stub, NonnullOwnPtr transport) + : IPC::ConnectionBase(stub, move(transport), TEST_MAGIC) + { + } +}; + +} + +// Regression test for #9582. wait_for_specific_endpoint_message_impl is reachable from any sync IPC call, including +// from inside a constructor whose members are still being initialized (e.g., PageHost's construction issues a sync +// allocate_compositor_context_id IPC call before ConnectionFromClient::m_page_host has been assigned). If the peer +// disconnects before responding, the failure path must not synchronously dispatch unrelated queued messages. That can +// reenter arbitrary handlers from a context where caller state is only partially built. +TEST_CASE(sync_wait_failure_does_not_dispatch_queued_messages) +{ + Core::EventLoop loop; + + int fds[2] = {}; + MUST(Core::System::socketpair(AF_LOCAL, SOCK_STREAM, 0, fds)); + + auto local_socket = TRY_OR_FAIL(Core::LocalSocket::adopt_fd(fds[0])); + auto peer_socket = TRY_OR_FAIL(Core::LocalSocket::adopt_fd(fds[1])); + + MUST(local_socket->set_blocking(false)); + MUST(peer_socket->set_blocking(false)); + + auto transport = make(move(local_socket)); + + CountingStub stub; + auto connection = TestConnection::construct(stub, move(transport)); + + connection->inject_unprocessed_message(make(OTHER_MESSAGE_ID)); + + peer_socket->close(); + + auto response = connection->call_wait_for_specific_endpoint_message_impl(TEST_MAGIC, TARGET_MESSAGE_ID); + + EXPECT(!response); + EXPECT_EQ(stub.handle_count(), 0u); +}