diff --git a/src/Watcher.zig b/src/Watcher.zig index fe5f967b3ac..c04a18edfb5 100644 --- a/src/Watcher.zig +++ b/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 { diff --git a/src/bun.js/node/path_watcher.zig b/src/bun.js/node/path_watcher.zig index 4047f176771..8b104a44a39 100644 --- a/src/bun.js/node/path_watcher.zig +++ b/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,31 +685,52 @@ pub const PathWatcherManager = struct { } } } + + should_deinit = this.deinit_on_last_watcher and this.watcher_count == 0; } fn deinit(this: *PathWatcherManager) void { - // enable to create a new manager - default_manager_mutex.lock(); - defer default_manager_mutex.unlock(); - if (default_manager == this) { - 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; + // Clear default_manager under the global mutex, then release it + // BEFORE joining the watcher thread. Holding default_manager_mutex + // across thread.join() deadlocks: the watcher thread's onError path + // acquires default_manager_mutex (line 343), so if we hold it while + // waiting for the thread to exit, neither side can make progress. + { + default_manager_mutex.lock(); + defer default_manager_mutex.unlock(); + if (default_manager == this) { + default_manager = null; + } } - 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 — unless we ARE the watcher thread + // (onError calls deinit() directly when watcher_count == 0, and + // joining ourselves would deadlock with EDEADLK). + if (this.main_watcher.skip_thread_cleanup) { + const watcher_tid = this.main_watcher.watchloop_handle; + if (watcher_tid == null or watcher_tid.? != std.Thread.getCurrentId()) { + this.main_watcher.thread.join(); + } + } this.main_watcher.deinit(false); if (this.watcher_count > 0) { @@ -714,6 +776,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 +889,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 +945,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; } diff --git a/test/js/node/watch/fs.watch.deadlock.test.ts b/test/js/node/watch/fs.watch.deadlock.test.ts new file mode 100644 index 00000000000..8982649e6d1 --- /dev/null +++ b/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); + }); +});