IPC stability bugs

Bug 19

Closing a connection writes to freed memory and leaks server objects

Status
Fixed
Symptom
Undefined behavior; worst effect seen: sanitizer report only
Where seen
Bitcoin Core unit tests
Trigger
A peer disconnects cleanly
First reported
2024-09-26 (first report)
Fixed
2024-10-03
Fix
mp#116
Shipped to Bitcoin Core
bitcoin#31105
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

When a libmultiprocess Connection was destroyed while the other side still held references to objects this side was serving, the cleanup of those objects wrote into memory that had already been freed. This is undefined behavior, and the cleanups queued that way never ran, so the wrapped objects leaked. The cause was the order in which ~Connection destroyed its members: the lists of cleanup functions went before the Cap'n Proto RpcSystem, and it is the RpcSystem's teardown that disposes of the remaining ProxyServer objects and queues their cleanups. It showed up only as a MemorySanitizer report in Bitcoin Core CI. Every connection teardown with undestroyed server objects hit it, in source builds of v22 through v28 with multiprocess enabled. Fixed in mp#116.

What happened

@Sjors's bitcoin#30975 ("ci: build multiprocess on most jobs") built multiprocess on most CI jobs, which ran ipc_tests under MemorySanitizer for the first time. It failed with MemorySanitizer: use-of-uninitialized-value /usr/src/mp/proxy.cpp:146:25 in mp::Connection::addAsyncCleanup, with a second stack ("Member fields were destroyed") showing the std::list already destroyed by ~Connection. @maflcko quoted the summary line the day the PR was opened (2024-09-26), and Sjors posted it again with the Cirrus task on 2024-10-01. ryanofsky opened mp#115 ("MSAN Error during connection shutdown") with the full trace and analysis the same day.

Root cause

Connection (include/mp/proxy-io.h) declared m_rpc_system before the m_sync_cleanup_fns and m_async_cleanup_fns lists, so on destruction the lists went first, then m_threads and m_thread_map, then m_rpc_system, and m_network last. Destroying the capnp::RpcSystem drops its exports table, which disposes of every ProxyServer the peer had not released: objects of interfaces without a destroy method, or any object whose peer went away. ~ProxyServerBase then calls m_context.connection->addAsyncCleanup(...) to schedule destruction of m_impl, inserting into a list whose storage is gone. Besides the invalid write, cleanups queued this way were never run, so the m_impl objects leaked.

The failing test destroyed its connection from an onDisconnect handler, the same pattern that ServeStream, the library function serving a connection on a socket, uses, so the bug was not specific to the test. The write to destroyed list storage is silent under a normal allocator, which is why it was only noticed under MSan.

The code

Quoted from 2350843a3196^. Connection in include/mp/proxy-io.h declares its RPC system before its cleanup lists:

    ::capnp::TwoPartyVatNetwork m_network;
    ::capnp::RpcSystem<::capnp::rpc::twoparty::VatId> m_rpc_system;
    // ...
    CleanupList m_sync_cleanup_fns;
    CleanupList m_async_cleanup_fns;
};

C++ destroys members in reverse order, so after the body of ~Connection runs, m_async_cleanup_fns is destroyed first and m_rpc_system later. Destroying the RPC system releases every ProxyServer the peer never called destroy on, and ~ProxyServerBase in the same file queues its implementation's destruction on the connection:

