IPC stability bugs

Bug 35

Disconnect test destroys a promise its server thread still needs

Status
Fixed
Symptom
Undefined behavior; 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-28 (first report)
Fixed
2025-07-01
Fix
mp#186
Shipped to Bitcoin Core
bitcoin#32345

Summary

The mptest test "Calling IPC method, disconnecting and blocking during the call" could destroy a std::promise that its server thread was still about to use, which MemorySanitizer reported as a read of destroyed memory. The defect was in test code and did not affect users. It was seen once in Bitcoin Core's MSan CI job and fixed in mp#186, a PR that fixed several mptest failures seen in Bitcoin Core CI, by reordering two declarations.

What happened

@Sjors's pull request Sjors/bitcoin#90, a Bitcoin Core fork PR that combined pending Bitcoin Core IPC changes with a libmultiprocess subtree update, failed the test in its MSan CI job on 2025-06-28 with:

[ TEST ] test.cpp:251: Calling IPC method, disconnecting and blocking during the call
MemorySanitizer: use-of-uninitialized-value
  Member fields were destroyed
SUMMARY: MemorySanitizer: use-of-uninitialized-value /msan/llvm-project/libcxx/src/future.cpp:154:16 in std::__1::promise<void>::get_future()

The fix's commit message links the report.

Root cause

In test/mp/test/test.cpp the test declared TestSetup setup{...} first and std::promise<void> signal; after it. TestSetup is the test fixture that runs a client and server connected over IPC. The test then set the server's m_fn to a lambda that calls setup.client_disconnect() and then signal.get_future().get(). The main thread waits for its callFnAsync() call to fail with the disconnect, calls signal.set_value() and returns.

Locals are destroyed in reverse order, so signal was destroyed before setup, and only ~TestSetup (which shuts down IPC and waits for the server thread) guarantees the server thread is done. Because the disconnect is triggered from inside m_fn, the main thread can see it, call set_value() and destroy signal before the server thread has even reached get_future(). The trace shows exactly that: the MSan read is in get_future() itself, called from the server thread's lambda, after ~promise had already run.

The code

The test code in test/mp/test/test.cpp, "Calling IPC method, disconnecting and blocking during the call", quoted from 84b292fcc4db^:

    TestSetup setup{/*client_owns_connection=*/false};
    ProxyClient<messages::FooInterface>* foo = setup.client.get();
    KJ_EXPECT(foo->add(1, 2) == 3);

    foo->initThreadMap();
    std::promise<void> signal;
    setup.server->m_impl->m_fn = [&] {
        EventLoopRef loop{*setup.server->m_context.loop};
        setup.client_disconnect();
        signal.get_future().get();
    };

    bool disconnected{false};
    try {
        foo->callFnAsync();
    } catch (const std::runtime_error& e) {
        KJ_EXPECT(std::string_view{e.what()} == "IPC client method call interrupted by disconnect.");
        disconnected = true;
    }
    KJ_EXPECT(disconnected);

    signal.set_value();
}

m_fn runs on the server's worker thread. Locals are destroyed in reverse order, so signal is destroyed before setup, and only ~TestSetup waits for the worker to finish:

  1. Worker: m_fn calls setup.client_disconnect().
  2. Main thread: callFnAsync() fails with the disconnect, signal.set_value() runs, the test function returns, and ~promise destroys signal.
  3. Worker: calls signal.get_future() on the destroyed promise. MSan reports reading its destroyed members; the trace shows ~promise at test.cpp:293 before get_future() at test.cpp:280.

Once get_future() has returned, the future holds its own reference to the shared state, so a worker already blocked in get() is not affected; the window is only before that call.

The fix (84b292fcc4db) declares signal before setup (the added comment is omitted):

@@ -268,12 +268,12 @@ KJ_TEST("Calling IPC method, disconnecting and blocking during the call")
+    std::promise<void> signal;
     TestSetup setup{/*client_owns_connection=*/false};
     ProxyClient<messages::FooInterface>* foo = setup.client.get();
     KJ_EXPECT(foo->add(1, 2) == 3);

     foo->initThreadMap();
-    std::promise<void> signal;
     setup.server->m_impl->m_fn = [&] {

signal is now destroyed after setup, and ~TestSetup joins the event-loop thread, which cannot exit while m_fn holds its EventLoopRef, so the worker is done with signal before it is destroyed.

Fix

The fix commit 84b292fcc4db declares signal before setup, so it outlives the server thread, and adds a comment (+6/-1). The comment gives the reason as "signalling may not wake up the thread right away", that is, a server thread already blocked in get(). That case was never unsafe: once get_future() has returned, the future holds its own reference to the shared state, which outlives the promise. The only window is the one in the reported trace, before the server thread calls get_future(), and the reordering closes it. The comment wording was changed after @Sjors asked what it meant (reply).

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 Built or torn down in the wrong order: A thread started before the members it uses

  • Prevented by a design rule
  • Earliest detector other (did not exist then)
  • Diagnosis moderate
  • Fix design easy
  • Debugging aids sanitizer output
  • Rust yes

Would Rust have prevented it? Yes. A closure that borrows signal and runs on another thread must be static in Rust, so a stack-owned promise could not be captured by reference; the equivalent oneshot channel halves are owned and moved, and the receiver keeps the shared state alive on its own. More on Rust

Could it have been prevented?

Yes, by a rule that is easy to state: anything a thread captures by reference must be declared before the object whose destructor joins that thread. In this test the std::promise the server thread reads was declared after the TestSetup fixture whose destructor is the only thing that waits for the server thread, so reverse destruction order destroyed the promise first. The fix (84b292fcc4db) is the rule applied, one moved declaration and a comment. The same PR fixed the mirror-image ordering bug in the fixture's own members (0d986ff144cd), which suggests the rule was not yet a habit when the tests were written for mp#160.

Could it have been found sooner?

Only by a sanitizer, and only intermittently. The window is between the main thread returning and the server thread reaching get_future(), so nothing fails without a tool, and even the MemorySanitizer job that reported it (Bitcoin Core's, on a fork PR staging the subtree update) saw it once. libmultiprocess had no MSan job and still does not (mp#188 is open); the TSan job added in the same PR did not report this test, so a library-side MSan run would have been the first chance, nine days after the test merged.

What made it hard to debug?

The trace was clear ("Member fields were destroyed", ~promise at one line before get_future() at another) and the fix followed within a day. The reasoning was the subtle part: the comment written with the fix attributes the problem to a slow wake-up after set_value(), but a thread already past get_future() holds its own reference to the shared state and is safe; the only window is before that call, which is what the trace showed. A correct fix with an inexact explanation is a mild warning sign that the lifetime rule was being rediscovered rather than applied.

Lesson

In a function or class that starts a thread, declaration order is the lifetime contract: declare what the thread uses before the thing that joins it, and say so in a comment, because the compiler enforces the order but not the reason.

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 test-code
Trigger Client disconnects abruptly (client-unclean-disconnect)
Phase shutdown
Side n/a
Component test-harness
Root cause kind lifetime
Fix kind test-only
Detection msan
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see yes
Confidence high