LibIPC: Don’t dispatch queued messages when sync IPC peer disconnects
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
This commit is contained in:
parent
762aea0568
commit
c551a8094a
3 changed files with 136 additions and 6 deletions
|
|
@ -128,6 +128,8 @@ ConnectionBase::PeerEOF ConnectionBase::drain_messages_from_peer()
|
|||
|
||||
OwnPtr<IPC::Message> 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<IPC::Message> 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 {};
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
if (UNIX AND NOT APPLE)
|
||||
ladybird_test("TestTransportSocket.cpp" LibIPC LIBS LibIPC)
|
||||
ladybird_test("TestConnection.cpp" LibIPC LIBS LibIPC)
|
||||
endif()
|
||||
|
|
|
|||
122
Tests/LibIPC/TestConnection.cpp
Normal file
122
Tests/LibIPC/TestConnection.cpp
Normal file
|
|
@ -0,0 +1,122 @@
|
|||
/*
|
||||
* Copyright (c) 2026, The Ladybird developers
|
||||
*
|
||||
* SPDX-License-Identifier: BSD-2-Clause
|
||||
*/
|
||||
|
||||
#include <AK/ByteString.h>
|
||||
#include <AK/Queue.h>
|
||||
#include <AK/RefPtr.h>
|
||||
#include <LibCore/EventLoop.h>
|
||||
#include <LibCore/Socket.h>
|
||||
#include <LibCore/System.h>
|
||||
#include <LibIPC/Connection.h>
|
||||
#include <LibIPC/Message.h>
|
||||
#include <LibIPC/Stub.h>
|
||||
#include <LibIPC/TransportSocket.h>
|
||||
#include <LibTest/TestCase.h>
|
||||
|
||||
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<IPC::MessageBuffer> 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<OwnPtr<IPC::MessageBuffer>> handle(NonnullOwnPtr<IPC::Message>) override
|
||||
{
|
||||
++m_handle_count;
|
||||
return OwnPtr<IPC::MessageBuffer> {};
|
||||
}
|
||||
|
||||
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<IPC::Message> message)
|
||||
{
|
||||
m_unprocessed_messages.append(move(message));
|
||||
}
|
||||
|
||||
OwnPtr<IPC::Message> 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<IPC::Message> try_parse_message(ReadonlyBytes, Queue<IPC::Attachment>&) override
|
||||
{
|
||||
return nullptr;
|
||||
}
|
||||
|
||||
private:
|
||||
TestConnection(IPC::Stub& stub, NonnullOwnPtr<IPC::Transport> 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<IPC::TransportSocket>(move(local_socket));
|
||||
|
||||
CountingStub stub;
|
||||
auto connection = TestConnection::construct(stub, move(transport));
|
||||
|
||||
connection->inject_unprocessed_message(make<TestMessage>(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);
|
||||
}
|
||||
Loading…
Reference in a new issue