diff --git a/src/Watcher.zig b/src/Watcher.zig index fe5f967b3acb..5bf51593ebc9 100644 --- a/src/Watcher.zig +++ b/src/Watcher.zig @@ -125,12 +125,24 @@ pub fn deinit(this: *Watcher, close_descriptors: bool) void { fd.close(); } } + this.freeOwnedFilePaths(); this.watchlist.deinit(this.allocator); const allocator = this.allocator; allocator.destroy(this); } } +fn freeOwnedFilePaths(this: *Watcher) void { + const slice = this.watchlist.slice(); + const file_paths = slice.items(.file_path); + const owns = slice.items(.owns_file_path); + for (file_paths, owns) |fp, owned| { + if (owned and fp.len > 0) { + this.allocator.free(@constCast(fp.ptr[0 .. fp.len + 1])); + } + } +} + pub fn getHash(filepath: string) HashType { return @as(HashType, @truncate(bun.hash(filepath))); } @@ -218,6 +230,7 @@ pub const WatchItem = struct { kind: Kind, package_json: ?*PackageJSON, eventlist_index: if (Environment.isLinux) Platform.EventListIndex else u0 = 0, + owns_file_path: bool = false, pub const Kind = enum { file, directory }; }; @@ -248,6 +261,8 @@ fn threadMain(this: *Watcher) !void { fd.close(); } } + // Free cloned file_path strings before freeing the backing storage. + this.freeOwnedFilePaths(); this.watchlist.deinit(this.allocator); // Close trace file if open @@ -291,8 +306,16 @@ pub fn flushEvictions(this: *Watcher) void { last_item = no_watch_item; // This is split into two passes because reading the slice while modified is potentially unsafe. + const file_paths = slice.items(.file_path); + const owns = slice.items(.owns_file_path); for (this.evict_list[0..this.evict_list_i]) |item| { if (item == last_item or this.watchlist.len <= item) continue; + // Free cloned file_path strings before swapRemove overwrites the slot. + // The string was allocated via allocator.dupeZ (len+1 bytes with null terminator). + if (owns[item]) { + const fp = file_paths[item]; + this.allocator.free(@constCast(fp.ptr[0 .. fp.len + 1])); + } this.watchlist.swapRemove(item); last_item = item; } @@ -376,6 +399,10 @@ fn appendFileAssumeCapacity( bun.asByteSlice(bun.handleOom(this.allocator.dupeZ(u8, file_path))) else file_path; + var should_free_file_path = comptime clone_file_path; + defer if (should_free_file_path) { + this.allocator.free(@constCast(file_path_.ptr[0 .. file_path_.len + 1])); + }; var item = WatchItem{ .file_path = file_path_, @@ -386,6 +413,7 @@ fn appendFileAssumeCapacity( .parent_hash = parent_hash, .package_json = package_json, .kind = .file, + .owns_file_path = clone_file_path, }; if (comptime Environment.isMac) { @@ -404,6 +432,7 @@ fn appendFileAssumeCapacity( } this.watchlist.appendAssumeCapacity(item); + should_free_file_path = false; // ownership transferred to watchlist return .success; } fn appendDirectoryAssumeCapacity( @@ -434,6 +463,10 @@ fn appendDirectoryAssumeCapacity( bun.asByteSlice(bun.handleOom(this.allocator.dupeZ(u8, file_path))) else file_path; + var should_free_file_path = comptime clone_file_path; + defer if (should_free_file_path) { + this.allocator.free(@constCast(file_path_.ptr[0 .. file_path_.len + 1])); + }; const parent_hash = getHash(bun.fs.PathName.init(file_path_).dirWithTrailingSlash()); @@ -448,6 +481,7 @@ fn appendDirectoryAssumeCapacity( .parent_hash = parent_hash, .kind = .directory, .package_json = null, + .owns_file_path = clone_file_path, }; if (Environment.isMac) { @@ -506,6 +540,7 @@ fn appendDirectoryAssumeCapacity( } this.watchlist.appendAssumeCapacity(item); + should_free_file_path = false; // ownership transferred to watchlist return .{ .result = @as(WatchItemIndex, @truncate(this.watchlist.len - 1)), }; diff --git a/src/bun.js/node/path_watcher.zig b/src/bun.js/node/path_watcher.zig index 4047f1767715..dffe54f96446 100644 --- a/src/bun.js/node/path_watcher.zig +++ b/src/bun.js/node/path_watcher.zig @@ -14,7 +14,6 @@ pub const PathWatcherManager = struct { deinit_on_last_watcher: bool = false, pending_tasks: u32 = 0, deinit_on_last_task: bool = false, - has_pending_tasks: std.atomic.Value(bool) = std.atomic.Value(bool).init(false), mutex: Mutex, const PathInfo = struct { fd: FD = .invalid, @@ -30,21 +29,20 @@ pub const PathWatcherManager = struct { defer this.mutex.unlock(); if (this.deinit_on_last_task) return false; this.pending_tasks += 1; - this.has_pending_tasks.store(true, .release); 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) { - this.has_pending_tasks.store(false, .release); - this.deinit(); + if (this.pending_tasks == 0 and this.deinit_on_last_task) { + should_deinit = true; } } @@ -160,6 +158,17 @@ pub const PathWatcherManager = struct { for (events) |event| { if (event.index >= file_paths.len) continue; + + // Skip entries pending eviction — these watches have been logically + // removed, so processing events for them could trigger callbacks + // for paths the user has stopped watching. + const dominated = std.mem.indexOfScalar( + Watcher.WatchItemIndex, + ctx.evict_list[0..ctx.evict_list_i], + event.index, + ) != null; + if (dominated) continue; + const file_path = file_paths[event.index]; const update_count = counts[event.index] + 1; counts[event.index] = update_count; @@ -313,8 +322,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 +463,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 = .{ @@ -470,7 +489,7 @@ pub const PathWatcherManager = struct { options.Loader.file, .invalid, null, - false, + true, )) { .err => |err| return .{ .err = err }, .result => {}, @@ -518,7 +537,7 @@ pub const PathWatcherManager = struct { // this should only be called if thread pool is not null fn _addDirectory(this: *PathWatcherManager, watcher: *PathWatcher, path: PathInfo) bun.sys.Maybe(void) { const fd = path.fd; - switch (this.main_watcher.addDirectory(fd, path.path, path.hash, false)) { + switch (this.main_watcher.addDirectory(fd, path.path, path.hash, true)) { .err => |err| return .{ .err = err.withPath(path.path) }, .result => {}, } @@ -561,7 +580,7 @@ pub const PathWatcherManager = struct { const path = watcher.path; if (path.is_file) { - try this.main_watcher.addFile(path.fd, path.path, path.hash, .file, .invalid, null, false).unwrap(); + try this.main_watcher.addFile(path.fd, path.path, path.hash, .file, .invalid, null, true).unwrap(); } else { if (comptime Environment.isMac) { if (watcher.fsevents_watcher != null) { @@ -604,17 +623,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| { @@ -626,18 +650,27 @@ pub const PathWatcherManager = struct { } this.watcher_count -= 1; - this._decrementPathRefNoLock(watcher.path.path); - if (comptime Environment.isMac) { - if (watcher.fsevents_watcher != null) { - break; + should_deinit = this.deinit_on_last_watcher and this.watcher_count == 0; + + // When this is the last watcher triggering deinit, skip + // freeing paths here. deinit() will stop the watcher thread + // first (setting running=false), then free ALL paths. Freeing + // paths here while the thread is still running could cause it + // to read freed PathWatcherManager state during onFileUpdate. + if (!should_deinit) { + this._decrementPathRefNoLock(watcher.path.path); + if (comptime Environment.isMac) { + if (watcher.fsevents_watcher != null) { + break; + } } - } - { - watcher.mutex.lock(); - defer watcher.mutex.unlock(); - while (watcher.file_paths.pop()) |file_path| { - this._decrementPathRefNoLock(file_path); + { + watcher.mutex.lock(); + defer watcher.mutex.unlock(); + while (watcher.file_paths.pop()) |file_path| { + this._decrementPathRefNoLock(file_path); + } } } break; @@ -648,29 +681,47 @@ pub const PathWatcherManager = struct { 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; + { + default_manager_mutex.lock(); + defer default_manager_mutex.unlock(); + if (default_manager == this) { + default_manager = null; + } } - 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; + } } + // deinit(false) sets running=false under main_watcher.mutex. + // The watcher thread checks running inside processINotifyEventBatch / + // processKEvent under the same mutex, so after this returns the thread + // won't START a new onFileUpdate call. this.main_watcher.deinit(false); + // The thread reads file_paths only inside onFileUpdate, which holds + // this.mutex (PathWatcherManager's mutex). Acquire it to wait for any + // in-progress onFileUpdate to finish before freeing paths below. + // We use our OWN mutex rather than main_watcher.mutex because the + // thread may call allocator.destroy() on the Watcher after seeing + // running=false, making the Watcher's mutex inaccessible. + this.mutex.lock(); + if (this.watcher_count > 0) { while (this.watchers.pop()) |watcher| { if (watcher) |w| { @@ -691,6 +742,9 @@ pub const PathWatcherManager = struct { this.file_paths.deinit(); this.watchers.deinit(bun.default_allocator); this.current_fd_task.deinit(); + + // Release our own mutex before destroying ourselves. + this.mutex.unlock(); bun.default_allocator.destroy(this); } }; @@ -712,7 +766,6 @@ pub const PathWatcher = struct { pending_directories: u32 = 0, // only used on macOS 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), pub const ChangeEvent = struct { hash: Watcher.HashType = 0, @@ -805,31 +858,26 @@ pub const PathWatcher = struct { defer this.mutex.unlock(); if (this.isClosed()) return false; this.pending_directories += 1; - this.has_pending_directories.store(true, .release); 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) { - this.has_pending_directories.store(false, .release); - this.deinit(); + if (this.pending_directories == 0 and this.isClosed()) { + should_deinit = true; } } @@ -874,10 +922,19 @@ 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; + } } if (this.manager) |manager| { diff --git a/test/js/node/watch/fs.watch.concurrency.test.ts b/test/js/node/watch/fs.watch.concurrency.test.ts new file mode 100644 index 000000000000..0f46de853b60 --- /dev/null +++ b/test/js/node/watch/fs.watch.concurrency.test.ts @@ -0,0 +1,96 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; + +// Regression test for PathWatcherManager deadlock and UAF bugs: +// - Self-deadlock in unregisterWatcher (deinit() called while holding mutex) +// - UAF in deferred deinit (destroy while mutex held) +// - AB/BA deadlock between watcher.mutex and manager.mutex +// - Race in pending_tasks/deinit_on_last_task +// - Race in PathWatcher.deinit (setClosed + hasPendingDirectories not atomic) +// +// Strategy: Create recursive watchers (which spawn directory-scanning thread +// pool tasks), then close them while those tasks are in-flight. Mixed timing +// of creation, file mutation, and closure maximizes race window coverage. +// The test spawns child processes; if one deadlocks or crashes, the test fails. + +test("concurrent recursive fs.watch create/destroy does not deadlock or crash", async () => { + const RUNS = 10; + for (let run = 0; run < RUNS; run++) { + const script = ` +const fs = require("fs"); +const path = require("path"); +const os = require("os"); + +const base = fs.mkdtempSync(path.join(os.tmpdir(), "bun-watch-stress-")); + +// Create directory trees with enough depth for directory scanning work +for (let i = 0; i < 4; i++) { + const sub = path.join(base, "sub" + i, "a", "b"); + fs.mkdirSync(sub, { recursive: true }); + fs.writeFileSync(path.join(sub, "f.txt"), "x"); + fs.writeFileSync(path.join(base, "sub" + i, "f.txt"), "x"); + fs.writeFileSync(path.join(base, "sub" + i, "a", "f.txt"), "x"); +} + +let cycle = 0; +const CYCLES = 60; + +function tick() { + const watchers = []; + const idx = cycle % 4; + + // Create recursive watchers on different directories each cycle + try { watchers.push(fs.watch(path.join(base, "sub" + idx), { recursive: true }, () => {})); } catch(e) {} + try { watchers.push(fs.watch(path.join(base, "sub" + ((idx+1)%4)), { recursive: true }, () => {})); } catch(e) {} + // Non-recursive watcher sharing same PathWatcherManager + try { watchers.push(fs.watch(base, { recursive: false }, () => {})); } catch(e) {} + + // Mutate file while scanning tasks are in-flight + try { fs.writeFileSync(path.join(base, "sub" + idx, "a", "b", "f.txt"), "" + cycle); } catch(e) {} + + // Close with mixed timing to maximize contention + for (let i = 0; i < watchers.length; i++) { + const w = watchers[i]; + if (i % 3 === 0) { try { w.close(); } catch(e) {} } + else if (i % 3 === 1) { Promise.resolve().then(() => { try { w.close(); } catch(e) {} }); } + else { setTimeout(() => { try { w.close(); } catch(e) {} }, 0); } + } + + cycle++; + if (cycle < CYCLES) { + if (cycle % 2 === 0) queueMicrotask(tick); + else setTimeout(tick, 0); + } else { + setTimeout(() => { + try { fs.rmSync(base, { recursive: true, force: true }); } catch(e) {} + process.exit(0); + }, 200); + } +} +tick(); +`; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", script], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const timeout = 30_000; + const result = await Promise.race([ + proc.exited.then(code => ({ kind: "exit" as const, code })), + new Promise<{ kind: "timeout" }>(resolve => setTimeout(() => resolve({ kind: "timeout" }), timeout)), + ]); + + if (result.kind === "timeout") { + proc.kill(); + await proc.exited; + expect().fail(`Process deadlocked on run ${run + 1}/${RUNS} (did not exit within ${timeout}ms).`); + } + + // Exit code 0 is the proof: deadlocks cause timeout (caught above), + // and crashes produce non-zero exit codes. + expect(result).toEqual({ kind: "exit", code: 0 }); + } +}, 360_000);