diff --git a/src/runtime/node/fs_events.zig b/src/runtime/node/fs_events.zig index f8f8c13eec78..368d6816ba63 100644 --- a/src/runtime/node/fs_events.zig +++ b/src/runtime/node/fs_events.zig @@ -114,7 +114,8 @@ pub const CoreFoundation = struct { RunLoopDefaultMode: *CFStringRef, pub fn get() CoreFoundation { - if (fsevents_cf) |cf| return cf; + // No unlocked fast path — see `watch()` below for the reasoning. The + // mutex is uncontended after initialization. fsevents_mutex.lock(); defer fsevents_mutex.unlock(); if (fsevents_cf) |cf| return cf; @@ -146,7 +147,8 @@ pub const CoreServices = struct { kFSEventStreamEventIdSinceNow: FSEventStreamEventId = 18446744073709551615, pub fn get() CoreServices { - if (fsevents_cs) |cs| return cs; + // No unlocked fast path — see `watch()` below for the reasoning. The + // mutex is uncontended after initialization. fsevents_mutex.lock(); defer fsevents_mutex.unlock(); if (fsevents_cs) |cs| return cs; @@ -622,16 +624,27 @@ pub const FSEventsWatcher = struct { }; pub fn watch(path: string, recursive: bool, callback: FSEventsWatcher.Callback, updateEnd: FSEventsWatcher.UpdateEndCallback, ctx: ?*anyopaque) !*FSEventsWatcher { - if (fsevents_default_loop) |loop| { - return FSEventsWatcher.init(loop, path, recursive, callback, updateEnd, ctx); - } else { + // No unlocked fast path: `fsevents_default_loop` is a plain global and an + // unsynchronized read here would be textbook broken DCLP. `Darwin.addWatch` + // (path_watcher.zig) calls this WITHOUT holding `manager.mutex`, so two + // Workers can enter concurrently; on ARM64 Worker B could observe the + // non-null pointer before Worker A's stores inside `FSEventsLoop.init()` + // (`this.* = fs_loop`) are visible, and then `registerWatcher()` would lock + // a garbage `loop.mutex` / read a garbage `loop.watchers`. Same pattern + // `PathWatcherManager.get()` already fixed. `watch()` runs once per + // `fs.watch()` call; the mutex is uncontended after initialization. + const loop = loop: { fsevents_default_loop_mutex.lock(); defer fsevents_default_loop_mutex.unlock(); if (fsevents_default_loop == null) { fsevents_default_loop = try FSEventsLoop.init(); } - return FSEventsWatcher.init(fsevents_default_loop.?, path, recursive, callback, updateEnd, ctx); - } + break :loop fsevents_default_loop.?; + }; + // Release `fsevents_default_loop_mutex` before `registerWatcher()` (which + // takes `loop.mutex`) so we never nest the two. `loop` is stable once + // published — only `closeAndWait()` at process exit ever clears it. + return FSEventsWatcher.init(loop, path, recursive, callback, updateEnd, ctx); } pub fn closeAndWait() void { @@ -639,9 +652,10 @@ pub fn closeAndWait() void { return; } + // No unlocked fast path — see `watch()` above. + fsevents_default_loop_mutex.lock(); + defer fsevents_default_loop_mutex.unlock(); if (fsevents_default_loop) |loop| { - fsevents_default_loop_mutex.lock(); - defer fsevents_default_loop_mutex.unlock(); loop.deinit(); fsevents_default_loop = null; } diff --git a/test/js/node/watch/fs.watch.worker-init-race.test.ts b/test/js/node/watch/fs.watch.worker-init-race.test.ts new file mode 100644 index 000000000000..d21b2bef81a0 --- /dev/null +++ b/test/js/node/watch/fs.watch.worker-init-race.test.ts @@ -0,0 +1,97 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, isMacOS, tempDir } from "harness"; + +// Regression test for broken double-checked locking on `fsevents_default_loop` +// in src/runtime/node/fs_events.zig. +// +// `FSEvents.watch()` is called from `Darwin.addWatch` (path_watcher.zig) +// WITHOUT holding `manager.mutex` (it's released first to keep lock order +// one-way). Two Workers can therefore enter `FSEvents.watch()` concurrently. +// +// Before the fix the function read `fsevents_default_loop` with no lock and +// no acquire fence; only the else-branch took `fsevents_default_loop_mutex`. +// On ARM64 Worker A's store of the pointer could become visible to Worker B +// before the stores inside `FSEventsLoop.init()` (`this.* = fs_loop`), so +// Worker B would call `registerWatcher()` on a partially-visible loop and +// lock a garbage `loop.mutex` / read a garbage `loop.watchers` BabyList. +// `CoreFoundation.get()` / `CoreServices.get()` had the identical pattern. +// +// This is `path_watcher.zig`'s own `PathWatcherManager.get()` comment applied +// to `fs_events.zig`: drop the unlocked fast path; the mutex is uncontended +// after initialization. +// +// The race requires (a) the very first `fs.watch()` in the process to happen +// on two threads at once and (b) store reordering, so it is low-probability +// even on Apple Silicon. This test spawns a fresh process per iteration so +// the loop is uninitialized each time, and fires several Workers that all +// call `fs.watch()` as their first statement on distinct directories (so +// `PathWatcherManager` dedup doesn't serialize them). +// +// macOS-only: the FSEvents code path doesn't exist on other platforms. +test.skipIf(!isMacOS)( + "FSEvents: concurrent first fs.watch() from Workers does not observe a partially-initialized loop", + async () => { + const WORKERS = 8; + const files: Record = {}; + for (let i = 0; i < WORKERS; i++) files[`d${i}/f.txt`] = "x"; + files["worker.js"] = ` + const fs = require("fs"); + const { workerData } = require("worker_threads"); + // First thing this thread does: hit FSEvents.watch() via Darwin.addWatch + // with manager.mutex released. Multiple Workers race here on a fresh + // process so fsevents_default_loop starts null. + const w = fs.watch(workerData.dir, () => {}); + w.close(); + `; + files["main.js"] = ` + const path = require("path"); + const { Worker } = require("worker_threads"); + const root = process.argv[2]; + const N = ${WORKERS}; + let done = 0; + let failed = false; + for (let i = 0; i < N; i++) { + const w = new Worker(path.join(root, "worker.js"), { + workerData: { dir: path.join(root, "d" + i) }, + }); + w.on("error", err => { + failed = true; + console.error("worker error:", err); + process.exit(1); + }); + // Track completion via 'exit' (fires exactly once per worker no matter + // how it ends) so a worker that dies without posting can't hang us. + w.on("exit", code => { + if (code !== 0 && !failed) { + failed = true; + console.error("worker exited with code", code); + process.exit(1); + } + if (++done === N && !failed) { + console.log("OK"); + process.exit(0); + } + }); + } + `; + + using dir = tempDir("fsevents-worker-init-race", files); + + // Fresh process each iteration so the FSEvents loop global starts null + // and the DCLP race window exists every time. + for (let i = 0; i < 20; i++) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.js", String(dir)], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("OK"); + expect(exitCode).toBe(0); + } + }, + 60_000, +);