Fix a race between a thread exiting after making IPC calls and a
connection being destroyed by its onDisconnect handler on the event loop
thread. The race was between ~ThreadContext destroying the thread-local
request_threads/callback_threads maps with no locking, and the SetThread
cleanup function (run by Connection::disconnect) erasing entries from
those maps on the event loop thread. When the two ran concurrently, both
could destroy the same ProxyClient<Thread> object: the SetThread cleanup
reset m_disconnect_cb just before ~ProxyClient<Thread> checked it
unsynchronized, so the exiting thread proceeded to destroy the object
while the event loop's map erase destroyed it too. The doubled
destruction consumed m_context.cleanup_fns on one thread, so the other
never unregistered the ProxyClientBase disconnect callback, and
Connection::disconnect then invoked that callback on the freed map node
(heap-use-after-free reading m_client, followed by a double free of the
node reported by glibc as "double free or corruption").
Fix by making map entry removal the synchronization point deciding which
side destroys each ProxyClient<Thread>:
- Add an explicit ~ThreadContext that removes map entries one at a time
under Waiter::m_mutex and destroys each removed node after releasing
the mutex (so ~ProxyClient<Thread> can lock EventLoop::m_mutex without
violating lock order), instead of destroying the maps unlocked.
- Change the SetThread cleanup function to look its entry up by
connection key under Waiter::m_mutex instead of dereferencing the
captured map iterator, extract it, and destroy the node outside the
lock, following the same pattern PassField already uses for mp.Context
arguments. If the entry is gone, the owning thread extracted it first
and is responsible for destroying it.
- Guard the removeSyncCleanup call in ~ProxyClient<Thread> with a
m_context.connection check, because when the entry was extracted by
~ThreadContext first, a concurrent disconnect still runs both the
SetThread cleanup (a no-op now) and the ProxyClientBase disconnect
callback, leaving m_disconnect_cb set but pointing at a spliced-out
list iterator that must not be passed to removeSyncCleanup. The
disconnect callback nulls m_context.connection, and posted functions
cannot interleave with Connection::disconnect on the event loop
thread, so a null connection reliably indicates this case.
The race is long-standing and reachable on master via connections
created by ConnectStream, whose onDisconnect handler deletes the client
Connection on the event loop thread when the peer disconnects while an
exiting thread may be running ~ThreadContext. It was exposed by the
"Waiting for in-flight server call to finish after disconnect" test
because commit bb47369f202b62b8b64f5a52984ff2c40d64ecdd ("Fix error
handling when creating clients") extended the delete-on-disconnect
handler to every ProxyClient created with destroy_connection=true,
including the test setup's directly-created client connection: the
server-side disconnect in the test then deleted the client Connection on
the event loop thread exactly while the test's call thread was exiting.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
