Bug 20

Server crashes when a client disconnects during a long-running call

Status

Fixed

Symptom

Crash; worst effect seen: crash

Where seen

Branch-build

Trigger

A client disconnects abruptly, for example because it is killed during a call

First reported

2024-10-08 (first report)

Fixed

2024-10-16

Fix

mp#118

Shipped to Bitcoin Core

bitcoin#31105

In released binaries

No; the code was in source releases only

Defect ever on master

Yes

Summary

If an IPC client disconnected while a server method it had called was still running, the server crashed when that method returned. While a call runs, the server's worker thread keeps an entry in its request_threads map that records which client thread to send callbacks to. The worker thread erased that entry after the call through an iterator saved before the call, but the disconnect had already removed the entry on the event loop thread. @tdb3 hit it every time by pressing Ctrl-C on a mining client blocked in a long wait and then mining a block. Source builds of v22 through v28 with multiprocess enabled have the bug, which became reachable on master once the mining interface had methods that wait a long time. Fixed in mp#118 by erasing the entry by key instead.

What happened

Reviewing bitcoin#31003 ("Add waitFeesChanged() to Mining interface"), @tdb3 ran the bitcoin-mine sample client from bitcoin#30437 ("ipc: add bitcoin-mine test program") against bitcoin-node, pressed Ctrl-C while the client was blocked in waitFeesChanged, then generated a block (2024-10-08). bitcoin-node segfaulted every time, on the worker thread, inside std::_Rb_tree_rebalance_for_erase called from request_threads.erase in the PassField Context overload; the review includes the gdb backtrace. ryanofsky reproduced it from the steps and opened the fix PR, mp#118 ("shutdown bugfix: Prevent segfault in server if connection is broken during long function call"), the same day.

Root cause

The PassField overload for Context arguments (then in include/mp/proxy-types.h, now type-context.h) runs the server method on a worker thread. Before the call it inserts the caller's callbackThread into the worker's request_threads map and keeps the returned iterator, and a KJ_DEFER erases by that iterator after the method returns.

When the client disconnected mid-call, the event-loop thread destroyed the Connection. Its sync cleanup callback erased that same map entry. That callback was registered by SetThread, which mp#106 added two months earlier so that map entries do not outlive their connection (its page). The fix's code comment also names request_threads.clear() in ~ProxyServer<Thread>, but that destructor asserts the thread is idle, so whether it can run during a call is unconfirmed; it is at most a possible second path. The worker's saved iterator then pointed at a freed tree node, and the deferred erase(iterator) corrupted the map and crashed.

The erase-by-iterator dates from the library's initial commit; mp#106 added the cleanup callback that made the entry disappear on a mid-call disconnect. The crash also needed a server method that blocks for a long time while the client goes away, which the mining interface's wait methods were the first to provide.

The code

Quoted from 9d11042772a2^. On the worker thread, the Context overload of PassField (include/mp/proxy-types.h) adds the caller's thread to request_threads, keeps the iterator, and erases by that iterator after the method returns:

auto [request_thread, inserted]{SetThread(
    request_threads, thread_context.waiter->m_mutex,
    server.m_context.connection,
    [&] { return context_arg.getCallbackThread(); })};
// ...
auto erase_thread{inserted ? request_thread : request_threads.end()};
KJ_DEFER(if (erase_thread != request_threads.end()) {
    std::unique_lock<std::mutex> lock(thread_context.waiter->m_mutex);
    request_threads.erase(erase_thread);
});
fn.invoke(server_context, args...);

SetThread (src/mp/proxy.cpp) also registers a callback that the Connection destructor runs on the event-loop thread, erasing the same entry:

thread->second.setCleanup([&threads, &mutex, thread] {
    // ...
    thread->second.m_cleanup.reset();
    // Remove connection pointer about to be destroyed from the map
    std::unique_lock<std::mutex> lock(mutex);
    threads.erase(thread);
});

When the client disconnects during a long call such as waitFeesChanged:

1. Worker: SetThread inserts the entry, and fn.invoke blocks waiting for fees to change.
2. Event loop: the client goes away, ~Connection runs the cleanup callback, and the entry is erased and its tree node freed.
3. Worker: the method returns, and the KJ_DEFER calls erase(erase_thread) with the iterator to the freed node, corrupting the map; bitcoin-node segfaults in _Rb_tree_rebalance_for_erase.

The fix erases by key instead of by iterator (9d11042772a2, the new comments are trimmed):

@@ -152,10 +152,19 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
-                    auto erase_thread{inserted ? request_thread : request_threads.end()};
-                    KJ_DEFER(if (erase_thread != request_threads.end()) {
+                    const bool erase_thread{inserted};
+                    KJ_DEFER(if (erase_thread) {
                         std::unique_lock<std::mutex> lock(thread_context.waiter->m_mutex);
-                        request_threads.erase(erase_thread);
+                        request_threads.erase(server.m_context.connection);
                     });

Erasing by key under the same mutex does nothing if the disconnect callback already removed the entry, so the worker never touches a freed node.

Fix

The fix commit 9d11042772a2 erases by key, request_threads.erase(server.m_context.connection), under the waiter mutex, which does nothing if the entry is already gone, with a comment naming both invalidation paths. It also adds a comment in SetThread explaining why the iterator its own cleanup callback captures stays valid: ~ProxyClient<Thread> unregisters the callback if the entry is removed first (+17/-3).

With this fix applied, the same reproduction exposed a second crash on the same path: a ProxyServer destroyed after its Connection. mp#118 worked around it with a second commit, and mp#160 ("refactor: EventLoop locking cleanups + client disconnect exception") fixed it. It has its own page.

Timeline

• 2019-06-25: erase by saved iterator present since the library's initial commit 06e1045baba0.
• 2024-08-09: mp#106 merged, adding the connection cleanup callback that erases the entry.
• 2024-10-08: reported by @tdb3 in a review of bitcoin#31003; mp#118 opened.
• 2024-10-16: fix merged in mp#118.
• 2024-10-21: shipped to Bitcoin Core in bitcoin#31105 ("Update libmultiprocess library").
• Releases: v28.x and earlier have the bug in source builds with multiprocess enabled; v29.0 has the fix.

Classification

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