Fix PathWatcherManager deadlock and UAF in deferred deinit

src/Watcher.zig

@@ -24,6 +24,10 @@ cwd: string,
 thread: std.Thread = undefined,
 running: bool = true,
 close_descriptors: bool = false,
+/// When true, threadMain skips its cleanup (watchlist.deinit + destroy).
+/// Used when the owner (e.g. PathWatcherManager) takes responsibility for
+/// destroying the Watcher after the thread exits.
+skip_thread_cleanup: bool = false,
 
 evict_list: [max_eviction_count]WatchItemIndex = undefined,
 evict_list_i: WatchItemIndex = 0,
@@ -125,6 +129,7 @@ pub fn deinit(this: *Watcher, close_descriptors: bool) void {
                 fd.close();
             }
         }
+        WatcherTrace.deinit();
         this.watchlist.deinit(this.allocator);
         const allocator = this.allocator;
         allocator.destroy(this);
@@ -241,20 +246,25 @@ fn threadMain(this: *Watcher) !void {
         .result => {},
     }
 
-    // deinit and close descriptors if needed
-    if (this.close_descriptors) {
-        const fds = this.watchlist.items(.fd);
-        for (fds) |fd| {
-            fd.close();
+    // If the owner has taken responsibility for cleanup (e.g.
+    // PathWatcherManager after an onError), skip freeing here to
+    // avoid UAF — the Watcher is still referenced by the manager.
+    if (!this.skip_thread_cleanup) {
+        // deinit and close descriptors if needed
+        if (this.close_descriptors) {
+            const fds = this.watchlist.items(.fd);
+            for (fds) |fd| {
+                fd.close();
+            }
         }
-    }
-    this.watchlist.deinit(this.allocator);
+        this.watchlist.deinit(this.allocator);
 
-    // Close trace file if open
-    WatcherTrace.deinit();
+        // Close trace file if open
+        WatcherTrace.deinit();
 
-    const allocator = this.allocator;
-    allocator.destroy(this);
+        const allocator = this.allocator;
+        allocator.destroy(this);
+    }
 }
 
 pub fn flushEvictions(this: *Watcher) void {

src/bun.js/node/path_watcher.zig

@@ -39,12 +39,17 @@ pub const PathWatcherManager = struct {
     }
 
     fn unrefPendingTask(this: *PathWatcherManager) void {
+        // deinit() may destroy(this). Defer it until after unlock so we don't
+        // unlock() a freed mutex.
+        var should_deinit = false;
+        defer if (should_deinit) this.deinit();
+
         this.mutex.lock();
         defer this.mutex.unlock();
         this.pending_tasks -= 1;
-        if (this.deinit_on_last_task and this.pending_tasks == 0) {
+        if (this.pending_tasks == 0) {
             this.has_pending_tasks.store(false, .release);
-            this.deinit();
+            if (this.deinit_on_last_task) should_deinit = true;
         }
     }
 
@@ -299,6 +304,7 @@ pub const PathWatcherManager = struct {
         this: *PathWatcherManager,
         err: bun.sys.Error,
     ) void {
+        var needs_deinit = false;
         {
             this.mutex.lock();
             defer this.mutex.unlock();
@@ -314,14 +320,39 @@ pub const PathWatcherManager = struct {
                 }
             }
 
-            // we need a new manager at this point
+            // Tell the Watcher thread to skip its own cleanup (lines 244-257
+            // in threadMain). PathWatcherManager still holds main_watcher and
+            // uses it until deinit(), so threadMain must not free it.
+            this.main_watcher.skip_thread_cleanup = true;
+
+            // Schedule deferred cleanup: watchers will unregister after
+            // receiving their error events, triggering deinit when the
+            // last one closes. If no watchers exist, we must deinit now.
+            if (this.watcher_count == 0) {
+                needs_deinit = true;
+            } else {
+                this.deinit_on_last_watcher = true;
+            }
+        }
+
+        // Acquire default_manager_mutex AFTER releasing this.mutex to maintain
+        // consistent lock ordering (default_manager_mutex before this.mutex).
+        // deinit() acquires default_manager_mutex first; holding this.mutex
+        // while acquiring default_manager_mutex would invert that order.
+        {
             default_manager_mutex.lock();
             defer default_manager_mutex.unlock();
-            default_manager = null;
+            if (default_manager == this) {
+                default_manager = null;
+            }
         }
 
-        // deinit manager when all watchers are closed
-        this.deinit();
+        // If no watchers were registered, the deferred cleanup won't
+        // trigger. Clean up manager resources directly, but skip
+        // main_watcher.deinit() since threadMain owns that.
+        if (needs_deinit) {
+            this.deinit();
+        }
     }
 
     pub const DirectoryRegisterTask = struct {
@@ -449,8 +480,13 @@ pub const PathWatcherManager = struct {
 
                 {
                     watcher.mutex.lock();
-                    defer watcher.mutex.unlock();
-                    watcher.file_paths.append(bun.default_allocator, child_path.path) catch |err| {
+                    const append_result = watcher.file_paths.append(bun.default_allocator, child_path.path);
+                    watcher.mutex.unlock();
+                    // On error, drop the ref we took in _fdFromAbsolutePathZ. Must do
+                    // this AFTER releasing watcher.mutex: _decrementPathRef acquires
+                    // manager.mutex, and unregisterWatcher acquires manager.mutex before
+                    // watcher.mutex — inverting here would AB/BA deadlock.
+                    append_result catch |err| {
                         manager._decrementPathRef(entry_path_z);
                         return switch (err) {
                             error.OutOfMemory => .{ .err = .{
@@ -604,17 +640,22 @@ pub const PathWatcherManager = struct {
         this._decrementPathRefNoLock(file_path);
     }
 
-    // unregister is always called form main thread
+    // unregister is always called from main thread
     fn unregisterWatcher(this: *PathWatcherManager, watcher: *PathWatcher) void {
+        // Must defer deinit() to AFTER releasing this.mutex, for two reasons:
+        // 1. deinit() re-acquires this.mutex when hasPendingTasks() is true.
+        //    The mutex is non-recursive, so calling deinit() while holding
+        //    the lock self-deadlocks.
+        // 2. deinit() may destroy(this). Unlocking a freed mutex is UAF.
+        // Zig defers fire LIFO, so registering this defer before the lock/unlock
+        // pair makes it fire last (after unlock).
+        var should_deinit = false;
+        defer if (should_deinit) this.deinit();
+
         this.mutex.lock();
         defer this.mutex.unlock();
 
         var watchers = this.watchers.slice();
-        defer {
-            if (this.deinit_on_last_watcher and this.watcher_count == 0) {
-                this.deinit();
-            }
-        }
 
         for (watchers, 0..) |w, i| {
             if (w) |item| {
@@ -644,6 +685,8 @@ pub const PathWatcherManager = struct {
                 }
             }
         }
+
+        should_deinit = this.deinit_on_last_watcher and this.watcher_count == 0;
     }
 
     fn deinit(this: *PathWatcherManager) void {
@@ -654,21 +697,29 @@ pub const PathWatcherManager = struct {
             default_manager = null;
         }
 
-        // only deinit if no watchers are registered
-        if (this.watcher_count > 0) {
-            // wait last watcher to close
-            this.deinit_on_last_watcher = true;
-            return;
-        }
-
-        if (this.hasPendingTasks()) {
+        // Check watcher_count and pending_tasks under one lock to avoid TOCTOU
+        {
             this.mutex.lock();
-            defer this.mutex.unlock();
-            // deinit when all tasks are done
-            this.deinit_on_last_task = true;
-            return;
+            if (this.watcher_count > 0) {
+                this.deinit_on_last_watcher = true;
+                this.mutex.unlock();
+                return;
+            }
+            if (this.pending_tasks > 0) {
+                // deinit when all tasks are done
+                this.deinit_on_last_task = true;
+                this.mutex.unlock();
+                return;
+            }
+            this.mutex.unlock();
         }
 
+        // When skip_thread_cleanup is true (error path), threadMain is still
+        // running but will skip its own cleanup. Join to ensure it has exited
+        // before we free the Watcher.
+        if (this.main_watcher.skip_thread_cleanup) {
+            this.main_watcher.thread.join();
+        }
         this.main_watcher.deinit(false);
 
         if (this.watcher_count > 0) {
@@ -714,6 +765,9 @@ pub const PathWatcher = struct {
     resolved_path: ?string = null,
     has_pending_directories: std.atomic.Value(bool) = std.atomic.Value(bool).init(false),
     closed: std.atomic.Value(bool) = std.atomic.Value(bool).init(false),
+    /// Guards against double-deinit from the TOCTOU race between
+    /// deinit() (main thread) and unrefPendingDirectory() (work pool).
+    deinit_started: std.atomic.Value(bool) = std.atomic.Value(bool).init(false),
     pub const ChangeEvent = struct {
         hash: Watcher.HashType = 0,
         event_type: EventType = .change,
@@ -824,12 +878,18 @@ pub const PathWatcher = struct {
     }
 
     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.
+        var should_deinit = false;
+        defer if (should_deinit) this.deinit();
+
         this.mutex.lock();
         defer this.mutex.unlock();
         this.pending_directories -= 1;
-        if (this.isClosed() and this.pending_directories == 0) {
+        if (this.pending_directories == 0) {
             this.has_pending_directories.store(false, .release);
-            this.deinit();
+            if (this.isClosed()) should_deinit = true;
         }
     }
 
@@ -874,9 +934,11 @@ pub const PathWatcher = struct {
     }
 
     pub fn deinit(this: *PathWatcher) void {
+        if (this.deinit_started.swap(true, .acq_rel)) return;
         this.setClosed();
         if (this.hasPendingDirectories()) {
             // will be freed on last directory
+            this.deinit_started.store(false, .release);
             return;
         }
 

test/js/node/watch/fs.watch.deadlock.test.ts

@@ -0,0 +1,72 @@
+import { describe, expect, test } from "bun:test";
+import { bunEnv, bunExe, tempDir } from "harness";
+
+describe("fs.watch deadlock", () => {
+  test("rapid create/close of recursive watchers should not hang", async () => {
+    // Create a deep directory tree to ensure DirectoryRegisterTask runs on
+    // the work pool, increasing the chance of the close() racing with it.
+    using dir = tempDir("watch-deadlock", {
+      "a/b/c/d/e/f1.txt": "x",
+      "a/b/c/d/e/f2.txt": "x",
+      "a/b/c/d/f3.txt": "x",
+      "a/b/c/f4.txt": "x",
+      "a/b/f5.txt": "x",
+      "g/h/i/j/f6.txt": "x",
+      "g/h/i/f7.txt": "x",
+      "g/h/f8.txt": "x",
+      "k/l/m/f9.txt": "x",
+      "k/l/f10.txt": "x",
+    });
+
+    // Spawn a subprocess that rapidly creates and closes recursive watchers.
+    // Before the fix, this could deadlock when:
+    // 1. unregisterWatcher() called deinit() while holding this.mutex
+    //    (deinit re-acquires the same non-recursive mutex → self-deadlock)
+    // 2. unrefPendingTask() called deinit() while holding mutex → UAF
+    // 3. processWatcher held watcher.mutex while calling _decrementPathRef
+    //    which acquires manager.mutex → AB/BA with unregisterWatcher
+    //
+    // The subprocess has a 10s timeout: if it hangs, it's a deadlock.
+    await using proc = Bun.spawn({
+      cmd: [
+        bunExe(),
+        "-e",
+        `
+        const fs = require("fs");
+        const dir = process.argv[1];
+
+        // Timeout: if we hang for 10s, it's a deadlock
+        const timer = setTimeout(() => {
+          process.stderr.write("DEADLOCK: hung for 10 seconds\\n");
+          process.exit(1);
+        }, 10000);
+        timer.unref();
+
+        let done = 0;
+        const total = 50;
+
+        for (let i = 0; i < total; i++) {
+          // Stagger slightly to increase thread interleaving
+          const w = fs.watch(dir, { recursive: true }, () => {});
+          // Close immediately — racing with directory scanning on worker thread
+          w.close();
+          done++;
+        }
+
+        // If we reach here without deadlocking, success
+        console.log("OK " + done);
+        `,
+        String(dir),
+      ],
+      env: bunEnv,
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+    expect(stderr.replace(/^WARNING: ASAN.*\n?/gm, "")).toBe("");
+    expect(stdout).toStartWith("OK");
+    expect(exitCode).toBe(0);
+  });
+});