Prevent IPC server crash if disconnected during IPC call

Antoine Poinsot <[email protected]> reported a bug with details in
#182 (comment)
where an IPC client rapidly connecting and disconnecting to the server in a
loop could cause problems.

One problem this uncovered was hangs on shutdown fixed by the previous commit.

Another problem it uncovered is that if a disconnect happened while the IPC
server was executing an IPC call, and the onDisconnect() handling code ran
before the IPC call finished, memory corruption would occur usually leading to
a hang or crash.

This happend because the ~ProxyServerBase() destructor was written with the
assumption that it would always be called either from the ~Connection()
destructor, or before that point, so its m_context.connection pointer would
always be valid. Typically this was the case, but not if the connection was
closed during an active IPC call.

A unit test to reproduce this error is added in the subsequent commit.

The fix for the bug here is actually a code simplification.  Previously
~ProxyServerBase() destructor would append a std::function callback destroying
the ProxyServerBase::m_impl object to
m_context.connection->m_async_cleanup_fns, so the object destructor could be
called asynchronously without blocking the event loop. Now it just appends the
same callback function to m_context.loop->m_async_fns without going through the
Connection object.

The new code is an improvement over the previous code in two other ways:

- It is a code simplification since previous code was needlessly complicated.

- In the case where there is no disconnect, and the remote ProxyClient is
  destroyed, and no "destroy" method is declared, this change causes the
  ProxyServer::m_impl object to be freed shortly after the client is freed,
  instead of being delayed until the connection is closed. (If a "destroy"
  method is declared, this change has no effect because in that case destroying
  the ProxyClient calls destroy and destroys ProxyServer::m_impl synchronously.)

The previous code was trying to make the ProxyServer cleanup with
Connection::m_async_cleanup_fns mirror the ProxyClient cleanup with
Connection::m_sync_cleanup_fns, but it was just fragile and unncecessarily
complicated.

Some comments about ProxyClient cleanup code are updated for clarity here but
no changes are made.

Author: ryanofsky

include/mp/proxy-io.h

@@ -178,6 +178,10 @@ class EventLoop
         post(std::forward<Callable>(callable));
     }
 
+    //! Register cleanup function to run on asynchronous worker thread without
+    //! blocking the event loop thread.
+    void addAsyncCleanup(std::function<void()> fn);
+
     //! Start asynchronous worker thread if necessary. This is only done if
     //! there are ProxyServerBase::m_impl objects that need to be destroyed
     //! asynchronously, without tying up the event loop thread. This can happen
@@ -329,10 +333,6 @@ class Connection
     CleanupIt addSyncCleanup(std::function<void()> fn);
     void removeSyncCleanup(CleanupIt it);
 
-    //! Register asynchronous cleanup function to run on worker thread when
-    //! disconnect() is called.
-    void addAsyncCleanup(std::function<void()> fn);
-
     //! Add disconnect handler.
     template <typename F>
     void onDisconnect(F&& f)
@@ -361,11 +361,10 @@ class Connection
     //! ThreadMap.makeThread) used to service requests to clients.
     ::capnp::CapabilityServerSet<Thread> m_threads;
 
-    //! Cleanup functions to run if connection is broken unexpectedly.
-    //! Lists will be empty if all ProxyClient and ProxyServer objects are
-    //! destroyed cleanly before the connection is destroyed.
+    //! Cleanup functions to run if connection is broken unexpectedly.  List
+    //! will be empty if all ProxyClient are destroyed cleanly before the
+    //! connection is destroyed.
     CleanupList m_sync_cleanup_fns;
-    CleanupList m_async_cleanup_fns;
 };
 
 //! Vat id for server side of connection. Required argument to RpcSystem::bootStrap()
