Audit report AR-75

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

Report date

2026-09-29

Kind

Hardening

Status on master

Present

Proposed issue state

Open

Summary

When a Connection is destroyed, it runs a list of synchronous cleanup callbacks with no exception handling, so if one throws, the rest are skipped and the exception leaves the destructor. The report is accurate, and the effect of a skipped callback is worse than it says: client objects can be left pointing at the freed Connection. But no callback registered by libmultiprocess or Bitcoin Core throws, so Bitcoin Core is not affected. Robustness for other library users, in the same family as AR-35 (throwing log callback) and AR-63 (throwing asynchronous cleanup callback).

The report

When a connection is torn down, Connection's destructor runs its synchronous cleanup callbacks in order to clear client-side state. The loop does not catch exceptions, so a callback that throws escapes the destructor before later callbacks run. A test that deletes a Connection directly sees both the exception and the skipped callback.

The report rates it low. It is a supported libmultiprocess callback path, but no current Bitcoin Core callback that throws during disconnect was reproduced. Skipping a callback can leave dependent client state stale, and an exception reaching an owner with a noexcept destructor can terminate the process. The test proves only the skipped callback and the escaping exception.

Proposed change

Run every synchronous cleanup callback even if an earlier one throws, and log one error after releasing the event loop mutex:

-    Lock lock{m_loop->m_mutex};
-    while (!m_sync_cleanup_fns.empty()) {
-        CleanupList fn;
-        fn.splice(fn.begin(), m_sync_cleanup_fns, m_sync_cleanup_fns.begin());
-        Unlock(lock, fn.front());
+    bool cleanup_failed{false};
+    {
+        Lock lock{m_loop->m_mutex};
+        while (!m_sync_cleanup_fns.empty()) {
+            CleanupList fn;
+            fn.splice(fn.begin(), m_sync_cleanup_fns, m_sync_cleanup_fns.begin());
+            try {
+                Unlock(lock, fn.front());
+            } catch (...) {
+                cleanup_failed = true;
+            }
+        }
     }
+    if (cleanup_failed) MP_LOG(*m_loop, Log::Error) << "Connection sync cleanup threw during teardown";

The test "Connection teardown handles a throwing sync cleanup" registers two callbacks, the first to run throws, and checks that deleting the connection does not throw and the other callback still runs. The report considered rethrowing after the remaining callbacks, rejected because the connection is often owned by a noexcept destructor. Failures earlier in the destructor and the policy for failed destroy() calls are left out.

Assessment

Accurate. On libmultiprocess master and in Bitcoin Core's subtree, Connection::~Connection() runs the list with no try, and the destructor can throw: it is declared noexcept(false) on master, and in Bitcoin Core's copy its members' kj destructors already make it potentially throwing.

The callbacks on this list come from two places in the library. Each ProxyClient registers one that releases its capability and sets its Connection pointer to null, and each thread map entry registers one that removes the entry. Reading the code, if one callback throws, the remaining clients keep their pointer to the Connection being freed, and destroying those clients later calls into the freed object. That is a use-after-free, not just stale state. Where the exception goes depends on who deletes the connection: from the listener's disconnect handler it is logged by the event loop, and from a client that owns its connection it reaches a noexcept destructor and terminates the process.

Neither library callback throws in practice, and Bitcoin Core registers none of its own. The one indirect route is a throwing log callback inside a destructor, which is AR-35, and that terminates before reaching this loop. So this matters only to applications that register their own synchronous cleanups.

The patch is small and correct. Catching inside the loop is safe because the lock helper relocks during unwinding. Two suggestions: log the exception's text rather than a fixed message, and decide this together with AR-63 and AR-35, which raise the same question for other callbacks: may application callbacks throw during teardown? Documenting that synchronous cleanups must not throw is the cheaper alternative.

Status

Present on master. Idea for libmultiprocess, best handled together with the throwing-callback reports above. No public issue or PR yet.

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