Bug 36

Test fixture starts its thread before the members that thread writes

Status

Fixed

Symptom

Data race; worst effect seen: sanitizer report only

Where seen

Libmultiprocess tests (mptest)

Trigger

Test setup code

First reported

2025-06-26 (first report)

Fixed

2025-07-01

Fix

mp#186

Shipped to Bitcoin Core

bitcoin#32345

Summary

The mptest fixture TestSetup declared its std::thread member before the members that thread writes, so the thread could start and write those members while the constructor was still default-initializing them, possibly overwriting the thread's values. ThreadSanitizer reported it in every test that used the fixture. The defect was in test code and did not affect users. Fixed in mp#186, a PR that fixed several mptest failures seen in Bitcoin Core CI, by moving the thread to the end of the member list.

What happened

New disconnect tests from mp#160 were causing segfaults and timeouts in the CI of bitcoin#32345, the pull request bringing them into Bitcoin Core (@maflcko's comment that the failures were real). To investigate, ryanofsky added a ThreadSanitizer CI job to libmultiprocess in mp#186. Running mptest under TSan reported, starting with the first test ("Call FooInterface methods"):

WARNING: ThreadSanitizer: data race
  Write of size 8 at 0x7ffcf7ba4c30 by thread T1:
  Previous write of size 8 at 0x7ffcf7ba4c30 by main thread:
  Location is stack of main thread.
SUMMARY: ThreadSanitizer: data race test/mp/test/test.cpp:71 in mp::test::TestSetup::TestSetup(bool)::{lambda()#1}::operator()() const::{lambda(mp::Connection&)#1}::operator()(mp::Connection&) const

The write at line 71 is the event loop thread's server = server_proxy;; the earlier write is the main thread applying the server{nullptr} default member initializer. No visible test failure was seen without TSan.

Root cause

TestSetup is the fixture that runs a client and server connected over IPC for most mptest tests. class TestSetup in test/mp/test/test.cpp declared std::thread thread; first, followed by server_disconnect, client_disconnect, client_promise, client and server. Members are constructed in declaration order, so the constructor's thread{[&] { ... }} initializer started the event loop thread before the other members were initialized. The thread immediately created the server Connection, whose make_client callback stored the ProxyServer pointer in server, and then set server_disconnect, client_disconnect and client_promise, while the main thread was still initializing those same members. If the main thread's initialization landed after the thread's write, the test would be left with a null server pointer or empty disconnect callback. The fix's commit message says the wrong order "could lead to bugs".

The code

The test fixture TestSetup in test/mp/test/test.cpp, quoted from 0d986ff144cd^:

class TestSetup
{
public:
    std::thread thread;
    std::function<void()> server_disconnect;
    std::function<void()> client_disconnect;
    std::promise<std::unique_ptr<ProxyClient<messages::FooInterface>>> client_promise;
    std::unique_ptr<ProxyClient<messages::FooInterface>> client;
    ProxyServer<messages::FooInterface>* server{nullptr};

    TestSetup(bool client_owns_connection = true)
        : thread{[&] {
              // ...
                      server = server_proxy;
              // ...
              server_disconnect = [&] { loop.sync([&] { server_connection.reset(); }); };
              // ...
                  client_disconnect = [&] { loop.sync([&] { client_connection.reset(); }); };
              // ...
              client_promise.set_value(std::move(client_proxy));
              loop.loop();
          }}

Members are initialized in declaration order, so thread is constructed first and its lambda starts running while the main thread is still initializing the members below it. The two threads then race on the same members:

1. Main thread: constructs thread, which starts the event-loop thread.
2. Event-loop thread: sets server, server_disconnect, client_disconnect and client_promise.
3. Main thread: goes on to initialize those members, server{nullptr} included, possibly overwriting the values from step 2 or constructing an object the thread has already used.

TSan reported this as a data race on server = server_proxy;. Depending on the order, the test would see a null server, an empty disconnect callback, or a promise whose value was lost.

The fix (0d986ff144cd) declares thread last:

@@ -49,12 +49,14 @@ namespace test {
 class TestSetup
 {
 public:
-    std::thread thread;
     std::function<void()> server_disconnect;
     std::function<void()> client_disconnect;
     std::promise<std::unique_ptr<ProxyClient<messages::FooInterface>>> client_promise;
     std::unique_ptr<ProxyClient<messages::FooInterface>> client;
     ProxyServer<messages::FooInterface>* server{nullptr};
+    //! Thread variable should be after other struct members so the thread does
+    //! not start until the other members are initialized.
+    std::thread thread;

Every member the thread touches is now fully initialized before thread is constructed, and starting a std::thread synchronizes with the start of its function, so the thread's writes always come after the main thread's initialization.

Fix

The fix commit 0d986ff144cd moves std::thread thread; after the other members, with a comment that it must stay after the members the thread uses (+3/-1). @Sjors asked for the comment in review. The commit message says this change removed every TSan report from mptest except one fixed by a later commit in the same PR.

Timeline

• 2025-06-19: introduced by 5108445e5d16, which added TestSetup, merged in mp#160 ("EventLoop locking cleanups + client disconnect exception").
• 2025-06-26: found by ryanofsky with TSan.
• 2025-07-01: fix merged in mp#186.
• 2025-08-18: shipped to Bitcoin Core master in bitcoin#32345 ("ipc: Handle unclean shutdowns better", which included a subtree update).

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.