IPC stability bugs

Bug 34

Unlocked cleanup state races when a client disconnects as a call returns

Status
Fixed
Symptom
Data race; worst effect seen: sanitizer report only
Where seen
Libmultiprocess tests (mptest)
Trigger
A client disconnects abruptly, for example because it is killed during a call
First reported
2025-06-27 (first report)
Fixed
2025-07-01
Fix
mp#186
Shipped to Bitcoin Core
bitcoin#32345
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

When a client disconnected while a server worker thread was finishing an IPC call, the event loop thread, destroying the Connection, read and wrote state shared with the worker without holding the locks the worker used. ThreadSanitizer reported three data races in the mptest test "Calling IPC method, disconnecting and blocking during the call". The unlocked list walk and pointer accesses could in principle crash, but no crash was attributed to them. Fixed in mp#186, a PR that fixed several mptest failures seen in Bitcoin Core CI, by taking the missing locks. The code was in Bitcoin Core source releases v22 through v29, which did not ship IPC binaries.

What happened

Root cause

A server worker thread keeps a ProxyClient<Thread> for each connection in its request_threads map, so that callbacks to the client run on the right client thread. The worker destroys that entry as the IPC call returns, while the event loop thread destroys the server Connection for the client's disconnect. Three pieces of state shared between them had no consistent lock:

The commit message explains that the third report is not a problem by itself, since the destructor skips removeSyncCleanup when m_disconnect_cb is null, but TSan reports it correctly because the flag it depends on was accessed without synchronization.

The first two unlocked accesses date from the initial commit. The third dates from 8ba0d03b4439, part of mp#106 ("Clean up ThreadContext pointers when Connection is destroyed", 2024-08-09), which added the SetThread callback. None mattered until mp#160 (merged 2025-06-19) added tests that break connections while a server call is running.

The code

Quoted from fe4a188803c6^. On the worker thread, ProxyClient<Thread>::~ProxyClient (src/mp/proxy.cpp) runs while PassField holds Waiter::m_mutex to erase its request_threads entry. It unregisters the disconnect callback if it is still set:

if (m_disconnect_cb) {
    m_context.connection->removeSyncCleanup(*m_disconnect_cb);
}

At the same time the event-loop thread can be in Connection::~Connection (src/mp/proxy.cpp), running the connection's callbacks without any lock:

while (!m_sync_cleanup_fns.empty()) {
    m_sync_cleanup_fns.front()();
    m_sync_cleanup_fns.pop_front();
}

Two of those callbacks write the fields the destructor above reads. The one from SetThread (src/mp/proxy.cpp) clears m_disconnect_cb before taking the Waiter mutex (mutex), and the one from the ProxyClientBase constructor (include/mp/proxy-io.h) clears the connection pointer with no lock:

        thread->second.m_disconnect_cb.reset();
        // Remove connection pointer about to be destroyed from the map
        const std::unique_lock<std::mutex> lock(mutex);
        threads.erase(thread);
// ...
        m_context.connection = nullptr;

So the worker reads m_context.connection and m_disconnect_cb while the event loop writes them, and removeSyncCleanup erases from m_sync_cleanup_fns under EventLoop::m_mutex while ~Connection walks the same list unlocked. TSan reported all three in the "disconnecting and blocking during the call" test. The m_disconnect_cb race matters because it decides whether the worker calls into a Connection that is being destroyed.

The fix (fe4a188803c6) puts each access under the lock the other side already uses (the comment added to m_disconnect_cb is omitted):

