|
| 1 | +From 77f589bbed397a8abd736aec075bd13b2152a39a Mon Sep 17 00:00:00 2001 |
| 2 | +From: Brendan Dahl <brendan.dahl@gmail.com> |
| 3 | +Date: Fri, 29 Aug 2025 23:09:28 +0000 |
| 4 | +Subject: [PATCH] [wasmfs] Fix data race in thread initialization. |
| 5 | + |
| 6 | +The `ProxyWorker` was creating a thread in the constructor initializer list and |
| 7 | +using a captured reference to the `started` member variable. This is problematic |
| 8 | +because it is not guaranteed that the class has been fully constructed during |
| 9 | +the initializer list. |
| 10 | + |
| 11 | +What happens: |
| 12 | +1) `ProxyWorker` initializers run |
| 13 | +2) Thread starts and sets `started = true` |
| 14 | +3) `ProxyWorker` finishes construction and sets `started = false` |
| 15 | +4) `ProxyWorker` waits for `started` to be `true` |
| 16 | + |
| 17 | +Step 4 will never finish in this case since the thread already ran. |
| 18 | + |
| 19 | +To fix this we simply need to move the thread creation into the constructor body |
| 20 | +where the class is guaranteed to be fully constructed. |
| 21 | + |
| 22 | +Fixes #24370, #24676, #20650 |
| 23 | +--- |
| 24 | + system/lib/wasmfs/thread_utils.h | 52 +++++++++++++++++--------------- |
| 25 | + 1 file changed, 28 insertions(+), 24 deletions(-) |
| 26 | + |
| 27 | +diff --git a/system/lib/wasmfs/thread_utils.h b/system/lib/wasmfs/thread_utils.h |
| 28 | +index 5d12400c51e00..3a7fd1f357dcf 100644 |
| 29 | +--- a/system/lib/wasmfs/thread_utils.h |
| 30 | ++++ b/system/lib/wasmfs/thread_utils.h |
| 31 | +@@ -34,32 +34,36 @@ class ProxyWorker { |
| 32 | + public: |
| 33 | + // Spawn the worker thread. |
| 34 | + ProxyWorker() |
| 35 | +- : queue(), thread([&]() { |
| 36 | +- // Notify the caller that we have started. |
| 37 | +- { |
| 38 | +- std::unique_lock<std::mutex> lock(mutex); |
| 39 | +- started = true; |
| 40 | +- } |
| 41 | +- cond.notify_all(); |
| 42 | ++ : queue() { |
| 43 | ++ // Initialize the thread in the constructor to ensure the object has been |
| 44 | ++ // fully constructed before thread starts using the object to avoid a data |
| 45 | ++ // race. See #24370. |
| 46 | ++ thread = std::thread([&]() { |
| 47 | ++ // Notify the caller that we have started. |
| 48 | ++ { |
| 49 | ++ std::unique_lock<std::mutex> lock(mutex); |
| 50 | ++ started = true; |
| 51 | ++ } |
| 52 | ++ cond.notify_all(); |
| 53 | + |
| 54 | +- // Sometimes the main thread is spinning, waiting on a WasmFS lock held |
| 55 | +- // by a thread trying to proxy work to this dedicated worker. In that |
| 56 | +- // case, the proxying message won't be relayed by the main thread and |
| 57 | +- // the system will deadlock. This heartbeat ensures that proxying work |
| 58 | +- // eventually gets done so the thread holding the lock can make forward |
| 59 | +- // progress even if the main thread is blocked. |
| 60 | +- // |
| 61 | +- // TODO: Remove this once we can postMessage directly between workers |
| 62 | +- // without involving the main thread or once all browsers ship |
| 63 | +- // Atomics.waitAsync. |
| 64 | +- // |
| 65 | +- // Note that this requires adding _emscripten_proxy_execute_queue to |
| 66 | +- // EXPORTED_FUNCTIONS. |
| 67 | +- _wasmfs_thread_utils_heartbeat(queue.queue); |
| 68 | ++ // Sometimes the main thread is spinning, waiting on a WasmFS lock held |
| 69 | ++ // by a thread trying to proxy work to this dedicated worker. In that |
| 70 | ++ // case, the proxying message won't be relayed by the main thread and |
| 71 | ++ // the system will deadlock. This heartbeat ensures that proxying work |
| 72 | ++ // eventually gets done so the thread holding the lock can make forward |
| 73 | ++ // progress even if the main thread is blocked. |
| 74 | ++ // |
| 75 | ++ // TODO: Remove this once we can postMessage directly between workers |
| 76 | ++ // without involving the main thread or once all browsers ship |
| 77 | ++ // Atomics.waitAsync. |
| 78 | ++ // |
| 79 | ++ // Note that this requires adding _emscripten_proxy_execute_queue to |
| 80 | ++ // EXPORTED_FUNCTIONS. |
| 81 | ++ _wasmfs_thread_utils_heartbeat(queue.queue); |
| 82 | + |
| 83 | +- // Sit in the event loop performing work as it comes in. |
| 84 | +- emscripten_exit_with_live_runtime(); |
| 85 | +- }) { |
| 86 | ++ // Sit in the event loop performing work as it comes in. |
| 87 | ++ emscripten_exit_with_live_runtime(); |
| 88 | ++ }); |
| 89 | + |
| 90 | + // Make sure the thread has actually started before returning. This allows |
| 91 | + // subsequent code to assume the thread has already been spawned and not |
0 commit comments