test: worker thread destroyed before it is initialized

Add testing_hook_makethread that fires on the worker thread between
the Lock and set_value operations in makeThread. A checker thread uses
try_lock to verify the waiter mutex is held at that point. With the
fix (Lock before set_value) the mutex is held, so try_lock fails.
Without the fix (set_value before Lock) the hook fires before Lock, so
try_lock succeeds -- indicating the race window exists.

Author: Sjors

include/mp/proxy-io.h

@@ -461,6 +461,11 @@ class Connection
     //! ThreadMap.makeThread) used to service requests to clients.
     ::capnp::CapabilityServerSet<Thread> m_threads;
 
+    //! Hook called on the worker thread inside makeThread, right after
+    //! set_value. Used by tests to verify the waiter mutex is held when
+    //! the thread context is published.
+    std::function<void()> testing_hook_makethread;
+
     //! Canceler for canceling promises that we want to discard when the
     //! connection is destroyed. This is used to interrupt method calls that are
     //! still executing at time of disconnection.

src/mp/proxy.cpp

@@ -418,6 +418,7 @@ kj::Promise<void> ProxyServer<ThreadMap>::makeThread(MakeThreadContext context)
         g_thread_context.waiter = std::make_unique<Waiter>();
         Lock lock(g_thread_context.waiter->m_mutex);
         thread_context.set_value(&g_thread_context);
+        if (this->m_connection.testing_hook_makethread) this->m_connection.testing_hook_makethread();
         // Wait for shutdown signal from ProxyServer<Thread> destructor (signal
         // is just waiter getting set to null.)
         g_thread_context.waiter->wait(lock, [] { return !g_thread_context.waiter; });

test/mp/test/test.cpp

@@ -325,6 +325,47 @@ KJ_TEST("Calling IPC method, disconnecting and blocking during the call")
     signal.set_value();
 }
 
+KJ_TEST("Worker thread destroyed before it is initialized")
+{
+    // Regression test for bitcoin/bitcoin#34711, bitcoin/bitcoin#34756
+    // (worker thread destroyed before it acquires the waiter mutex). The
+    // fix acquires the lock before calling set_value so the
+    // ProxyServer<Thread> destructor cannot null the waiter while the
+    // worker is between set_value and Lock.
+    //
+    // The testing_hook_makethread fires right after set_value in
+    // makeThread's worker thread. A checker thread uses try_lock to
+    // verify the waiter mutex is held at that point. With the fix
+    // (Lock before set_value) the mutex is held, so try_lock fails.
+    // Without the fix (set_value before Lock) the hook fires before
+    // Lock, so try_lock succeeds — indicating the race window exists.
+    TestSetup setup;
+    ProxyClient<messages::FooInterface>* foo = setup.client.get();
+    foo->initThreadMap();
+    setup.server->m_impl->m_fn = [] {};
+
+    std::promise<std::mutex*> mutex_promise;
+    std::promise<void> check_done;
+    Connection* conn = setup.server->m_context.connection;
+    conn->testing_hook_makethread = [&] {
+        mutex_promise.set_value(&g_thread_context.waiter->m_mutex.m_mutex);
+        check_done.get_future().wait();
+    };
+
+    std::atomic<bool> lock_was_held{false};
+    std::thread check_thread{[&] {
+        std::mutex* m = mutex_promise.get_future().get();
+        bool locked = m->try_lock();
+        if (locked) m->unlock();
+        lock_was_held = !locked;
+        check_done.set_value();
+    }};
+
+    foo->callFnAsync();
+    check_thread.join();
+    KJ_EXPECT(lock_was_held);
+}
+
 KJ_TEST("Calling async IPC method, with server disconnect racing the call")
 {
     // Regression test for bitcoin/bitcoin#34777 (heap-use-after-free where