fix: PathWatcherManager deadlock and UAF in deferred deinit

src/bun.js/node/path_watcher.zig

@@ -34,17 +34,21 @@ pub const PathWatcherManager = struct {
         return true;
     }
 
-    fn hasPendingTasks(this: *PathWatcherManager) callconv(.c) bool {
-        return this.has_pending_tasks.load(.acquire);
-    }
-
     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) {
+            // Clear unconditionally: if tasks drain to zero before deinit() runs,
+            // gating this on deinit_on_last_task leaves the flag stale-true and
+            // deinit() keeps deferring on a count that is already zero.
             this.has_pending_tasks.store(false, .release);
-            this.deinit();
+            if (this.deinit_on_last_task) should_deinit = true;
         }
     }
 
@@ -313,8 +317,13 @@ pub const PathWatcherManager = struct {
                     watcher.flush();
                 }
             }
+        }
 
-            // we need a new manager at this point
+        // Release this.mutex before acquiring default_manager_mutex to
+        // maintain consistent lock ordering (default_manager_mutex → this.mutex).
+        // deinit() acquires default_manager_mutex first, so reversing the order
+        // here would be an AB/BA deadlock.
+        {
             default_manager_mutex.lock();
             defer default_manager_mutex.unlock();
             default_manager = null;
@@ -449,8 +458,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 +618,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 to check pending state.
+        //    os_unfair_lock is non-recursive, so calling deinit() while holding
+        //    the lock self-deadlocks in __ulock_wait2.
+        // 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.
+        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 +663,8 @@ pub const PathWatcherManager = struct {
                 }
             }
         }
+
+        should_deinit = this.deinit_on_last_watcher and this.watcher_count == 0;
     }
 
     fn deinit(this: *PathWatcherManager) void {
@@ -654,19 +675,24 @@ 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, pending_tasks, and set deferred-deinit flags
+        // under this.mutex to prevent races with unregisterWatcher and
+        // unrefPendingTask which modify these fields under the same lock.
+        {
             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) {
+                // wait last watcher to close
+                this.deinit_on_last_watcher = true;
+                return;
+            }
+
+            if (this.pending_tasks > 0) {
+                this.deinit_on_last_task = true;
+                return;
+            }
+            this.has_pending_tasks.store(false, .release);
         }
 
         this.main_watcher.deinit(false);
@@ -809,27 +835,28 @@ 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() acquires this.mutex (to set closed and check
+        // pending_directories), 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) {
+            // Clear unconditionally: if the scan drains to zero before close()
+            // runs (the common case — scan is fast, close happens later),
+            // gating this on isClosed() leaves the flag stale-true, and
+            // unregisterWatcher never runs, leaking every fd the scan opened.
             this.has_pending_directories.store(false, .release);
-            this.deinit();
+            if (this.isClosed()) should_deinit = true;
         }
     }
 
@@ -874,10 +901,20 @@ pub const PathWatcher = struct {
     }
 
     pub fn deinit(this: *PathWatcher) void {
-        this.setClosed();
-        if (this.hasPendingDirectories()) {
-            // will be freed on last directory
-            return;
+        // Combine setting closed and checking pending_directories under a
+        // single mutex hold to prevent a double-deinit race: without this,
+        // a worker thread in unrefPendingDirectory() can observe closed=true
+        // and pending_directories==0 between the store and the check,
+        // causing both threads to proceed with destroy().
+        {
+            this.mutex.lock();
+            defer this.mutex.unlock();
+            this.closed.store(true, .release);
+            if (this.pending_directories > 0) {
+                // Will be freed by the last unrefPendingDirectory call.
+                return;
+            }
+            this.has_pending_directories.store(false, .release);
         }
 
         if (this.manager) |manager| {