From c32b38739fcd76237a64ec608bdc202b3f7c1563 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 15 Jan 2025 10:53:15 -0500 Subject: [PATCH 1/2] refactor: Add CleanupRun function to dedup clean list code Also rename "cleanup" variables to distinguish between cleanup iterators and cleanup callback functions. --- include/mp/proxy-io.h | 29 +++++++++++------------------ include/mp/proxy.h | 10 ++++++++-- src/mp/proxy.cpp | 16 ++++++++-------- test/mp/test/test.cpp | 2 +- 4 files changed, 28 insertions(+), 29 deletions(-) diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h index 29cd3a45..a06133b4 100644 --- a/include/mp/proxy-io.h +++ b/include/mp/proxy-io.h @@ -63,16 +63,16 @@ struct ProxyClient : public ProxyClientBase ProxyClient(const ProxyClient&) = delete; ~ProxyClient(); - void setCleanup(std::function cleanup); + void setCleanup(std::function fn); //! Cleanup function to run when the connection is closed. If the Connection //! gets destroyed before this ProxyClient 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 m_cleanup; + std::optional m_cleanup_it; }; template <> @@ -379,7 +379,7 @@ ProxyClientBase::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::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 @@ -437,9 +437,7 @@ ProxyClientBase::ProxyClientBase(typename Interface::Client cli template ProxyClientBase::~ProxyClientBase() noexcept { - for (auto& cleanup : m_context.cleanup) { - cleanup(); - } + CleanupRun(m_context.cleanup_fns); } template @@ -476,14 +474,12 @@ ProxyServerBase::~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 lock(m_context.connection->m_loop.m_mutex); m_context.connection->m_loop.removeClient(lock); } @@ -509,10 +505,7 @@ template void ProxyServerBase::invokeDestroy() { m_impl.reset(); - for (auto& cleanup : m_context.cleanup) { - cleanup(); - } - m_context.cleanup.clear(); + CleanupRun(m_context.cleanup_fns); } using ConnThreads = std::map>; diff --git a/include/mp/proxy.h b/include/mp/proxy.h index 1ee1c753..10ab8134 100644 --- a/include/mp/proxy.h +++ b/include/mp/proxy.h @@ -39,11 +39,17 @@ struct ProxyType; using CleanupList = std::list>; 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> cleanup; + CleanupList cleanup_fns; ProxyContext(Connection* connection) : connection(connection) {} }; @@ -147,7 +153,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 diff --git a/src/mp/proxy.cpp b/src/mp/proxy.cpp index bcfbde8b..d1b8a427 100644 --- a/src/mp/proxy.cpp +++ b/src/mp/proxy.cpp @@ -283,9 +283,9 @@ std::tuple 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 lock(mutex); threads.erase(thread); @@ -295,16 +295,16 @@ std::tuple SetThread(ConnThreads& threads, std::mutex& mutex, ProxyClient::~ProxyClient() { - if (m_cleanup) { - m_context.connection->removeSyncCleanup(*m_cleanup); + if (m_cleanup_it) { + m_context.connection->removeSyncCleanup(*m_cleanup_it); } } -void ProxyClient::setCleanup(std::function cleanup) +void ProxyClient::setCleanup(std::function 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::ProxyServer(ThreadContext& thread_context, std::thread&& thread) diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp index fc74fc98..661ae693 100644 --- a/test/mp/test/test.cpp +++ b/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); } From 2f0122121f3e28f40a31ed6177e929f1cf92b334 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 15 Jan 2025 12:32:05 -0500 Subject: [PATCH 2/2] ProxyClientBase: avoid static_cast to partially destructed object This is a bugfix that should fix the UBSan failure reported https://github.com/chaincodelabs/libmultiprocess/issues/125 In practice there is no real bug here, just like there was no real bug fixed in the recent "ProxyClientBase: avoid static_cast to partially constructed object" change in https://github.com/chaincodelabs/libmultiprocess/pull/121. This is because in practice, ProxyClient subclasses only inherit ProxyClientBase state and do not define any state of their own, so there is nothing bad that could actually happen from static_cast-ing a ProxyClientBase pointer to a ProxyClient pointer inside the ProxyClientBase constructor or destructor. However, using static_cast this way technically triggers undefined behavior, and that causes UBSan to fail, so this commit moves ProxyClientBase cleanup out of the ProxyClientBase destructor, into ProxyClient subclass destructors to prevent this. An alternate fix could maybe avoid the need to do this by making ProxyClient::construct() and ProxyClient::destroy() methods into static methods taking a ProxyClientBase& self argument instead of instance methods using *this. This would avoid the need to use static_cast at all. But it would also require changes to the ProxyClient class code generator so unclear if it would be a simpler fix. Fixes #125 --- include/mp/proxy-io.h | 6 ------ include/mp/proxy-types.h | 7 ++++--- include/mp/proxy.h | 10 +++++++++- src/mp/proxy.cpp | 5 +++++ 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h index a06133b4..37656021 100644 --- a/include/mp/proxy-io.h +++ b/include/mp/proxy-io.h @@ -434,12 +434,6 @@ ProxyClientBase::ProxyClientBase(typename Interface::Client cli }); } -template -ProxyClientBase::~ProxyClientBase() noexcept -{ - CleanupRun(m_context.cleanup_fns); -} - template ProxyServerBase::ProxyServerBase(std::shared_ptr impl, Connection& connection) : m_impl(std::move(impl)), m_context(&connection) diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index 6e6d419a..9a121bb0 100644 --- a/include/mp/proxy-types.h +++ b/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 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 diff --git a/include/mp/proxy.h b/include/mp/proxy.h index 10ab8134..7855a024 100644 --- a/include/mp/proxy.h +++ b/include/mp/proxy.h @@ -56,6 +56,13 @@ struct ProxyContext //! 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 class ProxyClientBase : public Impl_ { @@ -64,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 @@ -99,6 +106,7 @@ class ProxyClientBase : public Impl_ void destroy() {} ProxyClient& self() { return static_cast&>(*this); } + void cleanup() { CleanupRun(m_context.cleanup_fns); } typename Interface::Client m_client; ProxyContext m_context; diff --git a/src/mp/proxy.cpp b/src/mp/proxy.cpp index d1b8a427..4b323c0f 100644 --- a/src/mp/proxy.cpp +++ b/src/mp/proxy.cpp @@ -295,9 +295,14 @@ std::tuple SetThread(ConnThreads& threads, std::mutex& mutex, ProxyClient::~ProxyClient() { + // 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::setCleanup(std::function fn)