bugfix: prevent double delete segfault in server if client disconnects during method call

Fix issue where if a client disconnects in the middle of a long running IPC
method call, the server will segfault when the method eventually returns.

This behavior was reported by tdb3 in
bitcoin/bitcoin#31003 (review)
including a stack trace showing where the crash happens in the PassField
mp.Context overload while calling request_threads.erase.

Author: ryanofsky

include/mp/proxy-types.h

@@ -152,10 +152,19 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
                     // inserted, one already existed, meaning this must be a
                     // recursive call (IPC call calling back to the caller which
                     // makes another IPC call), so avoid modifying the map.
-                    auto erase_thread{inserted ? request_thread : request_threads.end()};
-                    KJ_DEFER(if (erase_thread != request_threads.end()) {
+                    const bool erase_thread{inserted};
+                    KJ_DEFER(if (erase_thread) {
                         std::unique_lock<std::mutex> lock(thread_context.waiter->m_mutex);
-                        request_threads.erase(erase_thread);
+                        // Call erase here with a Connection* argument instead
+                        // of an iterator argument, because the `request_thread`
+                        // iterator may be invalid if the connection is closed
+                        // during this function call. More specifically, the
+                        // iterator may be invalid because SetThread adds a
+                        // cleanup callback to the Connection destructor that
+                        // erases the thread from the map, and also because the
+                        // ProxyServer<Thread> destructor calls
+                        // request_threads.clear().
+                        request_threads.erase(server.m_context.connection);
                     });
                     fn.invoke(server_context, args...);
                 }

src/mp/proxy.cpp

@@ -277,6 +277,11 @@ std::tuple<ConnThread, bool> SetThread(ConnThreads& threads, std::mutex& mutex,
         std::piecewise_construct, std::forward_as_tuple(connection),
         std::forward_as_tuple(make_thread(), connection, /* destroy_connection= */ false)).first;
     thread->second.setCleanup([&threads, &mutex, thread] {
+        // Note: it is safe to use the `thread` iterator in this cleanup
+        // function, because the iterator would only be invalid if the map entry
+        // was removed, and if the map entry is removed the ProxyClient<Thread>
+        // 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
         // try unregister this callback after connection is destroyed.