ProxyServerBase<Interface, Impl>::~ProxyServerBase()
{
    if (m_impl) {
        // ...
        m_context.connection->addAsyncCleanup([impl=std::move(m_impl), c=std::move(m_context.cleanup)]() mutable {

Connection::addAsyncCleanup (src/mp/proxy.cpp) then does

m_async_cleanup_fns.emplace(m_async_cleanup_fns.end(), std::move(fn));

on a list that has already been destroyed. MSan reports the use of the dead list, and the queued callback never runs, so the m_impl object leaks.

The fix destroys the RPC system explicitly at the start of ~Connection, while the lists are alive and before the loop that hands the async list to the cleanup thread (2350843a3196; the -> changes at the other m_rpc_system uses are omitted):

@@ -333,7 +333,7 @@ public:
-    ::capnp::RpcSystem<::capnp::rpc::twoparty::VatId> m_rpc_system;
+    std::optional<::capnp::RpcSystem<::capnp::rpc::twoparty::VatId>> m_rpc_system;
@@ -48,6 +48,12 @@ void LoggingErrorHandler::taskFailed(kj::Exception&& exception)
 Connection::~Connection()
 {
+    // Shut down RPC system first, since this will garbage collect Server
+    // objects that were not freed before the connection was closed, some of
+    // which may call addAsyncCleanup and add more cleanup callbacks which can
+    // run below.
+    m_rpc_system.reset();
+

Any ProxyServer released by the RPC system now adds its callback to a live list, and the rest of ~Connection moves those callbacks to the async cleanup thread, so the implementations are destroyed instead of leaked.

Fix

The fix commit 2350843a3196, in mp#116 ("shutdown bugfix: destroy RPC system before running cleanup callbacks"), makes m_rpc_system a std::optional and resets it as the first statement of ~Connection, before the cleanup callbacks run, with a comment that garbage collection may add more cleanup callbacks, which then run below (+8/-2). Sjors confirmed the MSan job passed with the fix.

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: Teardown in the wrong order

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

Would Rust have prevented it? Yes. A ProxyServer's Drop could not reach its Connection through a plain reference (the struct would be self-referential), so it would go through Rc or Weak, where the cleanup list is either still alive or the upgrade fails and the callback is skipped; the write into a dead member of a live struct is unwritable in safe Rust, though the impl leak could remain. More on Rust

Could it have been prevented?

Yes. ~ProxyServerBase calls back into its Connection (addAsyncCleanup), and the RpcSystem that disposes of those servers was a plain member declared before the cleanup lists, so implicit member destruction ran the callbacks into lists that were already gone. The rule the fix applies is a standard one: a member whose destructor can call back into the enclosing object must be destroyed explicitly at the top of the destructor, while everything it may touch is still alive. The fix commit (2350843a3196) does exactly that, wrapping m_rpc_system in std::optional and resetting it as the first statement of ~Connection with a comment saying why. This was knowable in 2019, when the same commit wrote both the callback and the member order; it is the kind of dependency that a class comment listing "members that run callbacks on destruction" would have kept visible.

Could it have been found sooner?

Yes, by running ipc_tests under MemorySanitizer, and by nothing weaker. The write lands in a destroyed std::list whose storage is still inside the live Connection object, so ASan sees no freed memory and a normal allocator is silent; only MSan's use-after-destructor poisoning flags it, and the leaked m_impl objects were never counted. Bitcoin Core had an MSan job in 2024 but did not build multiprocess in it until bitcoin#30975 turned multiprocess on for most CI jobs; the failure appeared on that PR's first run, five years after the order was set and with every connection teardown in every ipc_tests run having hit it silently.

What made it hard to debug?

Nothing, once seen. The MSan report named the line in addAsyncCleanup and its second stack ("Member fields were destroyed") named the destructor that had already run, so the analysis in mp#115 and a proposed diff came the day the report was picked up, and the fix merged two days later. The delay was in the reporting: maflcko quoted the summary line on 2024-09-26 and it was not acted on until Sjors reposted it with the CI link five days later, a small cost of the sanitizer running in a repository other than the one that owned the code.

Lesson

When a member's destructor can call back into its owner, destroy that member first and by hand; implicit reverse-declaration order only protects dependencies that run one way. Run the code that owns your destructors under MSan, because a write into a destroyed member of a still-live object is invisible to everything else.

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 unit-test
Scope production
Trigger Clean disconnect (clean-disconnect)
Phase shutdown
Side server
Component connection
Root cause kind ordering
Fix kind local-fix
Detection msan
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see yes
Defect ever on master yes
Released source
Confidence high