path: bounds-check joinZBuf; fix fs.watch overflow near PATH_MAX#29978
path: bounds-check joinZBuf; fix fs.watch overflow near PATH_MAX#29978robobun wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces many path-joining calls to a new unchecked join variant and introduces a checked Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 10:45 AM PT - May 1st, 2026
❌ @dylan-conway, your commit 4daac00 has some failures in 🧪 To try this PR locally: bunx bun-pr 29978That installs a local version of the PR into your bun-29978 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Rebased onto main after #29952 (the The rewrite fixed the original non-recursive trigger (inotify
Both confirmed panicking on Gate re-verified locally: fails on Build #49541: |
There was a problem hiding this comment.
LGTM — both earlier notes (TMPDIR-independent test bound, processWatcher twin guard) addressed in d4c8a6b.
Extended reasoning...
Overview
Two defensive length checks in src/bun.js/node/path_watcher.zig before joining a watched directory's absolute path with a child filename into a stack bun.PathBuffer: one in onFileUpdate (the original fix) and one in DirectoryRegisterTask.processWatcher (added in d4c8a6b in response to my earlier note). Plus a Linux-only subprocess regression test in test/js/node/watch/fs.watch.test.ts.
Security risks
The change is strictly safety-additive: it converts a stack buffer overflow (panic in safe builds, silent corruption in ReleaseFast) into a continue for the unrepresentable-path edge case. No new attack surface, no auth/crypto/permission code touched. The test runs in an isolated subprocess inside a temp dir.
Level of scrutiny
Low-to-moderate. The arithmetic is trivially verifiable: onFileUpdate writes dir.len + 1 + name.len bytes and the guard is len + 1 > buf.len; processWatcher accounts for the trailing nul with an extra +1. Behavior for any path that fits in 4096 bytes is unchanged. The file is also slated for a full rewrite in #29952, so this is an interim hardening rather than a design decision.
Other factors
My two prior inline comments are both resolved: the test loop bound was raised to < 4050 so abs.length ∈ [3849, 4049] regardless of TMPDIR (guaranteeing the overflow path is exercised), and the registration-time twin guard was added. CodeRabbit had no actionable comments, the bug-hunting pass on the latest revision found nothing, the one CI failure was an unrelated pre-existing http2 flake, and CODEOWNERS does not cover these paths.
The Linux fs.watch backend joins the watched directory's absolute path with child names into a bun.PathBuffer (4096 bytes) via joinZBuf / joinStringBuf, which have no bounds check. When the watched directory's absolute path is near PATH_MAX and a NAME_MAX (255) entry is involved, the normalize step writes past the end of the buffer: a safety panic in debug/ASAN, silent corruption in ReleaseFast. Guard three join sites: - walkSubtree: skip readdir entries whose absolute path won't fit. inotify_add_watch / open would return ENAMETOOLONG for such a path anyway. rel_dir is a strict suffix of abs_dir so the same bound covers the relative-path buffer. - Linux.threadMain recursive new-dir handling: skip registering a new watch on a subdirectory whose absolute path won't fit. - Linux.threadMain relative emit path: fall back to the bare filename when subpath + sep + name would overflow, rather than crashing. inotify delivers names relative to the watch descriptor, so these joined paths can exceed PATH_MAX even though every individual syscall path was valid (deep tree built via relative mkdir, entries created via relative paths from inside it).
robobun
force-pushed
the
farm/6a9d58b7/fs-watch-pathbuffer-overflow
branch
from
d4c8a6b to
21b6036
Compare
robobun
changed the title
normalizeStringNodeT on POSIX reserves buf[0] for a potential leading '/' (resolve_path.zig:1747) and writes the normalized result into buf[1..], so the effective output capacity for a relative input is path_buf.len - 1. The previous '> path_buf.len' guard let through the exact boundary case subpath.len + 1 + name.len == 4096, overflowing by one byte. Use '>=' instead. The two joinZBuf guards are unaffected: their first part is an absolute path whose leading '/' is stripped before normalize and re-prepended at buf[0], so the reserved byte cancels out.
There was a problem hiding this comment.
LGTM — the off-by-one in the relative-path guard is fixed in e9d827d, and the other two bounds checks look correct.
Extended reasoning...
Overview
This PR adds three bounds checks in src/bun.js/node/path_watcher.zig (the Linux fs.watch backend) to prevent stack buffer overflows when joining a near-PATH_MAX watched-directory path with a near-NAME_MAX child name into a fixed 4096-byte bun.PathBuffer. It also adds a Linux-only regression test in test/js/node/watch/fs.watch.test.ts that exercises all four affected code paths (walkSubtree at registration, recursive new-dir handling in the inotify reader, the relative-path emit join, and non-recursive dispatch).
The three guards are:
walkSubtree:abs_dir.len + 1 + name.len + 1 > abs_buf.len→ skip entry (sep + NUL accounted for;rel_diris a strict suffix ofabs_dirso this coversrel_buftoo).threadMainrelative emit:subpath.len + 1 + name.len >= path_buf.len→ fall back to bare filename. The>=(vs>) accounts fornormalizeStringNodeTreservingbuf[0]on POSIX for relative inputs — this was the off-by-one I flagged in my prior review and is now fixed in e9d827d.threadMainrecursive new-dir:watcher.path.len + 1 + subpath.len + 1 + name.len + 1 <= abs_buf.len→ skip registration. Conservative whensubpathis empty (joinZBuf skips empty parts), which is fine.
Security risks
The original bug was a stack buffer overflow (UB in ReleaseFast, panic in safe builds) reachable from user-controlled directory structure. The fix is purely defensive — it adds length checks that cause the code to skip or degrade rather than write past the buffer. No new attack surface is introduced; the guards strictly reduce what the code attempts. No auth, crypto, or permission logic is touched.
Level of scrutiny
Moderate. This is platform-specific I/O code in the inotify reader thread, and the bounds arithmetic is subtle enough that an earlier revision had an off-by-one. However, the change is now small (~15 lines of logic plus comments), conservative in behavior (skip / fall back to bare name rather than anything novel), and the arithmetic has been verified across three review rounds. The PR was rebased onto the #29952 rewrite and re-targeted at the surviving join sites; the rebase note and test confirm the new sites panic on main without the fix.
Other factors
All three of my prior review comments have been addressed: the test loop bound was raised to < 4050 so abs.length is guaranteed in [3849, 4049] regardless of TMPDIR (d4c8a6b); the registration-time twin in the old processWatcher became moot after #29952 removed it; and the >= off-by-one for the relative-path join landed in e9d827d with an explanatory comment. The bug-hunting system found no issues in the current revision. The regression test runs in a subprocess and asserts clean exit + empty stderr, so a future regression would surface as a test failure rather than silently passing. zig:check-all was reported green on all targets.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Let's instead refactor joinZBuf to have a bounds check with try and then make the existing callers use joinZBufWithoutBoundsCheck which calls joinZBuf catch return @Panic("Out of bounds");
joinZBuf now returns error{NameTooLong} when the concatenated input
length (an upper bound on the normalized output) would exceed buf,
instead of letting normalizeStringGenericTZ write past it. The check
accounts for the trailing nul and, on non-Windows, the byte
normalizeStringNodeT reserves at buf[0] for relative inputs.
joinZBufWithoutBoundsCheck wraps it with 'catch @Panic' for callers
that have already ensured the result fits. All existing callers are
migrated to the WithoutBoundsCheck variant (no behavior change for
them in the fits-case; a clear panic instead of UB in the overflow
case).
path_watcher.zig's walkSubtree and Linux.threadMain now use the
error-returning joinZBuf directly and skip unrepresentable entries,
replacing the previous inline length checks.
robobun
changed the title
|
Done in bfc23e0 —
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/patch.zig (1)
176-183:⚠️ Potential issue | 🔴 CriticalUse the bounded path join to prevent panic on long patch targets.
part.file_mode_change.pathis patch-controlled input and unbounded. Whenabsfilepath + filepathexceeds the PATH_MAX buffer (~4096 bytes),joinZBufWithoutBoundsCheckpanics instead of returning an error. Replace withjoinZBuf()and handle theNameTooLongerror at the same error return site (line 25).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patch.zig` around lines 176 - 183, The code uses bun.path.joinZBufWithoutBoundsCheck to build joined_absfilepath which will panic on long inputs; replace the call to bun.path.joinZBufWithoutBoundsCheck with bun.path.joinZBuf and propagate/handle its error (specifically map NameTooLong to the same error return used for state.patchDirAbsPath failures, e.g., return e.withoutPath()) so constructing joined_absfilepath returns an error instead of panicking; adjust the joined_absfilepath variable and the subsequent bun.sys.open call to use the successful result from joinZBuf.src/shell/interpreter.zig (1)
1839-1844:⚠️ Potential issue | 🔴 Critical
ShellSyscall.getPath()should use the checked join variant to handle path-too-long errors.This Windows-only helper reuses the same buffer to compute
dirpath(viagetFdPath()) and then joindirpath + toback into it. Whendirpath + toexceeds the buffer,joinZBufWithoutBoundsCheckpanics instead of returning an error. Since all callers (statat,openat) already handle the.errcase, replace this with the checkedjoinZBufand maperror.NameTooLonginto the syscall error return like the surrounding code does.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shell/interpreter.zig` around lines 1839 - 1844, In ShellSyscall.getPath(), replace the unsafe ResolvePath.joinZBufWithoutBoundsCheck call with the checked ResolvePath.joinZBuf variant and handle its error: if joinZBuf returns error.NameTooLong map and return the appropriate syscall error result (consistent with how surrounding code maps NameTooLong in statat/openat callers), otherwise propagate or return the successful joined buffer in the existing .result structure; ensure you reuse the same buffer logic but do not allow joinZBuf to panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/node/node_fs.zig`:
- Around line 4674-4677: The current code uses the unchecked
joinZBufWithoutBoundsCheck when joining root_basename and basename in
node_fs.zig (e.g., the block returning .{ .err =
err.withPath(bun.path.joinZBufWithoutBoundsCheck(...)) }), which can panic on
deep recursive basename inputs; replace these calls with the checked
bun.path.joinZBuf so buffer bounds are validated, and convert handling of
error.NameTooLong to return a normal ENAMETOOLONG/fallback error path instead of
aborting the process; apply the same change to the sync helper and the other
occurrences you noted (around the other ranges listed) so all non-root and root
error-path joins are bounds-checked and propagate ENAMETOOLONG rather than
causing a panic.
In `@src/glob/GlobWalker.zig`:
- Line 109: Replace the unchecked path join at the statpath assignment in
GlobWalker.zig (the ResolvePath.joinZBufWithoutBoundsCheck(&buf, parts, .auto)
call) with the checked join helper (the checked join variant in ResolvePath) and
propagate its error through the surrounding Maybe(bun.Stat) return; when the
join fails due to a too-long path return the NAMETOOLONG error instead of
panicking so the caller receives a normal error path rather than an abort.
In `@src/install/extract_tarball.zig`:
- Around line 506-509: The code uses joinZBufWithoutBoundsCheck to build
json_path_buf from folder_name and "/package.json", which can overflow and
panic; replace joinZBufWithoutBoundsCheck with the bounds-checked joinZBuf and
handle its error return before calling bun.sys.File.readFileFrom (the variables
involved are json_path_buf, folder_name, joinZBufWithoutBoundsCheck, and
joinZBuf); on error, propagate or handle it the same way other failures are
handled (e.g., return the error or continue gracefully) so the install does not
crash on long folder_name values.
In `@src/install/PackageInstaller.zig`:
- Around line 58-61: The change replaced safe path-joining calls with the unsafe
bun.path.joinZBufWithoutBoundsCheck, which removes the runtime reserve check and
can cause panics and bypass the NAMETOOLONG fallback in openFile; revert these
to use the safe bun.path.joinZBuf (or the original checked join helper) in
directoryExistsAtWithoutOpeningDirectories and the other occurrences (around the
block that previously used joinZBuf at lines ~75-78 and in openFile), so the
existing heuristic guards and the .NAMETOOLONG fallback remain effective and
long paths degrade to the slower fallback instead of panicking.
In `@src/install/PackageManager/patchPackage.zig`:
- Around line 165-168: Replace the unsafe bun.path.joinZBufWithoutBoundsCheck
call that constructs changes_dir in doPatchCommit with the bounds-checked
bun.path.joinZBuf, capture its error union, and propagate path length failures
instead of panicking; specifically, call bun.path.joinZBuf with pathbuf[0..],
node_modules.relative_path and name, handle error.NameTooLong (and any other
returned errors) by returning the error from doPatchCommit (since it returns
!?PatchCommitResult) so the patch operation fails cleanly rather than crashing
Bun—mirror the error-handling pattern used in src/bun.js/node/path_watcher.zig.
In `@src/install/patch_install.zig`:
- Around line 239-242: Replace the unchecked join that can panic with the
checked path-joining API and convert overflow errors into the patch error
variant NameTooLong: in the patch_install.zig code where absolute_patchfile_path
is built (and the other occurrence at lines ~410-417), call the checked join
function instead of joinZBufWithoutBoundsCheck, match the returned error and map
the NameTooLong/PathTooLong case into the patch error you use for install
failures, and in calcHash() follow the same pattern – use the checked join, map
NameTooLong to return null (rather than panicking) so callers see a handled
install failure instead of an abort.
In `@src/repl.zig`:
- Around line 188-190: The code currently calls
bun.path.joinZBufWithoutBoundsCheck which can panic on oversized HOME; change
this to the bounds-checked variant (e.g. bun.path.joinZBuf or other join that
returns an error) and only call self.allocator.dupe when the join succeeds; if
the join returns an error (path too large/unrepresentable) treat it as
non-fatal: skip setting self.file_path (leave it undefined or set to null/empty)
and return success so History.load remains best-effort. Update the lines
referencing path_buf, bun.path.joinZBufWithoutBoundsCheck, and self.file_path
accordingly.
In `@src/shell/builtin/rm.zig`:
- Around line 810-813: The bufJoin function for ShellRmTask currently calls
ResolvePath.joinZBufWithoutBoundsCheck (in both .posix and .windows branches)
which can panic on path-length overflow; change those calls to the
checked/overflow-safe join variant (the ResolvePath function that returns a
Maybe/Error instead of panicking) so bufJoin returns a recoverable .err
(NameTooLong) and let removeEntryDir handle that branch as intended. Ensure you
update both branches (the .posix and .windows invocations) to the non-panicking
join function so recursive rm -r traversal won’t abort on long paths.
---
Outside diff comments:
In `@src/patch.zig`:
- Around line 176-183: The code uses bun.path.joinZBufWithoutBoundsCheck to
build joined_absfilepath which will panic on long inputs; replace the call to
bun.path.joinZBufWithoutBoundsCheck with bun.path.joinZBuf and propagate/handle
its error (specifically map NameTooLong to the same error return used for
state.patchDirAbsPath failures, e.g., return e.withoutPath()) so constructing
joined_absfilepath returns an error instead of panicking; adjust the
joined_absfilepath variable and the subsequent bun.sys.open call to use the
successful result from joinZBuf.
In `@src/shell/interpreter.zig`:
- Around line 1839-1844: In ShellSyscall.getPath(), replace the unsafe
ResolvePath.joinZBufWithoutBoundsCheck call with the checked
ResolvePath.joinZBuf variant and handle its error: if joinZBuf returns
error.NameTooLong map and return the appropriate syscall error result
(consistent with how surrounding code maps NameTooLong in statat/openat
callers), otherwise propagate or return the successful joined buffer in the
existing .result structure; ensure you reuse the same buffer logic but do not
allow joinZBuf to panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c662f074-b763-47e7-bcc2-ef28442aa758
📒 Files selected for processing (13)
src/bun.js/node/node_fs.zigsrc/bun.js/node/path_watcher.zigsrc/glob/GlobWalker.zigsrc/install/PackageInstaller.zigsrc/install/PackageManager/patchPackage.zigsrc/install/extract_tarball.zigsrc/install/patch_install.zigsrc/patch.zigsrc/repl.zigsrc/resolver/resolve_path.zigsrc/shell/builtin/cp.zigsrc/shell/builtin/rm.zigsrc/shell/interpreter.zig
There was a problem hiding this comment.
The fs.watch fix itself looks solid and all prior feedback is addressed. bfc23e0 expands scope to make joinZBuf itself bounds-checked (returning error{NameTooLong}) and renames a dozen call sites to joinZBufWithoutBoundsCheck — that's a reasonable direction, but it's an API design call on core path infrastructure (and the conservative pre-normalization bound + per-platform reserve arithmetic has enough edge cases) that I'd like a human to sign off on it.
Extended reasoning...
Overview
This PR fixes a stack-buffer overflow in the Linux fs.watch backend where joining a near-PATH_MAX watched directory with a NAME_MAX child name into a 4096-byte bun.PathBuffer overflowed via the unchecked joinZBuf/joinStringBuf pipeline. The fix has three layers:
path_watcher.zig— three guarded join sites (walkSubtree,threadMainnew-dir handling,threadMainrelative emit). The first two now usejoinZBuf(...) catch continue/if (...) |child_abs| {...} else |_| {}; the third keeps an explicit>=length check (sincejoinStringBufis still unchecked).resolve_path.zig—joinZBufnow computes an upper bound on the concatenated input length and returnserror.NameTooLongif it would exceedbuf.lenminus a platform-dependent reserve (1 byte for nul, +1 on non-Windows for relative inputs becausenormalizeStringNodeTwrites intobuf[1..]). The old behavior is preserved asjoinZBufWithoutBoundsCheck, which catches the error and panics.- 12 call sites across
node_fs.zig,GlobWalker.zig, install/,patch.zig,repl.zig, shell/ — mechanically renamed fromjoinZBuf→joinZBufWithoutBoundsCheck.
A Linux-only regression test exercises four code paths (non-recursive file, recursive pre-existing subdir, recursive post-watch subdir, recursive file) with a directory tree whose absolute path is guaranteed in [3849, 4049] bytes regardless of TMPDIR.
Security risks
The original bug is a memory-safety issue: in ReleaseFast, the unchecked @memcpy in normalizeStringGenericTZ writes past a stack PathBuffer. Exploitation requires the victim to fs.watch a directory whose absolute path approaches PATH_MAX while an attacker controls entry names inside it — a narrow but real threat model. The fix closes this. The renamed WithoutBoundsCheck callers now panic instead of silently corrupting memory on overflow, which is strictly safer; none of them take untrusted input that wasn't already at risk.
Level of scrutiny
Higher than the original PR. Through e9d827d this was a localized 3-site guard in one Linux-only file plus a test — straightforward to verify. bfc23e0 turns it into an API change to bun.path.joinZBuf, a function used throughout the codebase. The bounds-check arithmetic (upper + reserve > buf.len with reserve = 1 for Windows/absolute, 2 for non-Windows relative) is correct for the .posix callers in path_watcher.zig, and the doc comment correctly notes the check is conservative (a path that would fit after .././// collapsing may be rejected). But the interaction with .loose/.auto callers, Windows volume-name handling in normalizeStringGenericTZ, and the conservative-rejection behavior change for the renamed callers (previously-working inputs containing .. that normalize down could now panic) are design tradeoffs a maintainer should review.
Other factors
- All three of my prior inline comments (test TMPDIR-dependence, the
processWatcherregistration-time twin, and the>=off-by-one for relativejoinStringBuf) were addressed and resolved. - Build #49569 (at e9d827d) shows failures per the bot comment; I haven't seen a build status for bfc23e0.
- coderabbit's last review was at e9d827d; bfc23e0 hasn't been reviewed by it either.
- The mechanical renames are behavior-preserving for the success path; the only behavior change is panic-instead-of-UB on overflow plus the conservative-rejection edge case.
|
|
||
| /// Like `joinZBuf` but panics instead of returning an error when the result | ||
| /// would overflow `buf`. Use this when the inputs are known to fit (e.g. a | ||
| /// bounded number of fixed-length segments) and the error path would be | ||
| /// unreachable anyway. | ||
| pub fn joinZBufWithoutBoundsCheck(buf: []u8, _parts: anytype, comptime platform: Platform) [:0]const u8 { | ||
| return joinZBuf(buf, _parts, platform) catch @panic("joinZBuf: out of bounds"); |
There was a problem hiding this comment.
🟡 Minor doc-accuracy note on "no behavior change when the result fits": that only holds when the concatenated input fits. joinStringBufT concatenates into a separately-sized temp_buf (heap-allocated when count*2 > 4096) and only writes the normalized output into buf, so pre-PR joinZBuf succeeded on ../-heavy inputs whose concatenation exceeds buf.len but whose normalized result fits — the migrated WithoutBoundsCheck callers (e.g. shell cp.zig:459, interpreter.zig:1843) now panic on those instead. Trigger is essentially fuzzer-only (needs a near-PATH_MAX cwd plus an operand with enough ../ to push the raw concat past 4096 while the resolved path stays under), and the joinZBuf doc comment already calls the bound conservative — just noting so the description reads "when the concatenated input fits".
Extended reasoning...
What this is
The new joinZBuf computes its bound from the pre-normalization concatenated input length:
var upper: usize = 0;
for (_parts) |p| { ...; upper += p.len; }
...
if (upper + reserve > buf.len) return error.NameTooLong;and joinZBufWithoutBoundsCheck turns that error into @panic("joinZBuf: out of bounds"). But the underlying joinStringBufT (resolve_path.zig:1292–1339) does not concatenate into the caller's buf: it concatenates into a thread-local parser_join_input_buffer and, when count * 2 > temp_buf.len, heap-allocates a fresh temp_buf of size count * 2. Only the normalized result is written into the caller's buf via normalizeStringNodeT(temp_buf[0..written], buf, ...), and normalizeStringGenericTZ processes .. by decrementing buf_i over the previous segment rather than write-then-erase, so the peak write index into buf equals the longest normalized prefix, not the concatenated input length.
So there exists a class of inputs — those containing redundant .././// whose concatenation exceeds buf.len but whose normalized form fits — that the pre-PR unchecked joinZBuf handled successfully, and the post-PR joinZBufWithoutBoundsCheck panics on. The PR description's "no behavior change when the result fits" is therefore precise only if "result" means "concatenated input", not "normalized output".
Step-by-step example
Take cp.zig:459: ResolvePath.joinZBufWithoutBoundsCheck(buf2[0..bun.MAX_PATH_BYTES], &.{cwd_path, this.tgt}, .auto) with a 4096-byte buf2.
- Suppose
cwd_pathis a 3900-byte absolute path (the shell cancdinto a deep tree built via relativemkdir, same mechanism as the new fs.watch test). - User runs
cp src ../../../...wherethis.tgtis, say, 70../segments (210 bytes). upper = 3900 + 1 + 210 = 4111.leading_sep = true(cwd is absolute),reserve = 1.4111 + 1 = 4112 > 4096→error.NameTooLong→@panic.- Pre-PR:
joinStringBufTwrites 4111 bytes intotemp_buf(heap-allocated, since4111*2 > 4096), thennormalizeStringNodeTcollapses 70..segments against the 3900-byte cwd. The normalized path is short — well under 4096 — and is written intobuf2[1..]. The trailing nul fits. Succeeds.
So a previously-resolvable shell cp target now aborts the process. The same applies at interpreter.zig:1843 (ShellSyscall cwd-relative resolution) where the second part is a user-supplied operand.
Why this isn't a real-world concern
Of the 20 migrated call sites, almost none can carry ..: node_fs.zig joins readdir entry names (no /), GlobWalker joins getFdPath + a readdir name, PackageInstaller/patchPackage/extract_tarball/patch_install join controlled package-manager paths, and rm.zig's bufJoin is fed parent + readdir entry. Only the shell cp.zig and interpreter.zig sites take arbitrary user operands — and triggering requires the cwd to already be within a few hundred bytes of PATH_MAX and the user to type/generate an operand with dozens of ../ segments such that the concat overflows but the resolved path doesn't. That's a fuzzer scenario, not a workflow.
Addressing the "intentional behavior" objection
The joinZBuf doc comment explicitly says "a result that would fit after collapsing .././// may be conservatively rejected", so the over-rejection itself is a documented design choice (a simple, provably-sound upper bound vs. an exact bound that would require running normalization first). And on the much larger class of inputs that don't normalize down, the PR trades silent ReleaseFast UB for a clean panic — clearly the right call. The CodeRabbit threads about converting individual WithoutBoundsCheck sites to graceful NameTooLong handling were already resolved as deferred follow-up, and the eventual fix here (use the checked variant at user-input shell sites) is the same work item.
This note is therefore not asking for a code change — it's only observing that the PR description's "no behavior change when the result fits" is slightly imprecise for the migrated callers, and the WithoutBoundsCheck doc comment ("Use this when the inputs are known to fit") could mention that "fit" means the concatenation, not the normalized result. Purely a documentation/expectation nit; non-blocking.
3 participants
Repro
Debug/ASAN build:
ReleaseFast: silent buffer overflow (UB).
Cause
joinZBuf→joinStringBuf→normalizeStringNodeT→normalizeStringGenericTZwrites segments via unchecked@memcpyinto the caller's fixed buffer. The Linuxfs.watchbackend joins the watched directory's absolute path with child names into abun.PathBuffer(4096 bytes); inotify delivers names relative to the watch descriptor, so the joined absolute path can exceedPATH_MAXeven though every individual syscall path was valid (deep tree built with relativemkdir; entries created with relative paths from inside it).Fix
resolve_path.zig:joinZBufnow returnserror{NameTooLong}![:0]const u8. It computes the concatenated input length (an upper bound on the normalized output — normalization can only shrink) and returns the error when that exceedsbuf(accounting for the trailing nul and, on non-Windows, the bytenormalizeStringNodeTreserves atbuf[0]for relative inputs). AddedjoinZBufWithoutBoundsCheckwhich wraps it withcatch @panic("joinZBuf: out of bounds"). All 20 existing callers are migrated tojoinZBufWithoutBoundsCheck— no behavior change when the result fits, and a clear panic instead of UB when it doesn't.path_watcher.zig:walkSubtree:joinZBuf(...) catch continue— skip readdir entries whose absolute path is unrepresentable.rel_diris a strict suffix ofabs_dir, so therel_bufjoinStringBufis covered by the same bound.Linux.threadMainrecursive new-dir handling:if (joinZBuf(...)) |child_abs| { ... } else |_| {}— skip registering a watch on a subdirectory whose absolute path won't fit;inotify_add_watchwould returnENAMETOOLONGanyway.Linux.threadMainrelative emit path: fall back to the bare filename whensubpath + sep + namewould overflow (joinStringBufis still the unchecked variant here;>=not>becausenormalizeStringNodeTreservesbuf[0]for relative inputs on POSIX).Verification
New Linux-only test in
test/js/node/watch/fs.watch.test.tsexercises all four code paths (non-recursive file, recursive pre-existing subdir, recursive post-watch subdir, recursive file).walkSubtreeat registration time, test failszig:check-allgreen on all targets (linux/mac/windows/freebsd × debug/release).fs.watch.test.ts+fs.watch.rewrite.test.tsgreen (modulo the two root-only permission tests that also fail onmain). Spot-checked glob/shell/fs.cp tests for the migrated callers.Rebase note
This PR originally patched
PathWatcherManager.onFileUpdateandDirectoryRegisterTask.processWatcher, which were removed by the #29952 rewrite. The rewrite eliminated the original non-recursive trigger (it now emits the inotifynamedirectly without rebuilding the absolute path). The same bug class remained in the rewrite'swalkSubtreeandthreadMainfor recursive watches; this PR now targets those, and moves the bounds check intojoinZBufitself per review.