From 376a671c5928f8a401f5fb516f9bdc31fc8575ff Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 9 Mar 2026 20:51:58 +0000 Subject: [PATCH 01/12] fix: PathWatcherManager deadlock and UAF in deferred deinit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix multiple concurrency bugs in PathWatcherManager and PathWatcher: 1. **Deadlock in unregisterWatcher**: deinit() was called while holding this.mutex, but deinit() re-acquires the same mutex when hasPendingTasks() is true. os_unfair_lock is non-recursive, so this self-deadlocks. Fixed by deferring deinit() to after mutex unlock. 2. **UAF in deferred deinit**: deinit() may destroy(this) via bun.default_allocator.destroy(). If called while the mutex is still held, the subsequent unlock() writes to freed memory. Fixed with should_deinit pattern that defers destruction. 3. **AB/BA deadlock in directory scanning**: On append error, _decrementPathRef was called while holding watcher.mutex, but _decrementPathRef acquires manager.mutex. unregisterWatcher acquires manager.mutex then watcher.mutex — classic lock ordering inversion. Fixed by releasing watcher.mutex before the error path. 4. **Race in pending_tasks/deinit_on_last_task**: The check for hasPendingTasks() and setting deinit_on_last_task were not atomic, allowing the last task to complete between the two operations. Fixed by combining both under a single mutex hold. 5. **Race in PathWatcher.deinit**: setClosed() and hasPendingDirectories() were separate operations, allowing a double-deinit race. Fixed by combining both under a single mutex hold. Co-authored-by: chrislloyd https://claude.ai/code/session_013XzvW9VsevSPURzRLixE7G --- src/bun.js/node/path_watcher.zig | 88 ++++++++++++++++++++++++-------- 1 file changed, 68 insertions(+), 20 deletions(-) diff --git a/src/bun.js/node/path_watcher.zig b/src/bun.js/node/path_watcher.zig index 4047f1767715..4b4dc8c0d1e8 100644 --- a/src/bun.js/node/path_watcher.zig +++ b/src/bun.js/node/path_watcher.zig @@ -39,12 +39,20 @@ 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) { + // 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; } } @@ -449,8 +457,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 +617,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 at line ~670 when hasPendingTasks() is + // true. 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 +662,8 @@ pub const PathWatcherManager = struct { } } } + + should_deinit = this.deinit_on_last_watcher and this.watcher_count == 0; } fn deinit(this: *PathWatcherManager) void { @@ -661,12 +681,18 @@ pub const PathWatcherManager = struct { return; } - if (this.hasPendingTasks()) { + // Combine checking pending_tasks and setting deinit_on_last_task + // under a single mutex hold to prevent a race where the last task + // completes between the lockless hasPendingTasks() check and the + // mutex acquisition, causing neither thread to proceed with cleanup. + { this.mutex.lock(); defer this.mutex.unlock(); - // deinit when all tasks are done - this.deinit_on_last_task = 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); @@ -824,12 +850,23 @@ 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) { + // 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. deinit() + // then early-returns on hasPendingDirectories() forever, + // unregisterWatcher never runs, and every fd the scan opened leaks. this.has_pending_directories.store(false, .release); - this.deinit(); + if (this.isClosed()) should_deinit = true; } } @@ -874,10 +911,21 @@ 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 our setClosed() and + // hasPendingDirectories() calls, 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| { From 36b2f3d4f9319519b0f1d489b0a8a48efa7d8943 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 9 Mar 2026 20:58:57 +0000 Subject: [PATCH 02/12] Fix stale comment in unrefPendingDirectory deinit() no longer calls setClosed(); it acquires this.mutex directly and sets closed inline. Update the comment to reflect the current code. --- src/bun.js/node/path_watcher.zig | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/bun.js/node/path_watcher.zig b/src/bun.js/node/path_watcher.zig index 4b4dc8c0d1e8..ef1689187fda 100644 --- a/src/bun.js/node/path_watcher.zig +++ b/src/bun.js/node/path_watcher.zig @@ -850,9 +850,10 @@ 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. + // 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(); From 6843424c408f0abf1b341f57ed8818cbe99e03e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 9 Mar 2026 21:39:19 +0000 Subject: [PATCH 03/12] Fix lock ordering, dead code, and race in PathWatcherManager 1. Fix AB/BA deadlock in onError: release this.mutex before acquiring default_manager_mutex to maintain consistent lock ordering with deinit(). 2. Remove dead functions: setClosed(), hasPendingTasks(), and hasPendingDirectories() have zero callers after the prior refactor inlined their logic under mutex holds. 3. Fix race in deinit: move watcher_count check and deinit_on_last_watcher assignment inside the this.mutex hold, matching unregisterWatcher which modifies these fields under the same lock. --- src/bun.js/node/path_watcher.zig | 56 +++++++++++++------------------- 1 file changed, 22 insertions(+), 34 deletions(-) diff --git a/src/bun.js/node/path_watcher.zig b/src/bun.js/node/path_watcher.zig index ef1689187fda..b4ef79b9ee10 100644 --- a/src/bun.js/node/path_watcher.zig +++ b/src/bun.js/node/path_watcher.zig @@ -34,10 +34,6 @@ 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. @@ -321,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; @@ -620,8 +621,8 @@ pub const PathWatcherManager = struct { // 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 at line ~670 when hasPendingTasks() is - // true. os_unfair_lock is non-recursive, so calling deinit() while holding + // 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 @@ -674,20 +675,19 @@ 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; - } - - // Combine checking pending_tasks and setting deinit_on_last_task - // under a single mutex hold to prevent a race where the last task - // completes between the lockless hasPendingTasks() check and the - // mutex acquisition, causing neither thread to proceed with cleanup. + // 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(); + + 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; @@ -835,20 +835,10 @@ 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). @@ -863,9 +853,8 @@ pub const PathWatcher = struct { 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. deinit() - // then early-returns on hasPendingDirectories() forever, - // unregisterWatcher never runs, and every fd the scan opened leaks. + // 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); if (this.isClosed()) should_deinit = true; } @@ -915,9 +904,8 @@ pub const PathWatcher = struct { // 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 our setClosed() and - // hasPendingDirectories() calls, causing both threads to proceed - // with destroy(). + // and pending_directories==0 between the store and the check, + // causing both threads to proceed with destroy(). { this.mutex.lock(); defer this.mutex.unlock(); From 543bd03fead6395ab83ad2c1b1cd204df9c6c0b5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 9 Mar 2026 23:53:10 +0000 Subject: [PATCH 04/12] fix: prevent use-after-poison in PathWatcher onFileUpdate and clean up dead atomics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Skip events whose watchlist index is pending eviction in onFileUpdate. _decrementPathRefNoLock frees a path string after Watcher.remove() only queues eviction — the watchlist entry (with dangling file_path pointer) persists until flushEvictions(). The File Watcher thread could then read the dangling pointer → ASAN use-after-poison. 2. Join the watcher thread in deinit() before bulk-freeing paths, so the thread cannot be mid-callback reading path strings when they are freed. 3. Remove dead has_pending_tasks / has_pending_directories atomic fields (never read after the PR removed the getter functions) and simplify the conditionals in unrefPendingTask / unrefPendingDirectory. --- src/bun.js/node/path_watcher.zig | 41 +++++++++++++++++--------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/src/bun.js/node/path_watcher.zig b/src/bun.js/node/path_watcher.zig index b4ef79b9ee10..f08f818bee93 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,7 +29,6 @@ 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; } @@ -43,12 +41,8 @@ pub const PathWatcherManager = struct { this.mutex.lock(); defer this.mutex.unlock(); this.pending_tasks -= 1; - 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); - if (this.deinit_on_last_task) should_deinit = true; + if (this.pending_tasks == 0 and this.deinit_on_last_task) { + should_deinit = true; } } @@ -164,6 +158,17 @@ pub const PathWatcherManager = struct { for (events) |event| { if (event.index >= file_paths.len) continue; + + // Skip entries pending eviction — their file_path may be a dangling + // pointer if _decrementPathRefNoLock freed the string after remove() + // queued the eviction but before flushEvictions() ran. + 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; @@ -692,10 +697,16 @@ pub const PathWatcherManager = struct { this.deinit_on_last_task = true; return; } - this.has_pending_tasks.store(false, .release); } + // Save thread handle before deinit(false) signals the thread to stop, + // because the thread calls allocator.destroy on the Watcher when it exits. + const watcher_thread = this.main_watcher.thread; this.main_watcher.deinit(false); + // Wait for the File Watcher thread to finish before freeing paths. + // Without this, the thread may still be in onFileUpdate reading + // file_paths data when we free it below → use-after-free. + watcher_thread.join(); if (this.watcher_count > 0) { while (this.watchers.pop()) |watcher| { @@ -738,7 +749,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, @@ -831,7 +841,6 @@ 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; } @@ -850,13 +859,8 @@ pub const PathWatcher = struct { this.mutex.lock(); defer this.mutex.unlock(); this.pending_directories -= 1; - 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); - if (this.isClosed()) should_deinit = true; + if (this.pending_directories == 0 and this.isClosed()) { + should_deinit = true; } } @@ -914,7 +918,6 @@ pub const PathWatcher = struct { // Will be freed by the last unrefPendingDirectory call. return; } - this.has_pending_directories.store(false, .release); } if (this.manager) |manager| { From e35d6afc9a50f9a8902335b62e6bff9ac828b170 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Mar 2026 00:17:44 +0000 Subject: [PATCH 05/12] Clone path strings in watchlist to fix use-after-free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Watcher's watchlist stored pointers to the same path strings owned by PathWatcherManager.file_paths (clone_file_path=false). When deinit freed those strings, the File Watcher thread could still read them in onFileUpdate — a use-after-free. Fix by passing clone_file_path=true to addFile/addDirectory so the watchlist owns independent copies, eliminating shared string ownership. Replace thread.join() in deinit with a mutex barrier: join() hangs when the thread is blocked on the inotify read() syscall with no events to wake it. The mutex barrier waits for any in-progress onFileUpdate (which holds PathWatcherManager.mutex) then proceeds to free paths safely. Also skip path freeing in unregisterWatcher when triggering deferred deinit, and scope default_manager_mutex to avoid holding it across the entire deinit. --- src/bun.js/node/path_watcher.zig | 69 ++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 26 deletions(-) diff --git a/src/bun.js/node/path_watcher.zig b/src/bun.js/node/path_watcher.zig index f08f818bee93..f3658f89bb7f 100644 --- a/src/bun.js/node/path_watcher.zig +++ b/src/bun.js/node/path_watcher.zig @@ -489,7 +489,7 @@ pub const PathWatcherManager = struct { options.Loader.file, .invalid, null, - false, + true, )) { .err => |err| return .{ .err = err }, .result => {}, @@ -537,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 => {}, } @@ -580,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) { @@ -650,34 +650,43 @@ 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; } } } - - 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; + { + default_manager_mutex.lock(); + defer default_manager_mutex.unlock(); + if (default_manager == this) { + default_manager = null; + } } // Check watcher_count, pending_tasks, and set deferred-deinit flags @@ -699,14 +708,19 @@ pub const PathWatcherManager = struct { } } - // Save thread handle before deinit(false) signals the thread to stop, - // because the thread calls allocator.destroy on the Watcher when it exits. - const watcher_thread = this.main_watcher.thread; + // 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); - // Wait for the File Watcher thread to finish before freeing paths. - // Without this, the thread may still be in onFileUpdate reading - // file_paths data when we free it below → use-after-free. - watcher_thread.join(); + + // 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| { @@ -728,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); } }; From 208519097fe546d1bb99b92daeecaf0eede7f391 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Mon, 9 Mar 2026 18:52:21 -0700 Subject: [PATCH 06/12] Remove unnecessary getcwd() call (#27967) ### What does this PR do? ### How did you verify your code works? --- src/bun.js/node/node_fs_watcher.zig | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/bun.js/node/node_fs_watcher.zig b/src/bun.js/node/node_fs_watcher.zig index 980b90c3c96d..938163cb23ee 100644 --- a/src/bun.js/node/node_fs_watcher.zig +++ b/src/bun.js/node/node_fs_watcher.zig @@ -630,28 +630,17 @@ pub const FSWatcher = struct { const joined_buf = bun.path_buffer_pool.get(); defer bun.path_buffer_pool.put(joined_buf); const file_path: [:0]const u8 = brk: { - const buf = bun.path_buffer_pool.get(); - defer bun.path_buffer_pool.put(buf); var slice = args.path.slice(); if (bun.strings.startsWith(slice, "file://")) { slice = slice[6..]; } - const cwd = switch (bun.sys.getcwd(buf)) { - .result => |r| r, - .err => |err| return .{ .err = err }, - }; - buf[cwd.len] = std.fs.path.sep; - - const parts = &[_]string{ - cwd, - slice, - }; + const cwd = bun.fs.FileSystem.instance.top_level_dir; break :brk Path.joinAbsStringBufZ( - buf[0 .. cwd.len + 1], + cwd, joined_buf, - parts, + &.{slice}, .auto, ); }; From 1e8167c0e5d05d82752e04445361338e9ce88c4b Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Mon, 9 Mar 2026 18:53:06 -0700 Subject: [PATCH 07/12] Fix operator precedence bug causing excessive memory allocation (#27966) ### What does this PR do? ### How did you verify your code works? --- src/js/internal/streams/native-readable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/js/internal/streams/native-readable.ts b/src/js/internal/streams/native-readable.ts index d1d341b4ad0b..e742266ac6ee 100644 --- a/src/js/internal/streams/native-readable.ts +++ b/src/js/internal/streams/native-readable.ts @@ -111,7 +111,7 @@ function ensureConstructed(this: NativeReadable, cb: null | (() => void)) { function getRemainingChunk(stream: NativeReadable, maxToRead?: number) { maxToRead ??= stream[kHighWaterMark] as number; var chunk = stream[kRemainingChunk]; - if (chunk?.byteLength ?? 0 < MIN_BUFFER_SIZE) { + if ((chunk?.byteLength ?? 0) < MIN_BUFFER_SIZE) { var size = maxToRead > MIN_BUFFER_SIZE ? maxToRead : MIN_BUFFER_SIZE; stream[kRemainingChunk] = chunk = Buffer.alloc(size); } From f52cfafe52fb6b1bb9d282d0d4f455684bc21f3c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Mar 2026 01:54:00 +0000 Subject: [PATCH 08/12] Fix memory leak from clone_file_path=true MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Track per-entry ownership of file_path strings via owns_file_path field in WatchItem. Free cloned strings in three places: 1. flushEvictions() — before swapRemove overwrites evicted slots 2. threadMain exit — before watchlist.deinit() frees backing storage 3. deinit() else branch — same, for the non-thread path This prevents leaking the strings allocated by allocator.dupeZ() when clone_file_path=true is used (PathWatcher and addFileByPathSlow). --- src/Watcher.zig | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/Watcher.zig b/src/Watcher.zig index fe5f967b3acb..dfde5ebae534 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; } @@ -386,6 +409,7 @@ fn appendFileAssumeCapacity( .parent_hash = parent_hash, .package_json = package_json, .kind = .file, + .owns_file_path = clone_file_path, }; if (comptime Environment.isMac) { @@ -448,6 +472,7 @@ fn appendDirectoryAssumeCapacity( .parent_hash = parent_hash, .kind = .directory, .package_json = null, + .owns_file_path = clone_file_path, }; if (Environment.isMac) { From f261afcf025b2d80abcf6f3428506dd764097be1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Mar 2026 02:14:10 +0000 Subject: [PATCH 09/12] Free cloned file_path on error in append{File,Directory}AssumeCapacity When clone_file_path=true and watchPath()/watchDir() returns an error, the function returns early without appending the WatchItem to the list. The cloned string would leak since freeOwnedFilePaths never sees it. Use a should_free_file_path guard: starts true when cloned, set to false after appendAssumeCapacity transfers ownership to the watchlist. On error return the defer fires and frees the orphaned allocation. --- src/Watcher.zig | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/Watcher.zig b/src/Watcher.zig index dfde5ebae534..5bf51593ebc9 100644 --- a/src/Watcher.zig +++ b/src/Watcher.zig @@ -399,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_, @@ -428,6 +432,7 @@ fn appendFileAssumeCapacity( } this.watchlist.appendAssumeCapacity(item); + should_free_file_path = false; // ownership transferred to watchlist return .success; } fn appendDirectoryAssumeCapacity( @@ -458,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()); @@ -531,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)), }; From a551e2f1987f21bb257797aa9c90bdafd51b9f4b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Mar 2026 03:00:39 +0000 Subject: [PATCH 10/12] Fix misleading comment about eviction skip in onFileUpdate The comment claimed file_path could be a dangling pointer, which was true before clone_file_path=true was introduced. Now that watchlist entries own independent copies, the real reason for the skip is to avoid processing events for logically-removed watches. --- src/bun.js/node/path_watcher.zig | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/bun.js/node/path_watcher.zig b/src/bun.js/node/path_watcher.zig index f3658f89bb7f..dffe54f96446 100644 --- a/src/bun.js/node/path_watcher.zig +++ b/src/bun.js/node/path_watcher.zig @@ -159,9 +159,9 @@ pub const PathWatcherManager = struct { for (events) |event| { if (event.index >= file_paths.len) continue; - // Skip entries pending eviction — their file_path may be a dangling - // pointer if _decrementPathRefNoLock freed the string after remove() - // queued the eviction but before flushEvictions() ran. + // 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], From ae406f7f14903cd88f8c167576730a16c1ac3d68 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 10 Mar 2026 04:18:38 +0000 Subject: [PATCH 11/12] ci: retry windows-aarch64 build From 8e82d2acec5020ca53c324a578a0f46e568a2701 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 14 Mar 2026 02:34:08 +0000 Subject: [PATCH 12/12] Add regression test for PathWatcherManager concurrency bugs Stress test that creates and destroys recursive fs.watch() watchers concurrently, exercising the deadlock and UAF code paths fixed in this PR. The test spawns child processes that rapidly create recursive watchers (triggering directory-scanning thread pool tasks), mutate files, and close watchers with mixed timing. Deadlocks are detected via a 30-second timeout; crashes produce non-zero exit codes. Verified: pre-PR debug binary (main) panics with "Deadlock detected", PR debug binary passes consistently across 10 sequential runs. https://claude.ai/code/session_01EtWDdc3kCi8Dcu8yhQqGnJ --- .../node/watch/fs.watch.concurrency.test.ts | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 test/js/node/watch/fs.watch.concurrency.test.ts 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);