@@ -405,6 +407,7 @@ ProxyClientBase<Interface, Impl>::ProxyClientBase(typename Interface::Client cli
+        Lock lock{m_context.loop->m_mutex};
         m_context.connection = nullptr;
@@ -132,9 +132,11 @@ Connection::~Connection()
+    Lock lock{m_loop->m_mutex};
     while (!m_sync_cleanup_fns.empty()) {
-        m_sync_cleanup_fns.front()();
-        m_sync_cleanup_fns.pop_front();
+        CleanupList fn;
+        fn.splice(fn.begin(), m_sync_cleanup_fns, m_sync_cleanup_fns.begin());
+        Unlock(lock, fn.front());
     }
@@ -320,9 +322,9 @@ std::tuple<ConnThread, bool> SetThread(ConnThreads& threads, std::mutex& mutex,
-        thread->second.m_disconnect_cb.reset();
         // Remove connection pointer about to be destroyed from the map
         const std::unique_lock<std::mutex> lock(mutex);
+        thread->second.m_disconnect_cb.reset();
         threads.erase(thread);

m_disconnect_cb is now only read and written under Waiter::m_mutex, and m_sync_cleanup_fns and m_context.connection only under EventLoop::m_mutex, so each read in ~ProxyClient<Thread> sees either the value before the disconnect callback ran or after it finished. ~Connection also now waits for a removeSyncCleanup call already in progress, because it needs the same mutex to take the next callback.

Fix

The fix commit fe4a188803c6 (+8/-3):

A remaining race between ~ProxyClient<Thread> and the disconnect callback was reported in mp#189 and fixed separately in mp#201 (bug page), by moving the whole erase onto the event loop thread rather than adding locks.

Timeline

Lessons

What would have prevented this bug, found it sooner, or made it easier to debug. The patterns page groups these lessons across all bugs.

Pattern Locks, waits and wakeups: A field locked by some accessors and not others

  • Prevented by lock annotations, a design rule
  • Earliest detector ThreadSanitizer (did not exist then)
  • Diagnosis moderate
  • Fix design moderate
  • Debugging aids sanitizer output, a reproduction script
  • Rust yes

Would Rust have prevented it? Yes. Fields read on a worker thread and written on the event-loop thread would have to be inside a Mutex (or be atomics) for the types to be Sync, so each of the three TSan races is a compile error; which lock, and the rule that the destructor must wait for removeSyncCleanup, remain design decisions. More on Rust

Could it have been prevented?

Largely. Each of the three races is a field that one side accessed under a lock and the other side did not: removeSyncCleanup erased from m_sync_cleanup_fns under EventLoop::m_mutex while ~Connection walked it bare; ~ProxyClient<Thread> read m_disconnect_cb under Waiter::m_mutex while the SetThread callback reset it before taking that mutex; m_context.connection was read on a worker and nulled on the loop thread with no lock at all. Clang -Wthread-safety annotations (GUARDED_BY on those three members) turn every one of these into a compile error, which is what Sjors asked for in review and what mp#201 added afterwards. Two of the sites date from the 2019 initial commit, when the library had no lock discipline written down; the third was added in 2024 by mp#106 into code that still had none, so the missing rule, not any single commit, is the cause.

Could it have been found sooner?

Yes, by TSan on a test that disconnects a client while a server call is in flight. Neither half existed until June 2025: the disconnect tests arrived in mp#160 and the library's TSan job in mp#186 itself, so the six-year-old races surfaced within eight days of the tests and on the first sanitizer run. Bitcoin Core's TSan job existed since 2024 but ipc_tests never broke a connection mid-call, so it had nothing to see; the fork CI report that did appear in the same week came from the new mptest cases running under Core's job.

What made it hard to debug?

Moderately hard, because the three reports arrived only after the first fix in the same PR (d8011c83608e) shifted the timing of ~Connection, and one of them (the m_loop reset) is benign on its own and only reportable because the flag it depends on was racy, which the commit message had to work out. TSan gave symbolized traces for both threads, and a local build-tsan loop of mptest reproduced them, so no gdb session was needed. The fix needed a lock hierarchy (a companion commit documents that Waiter::m_mutex may be taken before EventLoop::m_mutex but not the reverse) and the splice-then-Unlock shape in ~Connection so callbacks run without the mutex; the case the locks did not cover was reported ten days later as mp#189.

Lesson

A lock protects a field only if every accessor takes it; when one side of a shared structure is locked and the other is not, the locked side is documentation, not protection. Annotate guarded fields so the compiler finds the unlocked side, and run TSan on a test that ends a connection while work is in progress before trusting any teardown path.

Notes

Classification

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

Field Value
Symptom data-race
Worst effect observed sanitizer-report-only
Seen in mptest
Scope production
Trigger Client disconnects abruptly (client-unclean-disconnect)
Phase steady-state
Side server
Component proxy-io
Root cause kind missing-lock
Fix kind local-fix
Detection tsan
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see yes
Defect ever on master yes
Released source
Confidence high