-
Notifications
You must be signed in to change notification settings - Fork 4.7k
path: bounds-check joinZBuf; fix fs.watch overflow near PATH_MAX #29978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
21b6036
e9d827d
bfc23e0
4daac00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1219,16 +1219,53 @@ | |
| return joinStringBuf(&join_buf, _parts, platform); | ||
| } | ||
| pub fn joinZ(_parts: anytype, comptime platform: Platform) [:0]const u8 { | ||
| return joinZBuf(&join_buf, _parts, platform); | ||
| return joinZBufWithoutBoundsCheck(&join_buf, _parts, platform); | ||
| } | ||
|
|
||
| pub fn joinZBuf(buf: []u8, _parts: anytype, comptime platform: Platform) [:0]const u8 { | ||
| /// Join `_parts` with separators and normalize into `buf`, nul-terminated. | ||
| /// | ||
| /// Returns `error.NameTooLong` when the concatenated input length exceeds | ||
| /// what `buf` can hold. The underlying `joinStringBuf` -> `normalizeStringNodeT` | ||
| /// pipeline has no output bounds check (`normalizeStringGenericTZ` writes | ||
| /// segments via unchecked `@memcpy`), so without this guard a too-long input | ||
| /// overflows `buf` — a safety panic in debug, UB in ReleaseFast. The check is | ||
| /// against the pre-normalization concatenated length; normalization can only | ||
| /// shrink, so a result that would fit after collapsing `..`/`.`/`//` may be | ||
| /// conservatively rejected. | ||
| /// | ||
| /// Callers that have already ensured the result fits (or want the legacy | ||
| /// panic-on-overflow) should use `joinZBufWithoutBoundsCheck`. | ||
| pub fn joinZBuf(buf: []u8, _parts: anytype, comptime platform: Platform) error{NameTooLong}![:0]const u8 { | ||
| var upper: usize = 0; | ||
| var first: u8 = 0; | ||
| for (_parts) |p| { | ||
| if (p.len == 0) continue; | ||
| if (upper == 0) first = p[0]; | ||
| if (upper > 0) upper += 1; // separator between non-empty parts | ||
| upper += p.len; | ||
| } | ||
| // One byte is reserved for the trailing nul. On non-Windows platforms | ||
| // `normalizeStringNodeT` additionally writes the body into `buf[1..]`, | ||
| // reserving `buf[0]` for a leading separator on absolute results; for | ||
| // relative inputs that byte is dead, so effective capacity is one less. | ||
| const leading_sep = first != 0 and platform.isSeparator(first); | ||
| const reserve: usize = if (platform == .windows or leading_sep) 1 else 2; | ||
| if (upper + reserve > buf.len) return error.NameTooLong; | ||
|
|
||
| const joined = joinStringBuf(buf[0 .. buf.len - 1], _parts, platform); | ||
| assert(bun.isSliceInBuffer(joined, buf)); | ||
| const start_offset = @intFromPtr(joined.ptr) - @intFromPtr(buf.ptr); | ||
| buf[joined.len + start_offset] = 0; | ||
| return buf[start_offset..][0..joined.len :0]; | ||
| } | ||
|
|
||
| /// 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"); | ||
|
Check warning on line 1267 in src/resolver/resolve_path.zig
|
||
|
Comment on lines
+1261
to
+1267
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor doc-accuracy note on "no behavior change when the result fits": that only holds when the concatenated input fits. Extended reasoning...What this isThe new var upper: usize = 0;
for (_parts) |p| { ...; upper += p.len; }
...
if (upper + reserve > buf.len) return error.NameTooLong;and So there exists a class of inputs — those containing redundant Step-by-step exampleTake
So a previously-resolvable shell Why this isn't a real-world concernOf the 20 migrated call sites, almost none can carry Addressing the "intentional behavior" objectionThe 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 |
||
| } | ||
| pub fn joinStringBuf(buf: []u8, parts: anytype, comptime platform: Platform) []const u8 { | ||
| return joinStringBufT(u8, buf, parts, platform); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.