ProxyClientBase: avoid static_cast to partially destructed object

include/mp/proxy-io.h

@@ -63,16 +63,16 @@ struct ProxyClient<Thread> : public ProxyClientBase<Thread, ::capnp::Void>
     ProxyClient(const ProxyClient&) = delete;
     ~ProxyClient();
 
-    void setCleanup(std::function<void()> cleanup);
+    void setCleanup(std::function<void()> fn);
 
     //! Cleanup function to run when the connection is closed. If the Connection
     //! gets destroyed before this ProxyClient<Thread> object, this cleanup
     //! callback lets it destroy this object and remove its entry in the
     //! thread's request_threads or callback_threads map (after resetting
-    //! m_cleanup so the destructor does not try to access it). But if this
+    //! m_cleanup_it so the destructor does not try to access it). But if this
     //! object gets destroyed before the Connection, there's no need to run the
     //! cleanup function and the destructor will unregister it.
-    std::optional<CleanupIt> m_cleanup;
+    std::optional<CleanupIt> m_cleanup_it;
 };
 
 template <>
@@ -379,7 +379,7 @@ ProxyClientBase<Interface, Impl>::ProxyClientBase(typename Interface::Client cli
     }
 
     // Handler for the connection getting destroyed before this client object.
-    auto cleanup = m_context.connection->addSyncCleanup([this]() {
+    auto cleanup_it = m_context.connection->addSyncCleanup([this]() {
         // Release client capability by move-assigning to temporary.
         {
             typename Interface::Client(std::move(self().m_client));
@@ -402,11 +402,11 @@ ProxyClientBase<Interface, Impl>::ProxyClientBase(typename Interface::Client cli
     // The first case is handled here when m_context.connection is not null. The
     // second case is handled by the cleanup function, which sets m_context.connection to
     // null so nothing happens here.
-    m_context.cleanup.emplace_front([this, destroy_connection, cleanup]{
+    m_context.cleanup_fns.emplace_front([this, destroy_connection, cleanup_it]{
     if (m_context.connection) {
         // Remove cleanup callback so it doesn't run and try to access
         // this object after it's already destroyed.
-        m_context.connection->removeSyncCleanup(cleanup);
+        m_context.connection->removeSyncCleanup(cleanup_it);
 
         // If the capnp interface defines a destroy method, call it to destroy
         // the remote object, waiting for it to be deleted server side. If the
@@ -434,14 +434,6 @@ ProxyClientBase<Interface, Impl>::ProxyClientBase(typename Interface::Client cli
     });
 }
 
-template <typename Interface, typename Impl>
-ProxyClientBase<Interface, Impl>::~ProxyClientBase() noexcept
-{
-    for (auto& cleanup : m_context.cleanup) {
-        cleanup();
-    }
-}
-
 template <typename Interface, typename Impl>
 ProxyServerBase<Interface, Impl>::ProxyServerBase(std::shared_ptr<Impl> impl, Connection& connection)
     : m_impl(std::move(impl)), m_context(&connection)
@@ -476,14 +468,12 @@ 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), c=std::move(m_context.cleanup)]() mutable {
+        m_context.connection->addAsyncCleanup([impl=std::move(m_impl), fns=std::move(m_context.cleanup_fns)]() mutable {
             impl.reset();
-            for (auto& cleanup : c) {
-                cleanup();
-            }
+            CleanupRun(fns);
         });
     }
-    assert(m_context.cleanup.size() == 0);
+    assert(m_context.cleanup_fns.size() == 0);
     std::unique_lock<std::mutex> lock(m_context.connection->m_loop.m_mutex);
     m_context.connection->m_loop.removeClient(lock);
 }
@@ -509,10 +499,7 @@ template <typename Interface, typename Impl>
 void ProxyServerBase<Interface, Impl>::invokeDestroy()
 {
     m_impl.reset();
-    for (auto& cleanup : m_context.cleanup) {
-        cleanup();
-    }
-    m_context.cleanup.clear();
+    CleanupRun(m_context.cleanup_fns);
 }
 
 using ConnThreads = std::map<Connection*, ProxyClient<Thread>>;

include/mp/proxy-types.h

@@ -1532,17 +1532,18 @@ struct CapRequestTraits<::capnp::Request<_Params, _Results>>
     using Results = _Results;
 };
 
-//! Entry point called by all generated ProxyClient destructors. This only logs
-//! the object destruction. The actual cleanup happens in the ProxyClient base
-//! destructor.
+//! Entry point called by all generated ProxyClient destructors.
 template <typename Client>
 void clientDestroy(Client& client)
 {
+    // Log that ProxyClient object is being destroyed.
     if (client.m_context.connection) {
         client.m_context.connection->m_loop.log() << "IPC client destroy " << typeid(client).name();
     } else {
         KJ_LOG(INFO, "IPC interrupted client destroy", typeid(client).name());
     }
+    // Call ProxyClientBase cleanup.
+    client.cleanup();
 }
 
 template <typename Server>

include/mp/proxy.h

@@ -39,17 +39,30 @@ struct ProxyType;
 using CleanupList = std::list<std::function<void()>>;
 using CleanupIt = typename CleanupList::iterator;
 
+inline void CleanupRun(CleanupList& fns) {
+    for (auto& fn : fns) {
+        fn();
+    }
+}
+
 //! Context data associated with proxy client and server classes.
 struct ProxyContext
 {
     Connection* connection;
-    std::list<std::function<void()>> cleanup;
+    CleanupList cleanup_fns;
 
     ProxyContext(Connection* connection) : connection(connection) {}
 };
 
 //! Base class for generated ProxyClient classes that implement a C++ interface
 //! and forward calls to a capnp interface.
+//
+//! @note: All ProxyClient subclasses that inherit from this are responsible for
+//! calling cleanup() in their destructors. The ProxyClientBase destructor
+//! cannot call cleanup() itself, because it runs too late after the ProxyClient
+//! subclass is destroyed, when the cleanup callback that is run would trigger
+//! C++ undefined behavior by calling self() and using a pointer to a partially
+//! destroyed object.
 template <typename Interface_, typename Impl_>
 class ProxyClientBase : public Impl_
 {
@@ -58,7 +71,7 @@ class ProxyClientBase : public Impl_
     using Impl = Impl_;
 
     ProxyClientBase(typename Interface::Client client, Connection* connection, bool destroy_connection);
-    ~ProxyClientBase() noexcept;
+    ~ProxyClientBase() noexcept = default;
 
     // construct/destroy methods called during client construction/destruction
     // that can optionally be defined in capnp interfaces to invoke code on the
@@ -93,6 +106,7 @@ class ProxyClientBase : public Impl_
     void destroy() {}
 
     ProxyClient<Interface>& self() { return static_cast<ProxyClient<Interface>&>(*this); }
+    void cleanup() { CleanupRun(m_context.cleanup_fns); }
 
     typename Interface::Client m_client;
     ProxyContext m_context;
@@ -147,7 +161,7 @@ struct ProxyServerBase : public virtual Interface_::Server
 //! state can be destroyed without blocking, because ProxyServer destructors are
 //! called from the EventLoop thread, and if they block, it could deadlock the
 //! program. One way to do avoid blocking is to clean up the state by pushing
-//! cleanup callbacks to the m_context.cleanup list, which run after the server
+//! cleanup callbacks to the m_context.cleanup_fns list, which run after the server
 //! m_impl object is destroyed on the same thread destroying it (which will
 //! either be an IPC worker thread if the ProxyServer is being explicitly
 //! destroyed by a client calling a destroy() method with a Context argument and

src/mp/proxy.cpp

@@ -283,9 +283,9 @@ std::tuple<ConnThread, bool> SetThread(ConnThreads& threads, std::mutex& mutex,
         // destructor unregisters the cleanup.
 
         // Connection is being destroyed before thread client is, so reset
-        // thread client m_cleanup member so thread client destructor does not
+        // thread client m_cleanup_it member so thread client destructor does not
         // try unregister this callback after connection is destroyed.
-        thread->second.m_cleanup.reset();
+        thread->second.m_cleanup_it.reset();
         // Remove connection pointer about to be destroyed from the map
         std::unique_lock<std::mutex> lock(mutex);
         threads.erase(thread);
@@ -295,16 +295,21 @@ std::tuple<ConnThread, bool> SetThread(ConnThreads& threads, std::mutex& mutex,
 
 ProxyClient<Thread>::~ProxyClient()
 {
-    if (m_cleanup) {
-        m_context.connection->removeSyncCleanup(*m_cleanup);
+    // If thread is being destroyed before connection is destroyed, remove the
+    // cleanup callback that was registered to handle the connection being
+    // destroyed before the thread being destroyed.
+    if (m_cleanup_it) {
+        m_context.connection->removeSyncCleanup(*m_cleanup_it);
     }
+    // Call ProxyClientBase cleanup.
+    cleanup();
 }
 
-void ProxyClient<Thread>::setCleanup(std::function<void()> cleanup)
+void ProxyClient<Thread>::setCleanup(std::function<void()> fn)
 {
-    assert(cleanup);
-    assert(!m_cleanup);
-    m_cleanup = m_context.connection->addSyncCleanup(cleanup);
+    assert(fn);
+    assert(!m_cleanup_it);
+    m_cleanup_it = m_context.connection->addSyncCleanup(fn);
 }
 
 ProxyServer<Thread>::ProxyServer(ThreadContext& thread_context, std::thread&& thread)

test/mp/test/test.cpp

@@ -122,7 +122,7 @@ KJ_TEST("Call FooInterface methods")
     thread.join();
 
     bool destroyed = false;
-    foo->m_context.cleanup.emplace_front([&destroyed]{ destroyed = true; });
+    foo->m_context.cleanup_fns.emplace_front([&destroyed]{ destroyed = true; });
     foo.reset();
     KJ_EXPECT(destroyed);
 }