Audit report AR-35

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

Report date

2026-09-16

Kind

Hardening

Status on master

Present

Proposed issue state

Open

Linked bugs

Bug 8: Node aborts when freeing a callback object from a client that disconnected

Summary

Several libmultiprocess lifecycle paths log before doing essential cleanup, and some log from destructors that cannot throw. If an application's log callback throws, a listener can keep a closed connection (and with a connection limit, stop accepting), a client can keep its Connection, and a throw from a destructor terminates the process. The report is correct about the code. Bitcoin Core's log callback does not throw at these log levels, so this matters only to other library users. Robustness improvement; the proposed patch is heavier than the benefit for Bitcoin Core.

The report

libmultiprocess lets an application install its own logging callback, and that callback is allowed to throw. The report lists the places where a throw leaves cleanup undone:

• The listener logs "IPC server: socket connected." before registering the connection's disconnect handler and before continuing to accept.
• The listener's disconnect handler logs "IPC server: socket disconnected." before erasing the connection and updating the listener's capacity count.
• An owning client's unexpected-disconnect handler logs before deleting its connection.
• Generated proxy destructors and both proxy base destructors log, although C++ destructors are noexcept. So does the warning added by mp#273 after a failed remote destroy() call during cleanup.

Consequences it describes: each of the first three exceptions reaches the event loop's task error handler while the connection stays retained, and the two listener cases also skip the disconnect hook. With a one-connection listener, a single failed "connected" log uses up the only slot, so no later client is accepted. Retained connections hold event-loop references and can delay shutdown. A throw from a destructor log terminates the process. It notes that Bitcoin Core's callback is not known to throw and that no peer can make it throw.

The reproduction is five KJ tests against the real library, each with a logger that throws on one specific message: "ListenConnections survives a throwing connected log callback", "ListenConnections survives a throwing disconnected log callback", "ConnectStream survives a throwing unexpected-disconnect log callback", "Proxy client destruction with a throwing logger", and "Destroying ProxyClient<> after peer disconnect with a throwing logger". The destructor cases terminate a child process before the fix.

Proposed change

Six commits, alternating tests and fixes:

+template <typename Fn>
+void LogNoThrow(Fn&& fn) noexcept
+{
+    KJ_IF_MAYBE(exception, kj::runCatchingExceptions(std::forward<Fn>(fn))) {
+        KJ_LOG(ERROR, "Logging callback threw from a noexcept context.", *exception);
+    }
+}
@@ _Serve
-    MP_LOG(loop, Log::Info) << "IPC server: socket connected.";
-    if (loop.testing_hook_connected) loop.testing_hook_connected();
     it->onDisconnect([&loop, it, on_disconnect = ...]() mutable {
-        MP_LOG(loop, Log::Info) << "IPC server: socket disconnected.";
-        loop.m_incoming_connections.erase(it);
-        on_disconnect();
+        TryFinally(
+            [&] { MP_LOG(loop, Log::Info) << "IPC server: socket disconnected."; },
+            [&] {
+                loop.m_incoming_connections.erase(it);
+                on_disconnect();

• Register the disconnect cleanup, count the connection and continue accepting before logging "connected" (the log moves into _Listen and ServeStream).
• Wrap the server and owning-client disconnect handlers in the existing TryFinally() helper, so cleanup runs even if logging throws; the exception still reaches the task error handler.
• Route destructor logging, including the failed-destroy() warning, through LogNoThrow, which reports a throwing callback through KJ logging instead of terminating.

The report considered catching every logging exception globally and rejected it because it changes public error behaviour. It prefers landing the change in libmultiprocess first.

Assessment

Present on libmultiprocess master, in a moved form: _Serve still logs before registering the disconnect handler and logs before erasing the connection, and since mp#298 the owning client's disconnect handler lives in ProxyClientBase and logs "IPC client: unexpected network disconnect." before delete connection. ~ProxyClientBase() is noexcept and logs. The patch's ConnectStream hunk targets the code before mp#298 and would need rebasing.

Bitcoin Core's callback, IpcLogFn in src/ipc/capnp/protocol.cpp, forwards Trace, Debug, Info, Warning and Error to the node's logging and throws only for Log::Raise, which none of these sites uses. So Bitcoin Core is not affected, and no peer is involved. This is robustness for other library users.

The mp#273 warning this touches was added to fix a real crash: bitcoin-node aborts when a server-owned ProxyClient's remote destroy() call throws after the peer disconnected. Related logging report: AR-33.

Status

Present on master. A simpler alternative to the six-commit change is to document that log callbacks must not throw except for Log::Raise, possibly with LogNoThrow in destructors only. Idea for libmultiprocess; no issue open.

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