Audit report AR-34

Reported privately in an independent security review, 2026-09-16.

Report date: 2026-09-16
Kind: Bug
Status on master: Present
Proposed issue state: Open
Linked bugs: Bug 86: Client thread hangs forever if building a request throws

Summary

SetThread() in libmultiprocess reserves an empty entry in a connection's thread map, releases the lock, and then calls the factory that creates the thread client. If the factory throws, the empty entry stays behind, and the next call for that thread and connection treats it as a constructed ProxyClient<Thread>, which is undefined behaviour. The report is correct about the mechanism. The factories used today throw only on allocation failure, and a throw there would first hit a separate hang, so this is latent. Small fix, mergeable.

The report

SetThread() (src/mp/proxy.cpp) looks up the entry for a connection in a thread's request or callback thread map. Under the map's mutex it calls try_emplace(connection), which inserts a disengaged std::optional<ProxyClient<Thread>>. It then releases the mutex and, if the entry is new, constructs the client in place from the supplied factory (make_thread()). If the factory throws, the reservation remains. A later caller finds the existing entry, gets inserted == false, and uses thread->second->... on an empty optional.

The report limits the impact to a libmultiprocess user whose thread-client factory throws and which then reuses the same connection. Bitcoin Core reaches this helper during IPC setup, but no stock factory exception followed by reuse was reproduced, and the later invalid dereference is supported from source only.

The reproduction is a KJ test against the real library, "SetThread construction failure". It injects a throwing factory on the real event-loop thread and checks that the empty entry survives; it deliberately stops before the dereference.

Proposed change

Catch the construction failure, erase the reserved entry under the mutex, and rethrow, so the next call can retry:

     if (inserted) {
-        thread->second.emplace(make_thread(), connection, /* destroy_connection= */ false);
+        try {
+            thread->second.emplace(make_thread(), connection, /* destroy_connection= */ false);
+        } catch (...) {
+            const Lock lock(threads.mutex);
+            threads.ref.erase(thread);
+            throw;
+        }

The report considered constructing the client under the map mutex instead, which avoids the reservation window but holds the lock across the factory call, and preferred cleaning up only on failure. It prefers landing the change in libmultiprocess first.

Assessment

The mechanism is present on libmultiprocess master. In practice the factories do not throw except on bad_alloc: on the client side the factory sends a makeThread request and waits for its result, and on the server side it adds a local thread server to the connection.

On the client side, SetThread runs while building the Context argument, inside clientInvoke's sync() call on the event loop. A throw there would first hit the hang described in AR-14: the event loop logs and discards the exception and the calling thread waits forever. The poisoned entry would then crash the next call from any other use of that thread and connection. So the two reports are related failure modes of the same unlikely throw, and fixing this one is cheap.

Status

Present on master; latent. Next step: a small libmultiprocess PR with the fix and test, ideally alongside the fix for AR-14. No public PR yet.

Reported privately in an independent security review on 2026-09-16.