IPC stability bugs

Bug 10

Connection deleted twice when its disconnect handler ran after destruction

Status
Fixed
Symptom
Crash; worst effect seen: crash
Where seen
Branch-build
Trigger
The process closes its own connection while the peer is still there
First reported
2019-08-06 (first report)
Fixed
2019-08-06
Fix
mp#7
Shipped to Bitcoin Core
bitcoin#18677
In released binaries
No
Defect ever on master
Yes

Summary

A Connection's disconnect handler could run after the Connection had already been destroyed and delete it a second time. It showed up at shutdown on the bitcoin#10102 branch, which splits Bitcoin Core into node, wallet and GUI processes, as soon as destroying the last client proxy started destroying the Connection directly on the event-loop thread. This was in libmultiprocess's first weeks, before Bitcoin Core used the library, so no release was affected. Fixed in mp#7, the same PR that added the destroy_connection option which exposed it; a narrower window of the same bug remained (bug page) and is addressed in mp#361, not yet merged.

What happened

Root cause

Before mp#7, callers registered disconnect handlers directly on the global EventLoop::m_task_set (loop.m_task_set->add(connection->m_network.onDisconnect().then(...)) in test.cpp and in bitcoin#10102's ipc.cpp). Nothing tied that promise to the lifetime of the Connection that produced it, so after the Connection was destroyed the promise stayed queued and its continuation could still run. The double delete needs two things: a handler that deletes the Connection, as bitcoin#10102's did, and a second deletion elsewhere, which the destroy_connection option added in the same commit provides from ~ProxyClientBase. The handler in libmultiprocess's own test only resets a std::unique_ptr, which is harmless to run twice.

With the old ShutdownLoop sequence, the client capability was released on the event-loop thread, control switched back to the client thread, and then back to the loop to free the connection, so the loop had most likely already processed the disconnect event and the window never opened. Destroying the capability and the Connection back to back on the loop thread opened it.

The code

Quoted from c685fa9bedaf^. Code that wanted to act on a disconnect added a continuation of the connection's onDisconnect() promise to the event loop's global task set, as the test in src/mp/test/test.cpp does:

loop.m_task_set->add(connection_server->m_network.onDisconnect().then([&] { connection_server.reset(); }));

m_network is a member of Connection (include/mp/proxy-io.h), but the continuation lives in EventLoop::m_task_set, which outlives it:

EventLoop& m_loop;
kj::Own<kj::AsyncIoStream> m_stream;
::capnp::TwoPartyVatNetwork m_network;
::capnp::RpcSystem<::capnp::rpc::twoparty::VatId> m_rpc_system;

Destroying the Connection does not cancel the queued handler. Bitcoin Core's client code (bitcoin#10102) registered such a handler to delete its Connection, and the destroy_connection option this commit adds deletes it from ~ProxyClientBase as well:

  1. Event loop: ~ProxyClientBase releases the client capability and, right after, deletes the Connection.
  2. Event loop: the disconnect event is processed; the handler in the global task set runs and deletes the same Connection again.

Before this commit the client released the capability and destroyed the connection in two separate sync() calls, and the loop usually processed the disconnect in between, so the handler ran while the Connection was still alive.

The fix gives each Connection its own task set for disconnect handlers, destroyed with it, and an onDisconnect() method that adds them there (c685fa9bedaf; the destroy_connection option and constructor changes are omitted):

@@ -297,8 +301,23 @@ public:
+    //! Add disconnect handler.
+    template <typename F>
+    void onDisconnect(F&& f)
+    {
+        m_on_disconnect.add(m_network.onDisconnect().then(
+            kj::mvCapture(f, [this](F&& f) { m_loop.m_task_set->add(kj::evalLater(kj::mv(f))); })));
+    }
+
     EventLoop& m_loop;
     kj::Own<kj::AsyncIoStream> m_stream;
+    LoggingErrorHandler m_error_handler{m_loop};
+    kj::TaskSet m_on_disconnect{m_error_handler};
     ::capnp::TwoPartyVatNetwork m_network;

Destroying a Connection now destroys m_on_disconnect and cancels any handler still waiting on it. A handler that has fired is moved to the global task set with kj::evalLater, so one that deletes the Connection does not destroy the promise it is running in.

Fix

c685fa9bedaf gives Connection its own kj::TaskSet m_on_disconnect and an onDisconnect(F&&) helper in include/mp/proxy-io.h that adds handlers there, so they are cancelled when the Connection is destroyed. When the disconnect fires, the handler body is re-posted to the EventLoop task set with kj::evalLater, so a handler that deletes its own Connection does not destroy the promise it is running inside. Most of the rest of the commit is the destroy_connection feature and the constructor signature change that exposed the race.

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 An object outliving what it points to: A callback outlives what it captured

  • Prevented by a clear ownership model, a different API shape
  • Earliest detector shutdown test (did not exist then)
  • Diagnosis easy
  • Fix design moderate
  • Debugging aids sanitizer output, logging
  • Rust yes

Would Rust have prevented it? Yes. A continuation added to the loop's task set must be 'static, so it could not capture a bare reference to the Connection, and there is no raw delete to run twice; the handler would hold a Weak or an Rc, and an Rc would keep the Connection alive until the handler ran and dropped it, which in this next-turn case is a short delay rather than a hang. More on Rust

Could it have been prevented?

Partly. Before c685fa9bedaf callers added disconnect continuations straight to the global EventLoop::m_task_set, so a handler that referred to a Connection was owned by something that outlived it. The API shape the fix introduces, Connection::onDisconnect() as the one way to register a handler, with the promise held in a per-connection kj::TaskSet, is the right instinct: the handler belongs to the object it will act on. The original design did not need it, because bitcoin#10102's ShutdownLoop released capabilities and destroyed the connection in separate steps with the loop running in between, and the gap only opened when destroy_connection folded both into one destructor. That the rule "a callback must be owned by what it references" was only half applied (the fix re-posts the fired handler to the global set, leaving the window fixed in mp#361) shows how easy the rule is to state and to lose in the same commit.

Could it have been found sooner?

It was found a month after the initial commit, by ryanofsky, while making the change that exposed it, so there was little room. The check that would have shown it in the library itself is a shutdown test with destroy_connection=true and a handler that deletes the connection; mptest used destroy_connection=false and its handler only reset a std::unique_ptr, which is harmless to run twice, so the only place the double delete ran was the bitcoin#10102 binaries. No such test was added with the fix, and the mp#7 PR had no reviewers or comments.

What made it hard to debug?

Not much is recorded: the PR body describes a double deletion of the Connection pointer at shutdown and the reasoning about the old sequence's extra thread switches that had hidden it. The failure was deterministic once the capability and the Connection were destroyed back to back on the loop thread. What the record cannot say is whether a sanitizer was involved; a double free under ASan reports both stacks and would have made the second deleter obvious. The harder part was the fix design: KJ raises "promise callback destroyed itself" if a handler deletes the Connection that owns the promise it runs in, and the kj::evalLater hop chosen to avoid that is what let a fired handler escape cancellation for seven years, until enirox001 found it.

Lesson

A callback that refers to an object should be owned by that object, or check that the object is alive before running; when a framework constraint forces you to hand the callback to a longer-lived owner, that check is the only thing left protecting it.

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 crash
Worst effect observed crash
Seen in branch-build
Scope production
Trigger Process closes its own connection (local-disconnect)
Phase shutdown
Side both
Component connection
Root cause kind lifetime
Fix kind new-code-path
Detection manual-test
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see unknown
Defect ever on master yes
Released no
Confidence high