@@ -454,6 +453,16 @@ ProxyServerBase<Interface, Impl>::ProxyServerBase(std::shared_ptr<Impl> impl, Co
 
 //! ProxyServer destructor, called from the EventLoop thread by Cap'n Proto
 //! garbage collection code after there are no more references to this object.
+//! This will typically happen when the corresponding ProxyClient object on the
+//! other side of the connection is destroyed. It can also happen earlier if the
+//! connection is broken or destroyed. In the latter case this destructor will
+//! typically be called inside m_rpc_system.reset() call in the ~Connection
+//! destructor while the Connection object still exists. However, because
+//! ProxyServer objects are refcounted, and the Connection object could be
+//! destroyed while asynchronous IPC calls are still in-flight, it's possible
+//! for this destructor to be called after the Connection object no longer
+//! exists, so it is NOT valid to dereference the m_context.connection pointer
+//! from this function.
 template <typename Interface, typename Impl>
 ProxyServerBase<Interface, Impl>::~ProxyServerBase()
 {
@@ -477,7 +486,7 @@ ProxyServerBase<Interface, Impl>::~ProxyServerBase()
         // connection is broken). Probably some refactoring of the destructor
         // and invokeDestroy function is possible to make this cleaner and more
         // consistent.
-        m_context.connection->addAsyncCleanup([impl=std::move(m_impl), fns=std::move(m_context.cleanup_fns)]() mutable {
+        m_context.loop->addAsyncCleanup([impl=std::move(m_impl), fns=std::move(m_context.cleanup_fns)]() mutable {
             impl.reset();
             CleanupRun(fns);
         });

src/mp/proxy.cpp

@@ -85,10 +85,13 @@ ProxyContext::ProxyContext(Connection* connection) : connection(connection), loo
 
 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.
+    // Shut down RPC system first, since this will garbage collect any
+    // ProxyServer objects that were not freed before the connection was closed.
+    // Typically all ProxyServer objects associated with this connection will be
+    // freed before this call returns. However that will not be the case if
+    // there are asynchronous IPC calls over this connection still currently
+    // executing. In that case, Cap'n Proto will destroy the ProxyServer objects
+    // after the calls finish.
     m_rpc_system.reset();
 
     // ProxyClient cleanup handlers are in sync list, and ProxyServer cleanup
@@ -137,13 +140,6 @@ Connection::~Connection()
         m_sync_cleanup_fns.front()();
         m_sync_cleanup_fns.pop_front();
     }
-    while (!m_async_cleanup_fns.empty()) {
-        const Lock lock(m_loop->m_mutex);
-        m_loop->m_async_fns->emplace_back(std::move(m_async_cleanup_fns.front()));
-        m_async_cleanup_fns.pop_front();
-    }
-    Lock lock(m_loop->m_mutex);
-    m_loop->startAsyncThread();
 }
 
 CleanupIt Connection::addSyncCleanup(std::function<void()> fn)
@@ -166,9 +162,9 @@ void Connection::removeSyncCleanup(CleanupIt it)
     m_sync_cleanup_fns.erase(it);
 }
 
-void Connection::addAsyncCleanup(std::function<void()> fn)
+void EventLoop::addAsyncCleanup(std::function<void()> fn)
 {
-    const Lock lock(m_loop->m_mutex);
+    const Lock lock(m_mutex);
     // Add async cleanup callbacks to the back of the list. Unlike the sync
     // cleanup list, this list order is more significant because it determines
     // the order server objects are destroyed when there is a sudden disconnect,
@@ -185,7 +181,8 @@ void Connection::addAsyncCleanup(std::function<void()> fn)
     // process, otherwise shared pointer counts of the CWallet objects (which
     // inherit from Chain::Notification) will not be 1 when WalletLoader
     // destructor runs and it will wait forever for them to be released.
-    m_async_cleanup_fns.emplace(m_async_cleanup_fns.end(), std::move(fn));
+    m_async_fns->emplace_back(std::move(fn));
+    startAsyncThread();
 }
 
 EventLoop::EventLoop(const char* exe_name, LogFn log_fn, void* context)