IPC stability bugs

Bug 33

Calling a method after disconnect binds a reference to a null connection

Status
Fixed
Symptom
Undefined behavior; worst effect seen: sanitizer report only
Where seen
Libmultiprocess tests (mptest)
Trigger
The process closes its own connection while the peer is still there
First reported
2025-06-27 (first report)
Fixed
2025-07-01
Fix
mp#186
Shipped to Bitcoin Core
bitcoin#32345
In released binaries
No
Defect ever on master
Yes

Summary

When a C++ IPC client called a method after its connection was closed, clientInvoke (the library function that sends a call and waits for the reply) bound a Connection& reference to a null pointer before checking for the disconnect. The reference was never used on that path, so the only effect was an UndefinedBehaviorSanitizer report in the mptest test "Call IPC method after client connection is closed". The bad ordering existed on libmultiprocess master for twelve days (2025-06-19 to 2025-07-01) and never reached Bitcoin Core's subtree or any release. Fixed in mp#186, a PR that fixed several mptest failures seen in Bitcoin Core CI.

What happened

@Sjors's pull request Sjors/bitcoin#90, a Bitcoin Core fork PR whose CI also builds and runs the libmultiprocess tests, failed its UBSan job twice on 2025-06-27/28 with:

[ TEST ] test.cpp:197: Call IPC method after client connection is closed
include/mp/proxy-types.h:612:40: runtime error: reference binding to null pointer of type 'Connection'
SUMMARY: UndefinedBehaviorSanitizer: null-pointer-use include/mp/proxy-types.h:612:40

The fix's commit message links both reports (first, second).

Root cause

A client proxy keeps a pointer to its Connection, and a disconnect callback sets that pointer to null when the connection goes away. The commit 56fff76f940b, part of mp#160 ("EventLoop locking cleanups + client disconnect exception"), moved clientInvoke's "called after disconnect" check from the calling thread into the lambda that loop->sync() runs on the event loop thread. It left this line above the sync call:

ClientInvokeContext invoke_context{*proxy_client.m_context.connection, g_thread_context};

After a disconnect, m_context.connection is null, so this binds a Connection& to *nullptr before the lambda runs its check. That is undefined behavior even though the lambda then returns early with "IPC client method called after disconnect." and never reads the reference. On the connected path the reference is only used inside the same sync() call, where the connection is alive. The fix's commit message says it "should not cause problems in practice because the reference is not used".

The code

Quoted from e09143d2ea2f^. clientInvoke (include/mp/proxy-types.h) builds its ClientInvokeContext on the calling thread, before the sync() lambda checks whether the connection still exists:

