Bug 18

Client crashes after reconnecting because it reuses a remote thread from the old connection

Status

Fixed

Symptom

Crash; worst effect seen: crash

Where seen

Bitcoin Core unit tests

Trigger

A peer disconnects cleanly

First reported

2024-08-07 (first report)

Fixed

2024-08-09

Fix

mp#106

Shipped to Bitcoin Core

bitcoin#30509

In released binaries

No; the code was in source releases only

Defect ever on master

Yes

Summary

A client thread that made IPC calls over one connection, disconnected, and then connected again could segfault. Each client thread remembers which worker thread on the server runs its calls, per connection, in a map keyed by the raw Connection* pointer. libmultiprocess never removed the entry when the connection went away. If the new Connection was allocated at the old one's address, the next call went to a thread belonging to the dead connection. It was found by a new Bitcoin Core unit test that disconnects and reconnects, before Bitcoin Core accepted IPC connections from outside programs. Source builds of v22 through v28 with multiprocess enabled have the defect, but nothing in them reconnects this way outside that test. Fixed in mp#106.

What happened

@TheCharlatan suggested that bitcoin#30509, which added -ipcbind/-ipcconnect on unix sockets, test binding and connecting on several sockets in one process. The ipc_tests case added for that disconnects and immediately reconnects, and the next CI run failed (DrahtBot CI report, 2024-08-07). ryanofsky diagnosed it the next day from the failing Cirrus task: it did not reproduce natively, but did inside the CI container, most likely because that container was 32-bit and its heap reused the freed address.

Root cause

Each client thread's ThreadContext keeps two maps, request_threads and callback_threads, both std::map<Connection*, ProxyClient<Thread>>, so a thread that has already obtained a remote worker thread for a connection reuses it on later calls. Nothing removed an entry when its Connection was destroyed. When the allocator gave the next Connection the same address, the lookup in the Context overload of CustomBuildField (include/mp/proxy-types.h) found the stale entry and sent the call to a Thread capability of the previous connection, whose server thread no longer existed, and the client segfaulted.

This was safe as long as a Bitcoin Core process had one connection per peer for its whole life. bitcoin#30509's test was the first workload where one thread used two connections in sequence that could share an address.

The code

Quoted from 8ba0d03b4439^. Each thread's ThreadContext (include/mp/proxy-io.h) remembers, per connection, the remote thread it makes calls on, keyed by the raw Connection pointer:

std::map<Connection*, ProxyClient<Thread>> callback_threads;
// ...
std::map<Connection*, ProxyClient<Thread>> request_threads;

Before every call, the Context overload of CustomBuildField (include/mp/proxy-types.h) looks the connection up and reuses whatever entry it finds:

auto request_thread = request_threads.find(&connection);
if (request_thread == request_threads.end()) {
    // ...
    auto request = connection.m_thread_map.makeThreadRequest();
    request.setName(thread_context.thread_name);
    request_thread =
        request_threads
            .emplace(std::piecewise_construct, std::forward_as_tuple(&connection),
                std::forward_as_tuple(request.send().getResult(), &connection, /* destroy_connection= */ false))
            .first; // Nonblocking due to capnp request pipelining.
}

auto context = output.init();
context.setThread(request_thread->second.m_client);
context.setCallbackThread(callback_thread->second.m_client);

Nothing removes an entry when its Connection is destroyed. In a test that connects, disconnects and reconnects in a loop:

1. Client thread: makes a call on connection A; request_threads[&A] now holds a Thread capability for a worker in A's server.
2. Connection A is destroyed, and the server's worker thread for it goes away; the map entry stays.
3. Connection B is allocated at the same address as A.
4. Client thread: makes a call on B; find(&connection) returns A's stale entry, and the call is built with a Thread capability from the destroyed connection, which segfaults.

The fix (8ba0d03b4439, mp#106, +93/-42, so no diff here) moves the find-or-insert into a new SetThread() in src/mp/proxy.cpp, which registers a Connection::addSyncCleanup callback that erases the entry when the connection is destroyed, and gives ProxyClient<Thread> a destructor that unregisters the callback if the entry goes first.

An entry can no longer outlive its connection, so a new connection at a recycled address starts with no entry and gets a fresh makeThread call.

Fix

The fix commit 8ba0d03b4439, in mp#106 ("Bugfix: Clean up ThreadContext pointers when Connection is destroyed"), adds SetThread() (src/mp/proxy.cpp), which looks up or creates the map entry under the thread's Waiter::m_mutex. It registers a Connection::addSyncCleanup callback, stored in a new ProxyClient<Thread>::m_cleanup, that erases the entry if the connection is destroyed first. A new ~ProxyClient<Thread> unregisters the callback if the thread entry goes first. The server-side PassField Context overload uses SetThread too and erases its entry under the lock. No new libmultiprocess test was added; the Bitcoin Core test that found it covers it.

Timeline

• 2019-06-25: maps keyed by Connection* with no removal since the library's initial commit 06e1045baba0.
• 2024-08-07: CI failure of the new multiple-sockets test in bitcoin#30509; diagnosed 2024-08-08.
• 2024-08-09: fix merged in mp#106.
• 2024-09-09: shipped to Bitcoin Core in bitcoin#30509 itself, which updated the libmultiprocess depends package.
• Releases: v28.x and earlier have the defect in source builds with multiprocess enabled; v29.0 has the fix.

Notes

• The cleanup callbacks this fix added were involved in later bugs. Two months later, a server crashed erasing a map entry through an iterator that the new callback had already invalidated; that was fixed in mp#118 (its page). Later, a race between a worker thread and the event loop thread involving these callbacks, reported in mp#189 ("Races between ~ProxyClient<Thread> and ~Connection destructors"), was fixed in mp#201 (its page).

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.