diff --git a/src/bun.js/node/path_watcher.zig b/src/bun.js/node/path_watcher.zig index b4497eff14ec..80921f4384ff 100644 --- a/src/bun.js/node/path_watcher.zig +++ b/src/bun.js/node/path_watcher.zig @@ -857,25 +857,15 @@ pub const PathWatcher = struct { return true; } - pub fn hasPendingDirectories(this: *PathWatcher) callconv(.c) bool { - return this.has_pending_directories.load(.acquire); - } - pub fn isClosed(this: *PathWatcher) bool { return this.closed.load(.acquire); } - pub fn setClosed(this: *PathWatcher) void { - this.mutex.lock(); - defer this.mutex.unlock(); - this.closed.store(true, .release); - } - pub fn unrefPendingDirectory(this: *PathWatcher) void { - // deinit() calls setClosed() which re-locks this.mutex, and may then - // proceed to destroy(this). Defer it until after unlock so we don't - // self-deadlock or unlock() a freed mutex. Zig defers fire LIFO, so - // registering this defer before the lock/unlock pair makes it fire last. + // deinit() re-locks this.mutex and may proceed to destroy(this). + // Defer it until after unlock so we don't self-deadlock or unlock() + // a freed mutex. Zig defers fire LIFO, so registering this defer + // before the lock/unlock pair makes it fire last. var should_deinit = false; defer if (should_deinit) this.deinit(); @@ -929,10 +919,32 @@ pub const PathWatcher = struct { } pub fn deinit(this: *PathWatcher) void { - this.setClosed(); - if (this.hasPendingDirectories()) { - // will be freed on last directory - return; + // Decide under the mutex whether THIS call owns teardown. Both the + // main thread (via FSWatcher.detach) and a worker thread (via + // unrefPendingDirectory's deferred deinit) can reach here for the + // same watcher. The old sequence + // setClosed(); // lock; closed=true; unlock + // if (hasPendingDirectories()) return; // lock-free atomic read + // allowed the worker's unrefPendingDirectory() to run in the gap: + // it observed closed==true, stored has_pending_directories=false, + // and scheduled its own deinit(). Both callers then saw + // has_pending_directories==false and both destroyed `this`. + // + // Merging the store and the check into one critical section closes + // the gap: once this call sets closed=true, the worker cannot have + // already cleared has_pending_directories (it needs the lock and + // closed==true to do so), so this call observes it still true and + // returns early; the worker's subsequent deinit() is the sole owner + // of teardown. For file watches (no DirectoryRegisterTask, so the + // atomic was never set) this call proceeds and destroys as before. + { + this.mutex.lock(); + defer this.mutex.unlock(); + this.closed.store(true, .release); + if (this.has_pending_directories.load(.acquire)) { + // Last unrefPendingDirectory() will re-enter deinit(). + return; + } } if (this.manager) |manager| { diff --git a/test/js/node/watch/fs.watch.close-race.test.ts b/test/js/node/watch/fs.watch.close-race.test.ts new file mode 100644 index 000000000000..ac8ca0ebb275 --- /dev/null +++ b/test/js/node/watch/fs.watch.close-race.test.ts @@ -0,0 +1,87 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, isMacOS, isWindows, tempDir } from "harness"; + +// Regression test for a PathWatcher double-free when `fs.watch(dir).close()` +// races the work-pool directory scan. +// +// On Linux/FreeBSD, watching a directory schedules a `DirectoryRegisterTask` +// on the work pool (refPendingDirectory → pending_directories = 1, +// has_pending_directories = true). When the watcher is closed: +// +// main: PathWatcher.deinit() +// setClosed() // lock; closed = true; unlock +// if (hasPendingDirectories()) // ← lock-free atomic load +// return; +// ...destroy(this) +// +// worker: unrefPendingDirectory() // lock; pending = 0; +// if (closed && pending == 0) // sees closed == true +// has_pending = false // (store) +// should_deinit = true // unlock +// → deinit() → ...destroy(this) +// +// If the worker's critical section lands between main's setClosed() unlock +// and main's hasPendingDirectories() load, main observes has_pending == false +// and *also* proceeds to destroy — two `bun.default_allocator.destroy()` on +// the same PathWatcher. In release builds this corrupts mimalloc's +// cross-thread free list; on alpine aarch64 CI it surfaced as a segfault at +// address 0x75622F706D742F (ASCII `/tmp/bu`) inside `PathWatcher.init()`'s +// next allocation. Under ASAN it reports use-after-poison on `this`. +// +// The fix merges `closed = true` and the `has_pending_directories` check +// into a single critical section so the worker cannot interleave. +// +// Windows uses win_watcher.zig and macOS directories use FSEvents; neither +// schedules a DirectoryRegisterTask, so the race does not exist there. +test.skipIf(isWindows || isMacOS)( + "close() racing DirectoryRegisterTask completion does not double-free PathWatcher", + async () => { + // One file is the sweet spot: processWatcher() has just enough work that + // close() on the main thread lands while the worker is finishing, so the + // worker observes closed == true in unrefPendingDirectory(). An empty + // directory finishes too fast (worker always wins → no race); more files + // make the worker finish after main has already returned early. + using dir = tempDir("fswatch-close-race", { "f.txt": "x" }); + + const fixture = /* js */ ` + const fs = require("fs"); + const dir = process.argv[1]; + const ITERS = 3000; + for (let i = 0; i < ITERS; i++) { + const w = fs.watch(dir, { persistent: false }, () => {}); + w.close(); + } + console.log("ok " + ITERS); + `; + + // The race is timing-dependent (~90% hit rate per run under ASAN on the + // unpatched build); run a handful of attempts so an unpatched build fails + // with overwhelming probability while a patched build stays fast. + const ATTEMPTS = 4; + const results: Array<{ attempt: number; stdout: string; stderr: string; exitCode: number }> = []; + for (let attempt = 0; attempt < ATTEMPTS; attempt++) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture, String(dir)], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // ASAN builds emit a benign "WARNING: ASAN interferes with JSC signal + // handlers..." line on startup; strip it so it doesn't fail the test. + const filteredStderr = stderr + .split("\n") + .filter(l => l && !l.startsWith("WARNING: ASAN interferes")) + .join("\n"); + results.push({ attempt, stdout: stdout.trim(), stderr: filteredStderr, exitCode }); + } + + // Every attempt must have completed the full loop cleanly. Comparing the + // whole array at once surfaces every failing attempt's stderr/exitCode + // in a single diff instead of stopping at the first. + expect(results).toEqual( + Array.from({ length: ATTEMPTS }, (_, attempt) => ({ attempt, stdout: "ok 3000", stderr: "", exitCode: 0 })), + ); + }, + 60000, +);