Fix PathWatcherManager deadlock and UAF in deferred deinit

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;
         }
     }
 
@@ -313,8 +318,13 @@ pub const PathWatcherManager = struct {
                     watcher.flush();
                 }
             }
+        }
 
-            // we need a new manager at this point
+        // 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;
@@ -449,8 +459,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 +619,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 +664,8 @@ pub const PathWatcherManager = struct {
                 }
             }
         }
+
+        should_deinit = this.deinit_on_last_watcher and this.watcher_count == 0;
     }
 
     fn deinit(this: *PathWatcherManager) void {
@@ -824,12 +846,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;
         }
     }
 

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