fs.watch: fix PathWatcher double-free race between close() and DirectoryRegisterTask

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| {

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,
+);