ClientInvokeContext invoke_context{*proxy_client.m_context.connection, g_thread_context};
// ...
proxy_client.m_context.loop->sync([&]() {
    if (!proxy_client.m_context.connection) {
        const std::unique_lock<std::mutex> lock(invoke_context.thread_context.waiter->m_mutex);
        done = true;
        disconnected = "IPC client method called after disconnect.";
        invoke_context.thread_context.waiter->m_cv.notify_all();
        return;
    }

ClientInvokeContext (include/mp/proxy-io.h) holds the connection as a reference:

struct ClientInvokeContext : InvokeContext
{
    ThreadContext& thread_context;
    ClientInvokeContext(Connection& conn, ThreadContext& thread_context)
        : InvokeContext{conn}, thread_context{thread_context}

After a disconnect, the disconnect callback has set m_context.connection to null, so a method call dereferences a null pointer to bind Connection& conn. The lambda then returns early without reading the reference, but binding it is already undefined behaviour, and UBSan reports "reference binding to null pointer of type 'Connection'" in the "Call IPC method after client connection is closed" test.

The fix (e09143d2ea2f) keeps the thread context separately and constructs the invoke context inside the lambda, after the check. The hunks that rename invoke_context.thread_context to thread_context and invoke_context to *invoke_context elsewhere are omitted:

@@ -609,42 +609,44 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel
-    ClientInvokeContext invoke_context{*proxy_client.m_context.connection, g_thread_context};
+    ThreadContext& thread_context{g_thread_context};
+    std::optional<ClientInvokeContext> invoke_context; // Must outlive waiter->wait() call below
@@
         auto request = (proxy_client.m_client.*get_request)(nullptr);
         using Request = CapRequestTraits<decltype(request)>;
         using FieldList = typename ProxyClientMethodTraits<typename Request::Params>::Fields;
-        IterateFields().handleChain(invoke_context, request, FieldList(), typename FieldObjs::BuildParams{&fields}...);
+        invoke_context.emplace(*proxy_client.m_context.connection, thread_context);
+        IterateFields().handleChain(*invoke_context, request, FieldList(), typename FieldObjs::BuildParams{&fields}...);

The reference is now bound only after the lambda has seen a non-null m_context.connection, and on the event-loop thread, where the disconnect callback that clears the pointer also runs, so it is never bound to null or to a connection being destroyed.

Fix

The fix commit e09143d2ea2f makes invoke_context a std::optional<ClientInvokeContext> declared in the outer scope and emplaces it inside the lambda only after the null check passes, taking ThreadContext& thread_context{g_thread_context} separately for the waiter and logging uses (+16/-14 in include/mp/proxy-types.h). Sjors asked why it was not declared inside the lambda; ryanofsky explained that it must outlive the waiter->wait() call because the request.send().then(...) continuation reads it, and added a comment saying so.

A side effect: the read of m_context.connection that built the reference used to happen on the calling thread with no synchronization against the disconnect callback that nulls it; it now happens on the event loop thread inside the same sync().

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 Work or cleanup on the wrong thread: A check moved away from the use it guards

  • Prevented by a thread-affinity assertion, a review checklist item
  • Earliest detector UBSan (did not exist then)
  • Diagnosis easy
  • Fix design easy
  • Debugging aids sanitizer output, a clear assertion message
  • Rust yes

Would Rust have prevented it? Yes. Rust has no null references: the connection would be an Option or a raw pointer that must be checked before a reference exists, so the reference could only be formed inside the branch that has seen Some (an unwrap() before the check would panic instead of being silent UB). More on Rust

Could it have been prevented?

Yes. The introducing commit (56fff76f940b, mp#160) moved the "called after disconnect" check into the loop->sync() lambda but left the line that dereferences m_context.connection above it, so the check now guarded nothing. The underlying rule is thread affinity: m_context.connection is written by the disconnect callback on the event-loop thread, so it should only be read there, and an assert(loop->isLoopThread()) on that read would have failed the very test the same PR added. Short of that, a review item "when a check moves, does every use it guards move with it?" catches this diff in one reading; the fix (e09143d2ea2f) does exactly that by constructing the context inside the lambda after the check.

Could it have been found sooner?

Yes, and it nearly was: the test that exercises the path ("Call IPC method after client connection is closed") landed in the same PR, so any UBSan run of mptest would have reported it on the day of the merge. libmultiprocess had no UBSan job then; the first UBSan run of the new test was Bitcoin Core's CI on a fork PR a week later, which reported it twice. Enabling UBSan in the library's own sanitizer job was deferred during the fix because of unexplained interactions with TSan, so the library still relied on a downstream CI to see this class of report.

What made it hard to debug?

Nothing. UBSan named the file and line and the message ("reference binding to null pointer of type 'Connection'") says what happened. The fix took four days from report to merge, and the only design question in review was why the std::optional lives in the outer scope (it must outlive waiter->wait() because the send continuation reads it), which ryanofsky answered with a comment in the code.

Lesson

When a guard moves to a different thread or scope, move every operation it protects with it, and dereference a pointer another thread can clear only on the thread that clears it. A sanitizer run on the test that ships with a change is the cheapest check there is; make sure the project runs one rather than relying on a downstream CI to do it.

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 ub
Worst effect observed sanitizer-report-only
Seen in mptest
Scope production
Trigger Process closes its own connection (local-disconnect)
Phase steady-state
Side client
Component proxy-types
Root cause kind ordering
Fix kind local-fix
Detection ubsan
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see yes
Defect ever on master yes
Released no
Confidence high