IPC stability bugs

Bug 86

Client thread hangs forever if building a request throws

Status
Open
Symptom
Hang; worst effect seen: hang
Where seen
Libmultiprocess tests (mptest)
Trigger
Unusual input from a peer or caller
First reported
Reported privately in an independent security review, 2026-08-07
Fixed
Not yet
In released binaries
Yes
Defect ever on master
Yes

Summary

If anything throws while an IPC client is building a request, the calling thread waits forever in clientInvoke, the library function that sends a call and waits for its reply. The exception escapes into EventLoop::loop(), which logs and discards it, so sync() returns normally, no request is sent, and nothing ever marks the call done. It was reported privately in an independent security review. A test with a throwing argument builder reproduces the hang every time, but it has not been seen in practice: no argument builder in the library or in Bitcoin Core throws in normal use. The unguarded code is in all releases since v30.0 (v30 and v31 in an older form that hangs in post() instead). No fix yet.

What happened

Root cause

clientInvoke (include/mp/proxy-types.h) does all client-side request setup on the event loop thread, inside proxy_client.m_context.loop->sync([&]() {...}): it gets the request, emplaces the invoke context, runs IterateFields().handleChain(..., BuildParams ...) to serialize every argument, then calls request.send().then(...). Only the response callback catches exceptions into an exception_ptr; the build half has no guard. done is set only in the "called after disconnect" branch and in the two .then callbacks, and the calling thread then blocks in thread_context.waiter->wait(lock, [&done]() { return done; }).

If the build half throws, the exception unwinds into EventLoop::loop() (src/mp/proxy.cpp). Since mp#260 ("event loop: tolerate unexpected exceptions in post() callbacks"), the loop runs the function under kj::runCatchingExceptions, logs EventLoop: m_sync_fn threw: ..., clears m_sync_fn and notifies. So sync() returns normally with no request sent, done stays false, and the caller waits forever. The comment mp#260 added in loop() anticipates this: the caller "will return without any indication of failure, which will likely cause other bugs". mp#347 later renamed post()/m_post_fn to sync()/m_sync_fn and changed nothing else relevant here.

Today nothing realistic throws there. Library BuildPrimitives use static_asserts, and Bitcoin Core's CustomBuildFields (serialization into DataStream, UniValue::write) do not throw in practice. What is left is Cap'n Proto size limits, bad_alloc, the ThreadMap request made inside the Context builder's SetThread, or a future validating CustomBuildField. The private report noted a second consequence of such a throw inside SetThread: it would leave a disengaged std::optional in the thread map, which the next call from that thread would dereference.

Fix

No fix yet. ryanofsky had suggested during mp#260 review catching the exception on the event loop thread and rethrowing it to the caller, and repeated it in his merge ACK as the best follow-up; the private report suggests the same. Either the sync() lambda in clientInvoke or sync() itself would capture the exception and rethrow it in the calling thread.

Reproduction

A test in mptest, the libmultiprocess unit test program, makes the test's custom builder for its FooMessage argument type throw when the message is "throw". It then calls passMessage with that message from a separate thread and waits up to 5 seconds for the call to return or throw. It was run on 2026-09-26 against Bitcoin Core master bfdcd97, which carries libmultiprocess v13 in its subtree.

Patch: mptest-repro.patch. It applies to Bitcoin Core master; last checked against badc957 on 2026-10-15. From a Bitcoin Core checkout:

curl -sO https://ipcbugs.ofsky.org/repro/mptest-repro.patch
git apply mptest-repro.patch
cmake --build build --target mptest
build/src/ipc/libmultiprocess/test/mptest -f test.cpp:600 --verbose

The call never returns. The event loop logs the discarded exception (the subtree still uses the older m_post_fn name), and the test fails after 5 seconds:

EventLoop: m_post_fn threw: ... std::exception: FooMessage build throw
passMessage still blocked in clientInvoke after 5 seconds
failed: expected finished; clientInvoke hung after CustomBuildField threw

The result is the same on every run. Teardown still completes, because the blocked thread holds no lock while it waits. The test patch is available on request and will be included in the GitHub issue for this bug when it is opened.

Next steps

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 Error paths that skip cleanup or completion: A throw skips the done signal

  • Prevented by a different API shape
  • Earliest detector a unit test (did not exist then)
  • Diagnosis easy
  • Fix design easy
  • Debugging aids logging, thread names
  • Rust no

Would Rust have prevented it? No. Losing the completion signal when the posted closure fails is a control-flow design gap; Result makes returning the build error from sync() the natural shape, but a panic inside the loop closure would still leave the caller waiting on its condition variable. More on Rust

Could it have been prevented?

Yes, by giving EventLoop::sync() the shape of a function call: run the callback on the loop thread and either return or rethrow in the caller. Today the callback's exception is caught by the loop, logged, and dropped, and sync() returns as if the work completed, so any caller whose "done" flag is set inside the callback waits forever. clientInvoke has built its request inside sync() without a guard since the initial commit (06e1045baba0); before mp#260 the same throw took the whole loop down instead. ryanofsky asked for catch-and-rethrow as the follow-up during that review, and the comment mp#260 added to loop() predicts this outcome ("will likely cause other bugs"), so the rule was known; it was not applied because nothing in the library or Bitcoin Core throws while building a request today.

Could it have been found sooner?

Yes, by a unit test with an argument builder that throws, which is what the survey's 2026-09-26 mptest case does: passMessage is still blocked five seconds after the loop logs m_post_fn threw. No such test existed, because no builder throws and the mp#259 report stated that clientInvoke was safe, which is true only of its response half. The check is cheap and deterministic; it was simply not written when mp#260 chose to make the loop survive a throw without deciding what the waiting caller should see.

What made it hard to debug?

Little, in the form it would take in the field: the loop's EventLoop: m_sync_fn threw: ... log line, added by mp#260, names the exception, and a thread dump would show one client thread parked in clientInvoke's waiter->wait with no request in flight. The harder part was noticing that the bug survived mp#260 at all; the independent security review that reported it modelled the mechanism rather than running the library, and it took a survey test to confirm the hang on Bitcoin Core master. A second consequence, a disengaged std::optional left in the thread map if the throw happens inside SetThread, is only visible on the next call from that thread.

Lesson

A cross-thread "run this and wait" primitive must carry failure back to the waiter; logging and returning normally turns every exception into a hang for whichever caller was counting on the callback to set its completion flag. Write the throwing-callback test when adding the primitive, not when something finally throws.

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 hang
Worst effect observed hang
Seen in mptest
Scope production
Trigger Unusual input (unusual-input)
Phase any
Side client
Component proxy-types
Root cause kind unhandled-exception
Fix kind none
Detection review
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released binary
Confidence medium