Skip to content

bun test: stop panicking on a path argument or tree entry longer than the path buffer - #35863

Open
robobun wants to merge 3 commits into
mainfrom
claude/farm/93228605/join-abs-overflow-bound
Open

bun test: stop panicking on a path argument or tree entry longer than the path buffer#35863
robobun wants to merge 3 commits into
mainfrom
claude/farm/93228605/join-abs-overflow-bound

Conversation

@robobun

@robobun robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Fixes #35728

Problem

  • bun test "/$(printf 'a%.0s' $(seq 1 5000)).test.ts" aborts with panic: range end index 5008 out of range for slice of length 4095 (exit 134) instead of reporting that the path matched nothing. The reporter hit it at 997 bytes on macOS, where the buffer is 1024 bytes.
  • bun test with no arguments aborts the same way when the tree it walks contains an entry (directory, test file, or symlink) whose absolute path is longer than the buffer; this is the scenario of test scanner: skip paths exceeding MAX_PATH_BYTES instead of panicking #32412.
  • Scanner (src/runtime/cli/test/Scanner.rs) builds every absolute path it needs into a fixed PathBuffer with the unchecked join_abs_string_buf / FileSystem::abs_buf: the positional argument in scan(), each directory it descends into in the dirs_to_scan loop, and each directory and candidate file in next().
  • For an entry that readdir reports as a symlink (or as unknown, which some filesystems do for everything), next() first asks the resolver for the entry's kind, and RealFS::kind (src/resolver/lib.rs) joins dir + name into its own PathBuffer with the same unchecked join. normalize_string_buf writes into the caller's buffer without a bounds check, so any result longer than the buffer panics at either layer.

Fix

  • Every join in Scanner.rs goes through the existing join_abs_string_buf_checked / FileSystem::abs_buf_checked, which return None when the normalized result does not fit.
  • scan() maps None to ScanError::DoesNotExist, so a single over-long argument prints Test filter "..." had no matches and exits 1, and an over-long argument next to valid ones is ignored, exactly like a path that does not exist. This is correct because such a path cannot be opened either: the OS rejects it with ENAMETOOLONG.
  • The directory walk skips an entry whose path does not fit (not descended into, not matched, not added) and the rest of the run is unaffected. Directories are opened relative to the parent fd, so everything that does fit is still scanned when a tree continues past the limit.
  • RealFS::kind uses the checked join too and returns Error::Sys(ENAMETOOLONG) when the path does not fit. Its only callers, Entry::kind / Entry::symlink, already treat a failed stat as "unknown, assume file" (the same thing that happens when an entry is deleted between readdir and stat), and the scanner's file path then skips the entry through its checked join. Without this part, a symlink in the deepest directory still aborted with the scanner change alone (range end index 4100 out of range for slice of length 4095, in the platform::Posix join that only the resolver uses).
  • The POSIX dirs_to_scan join and RealFS::kind join into buf[..len - 1] / buf[..len - 2] because they write NULs after the path (same shape as node_fs.rs), so the set of paths that work is exactly the set that worked before; only the panicking inputs change behavior. The Windows dirs_to_scan branch passes the joined bytes to open_dir_no_renaming_or_deleting_windows directly (it transcodes to UTF-16 and never used the NUL), which leaves FileSystem::abs_buf_z without callers, so it is removed.
  • Scope: this PR covers the scanner, which is what bun test: panic on a single positional argument >= 997 bytes (fixed 1023-byte buffer) #35728, test scanner: skip paths exceeding MAX_PATH_BYTES instead of panicking #32412 and fix(test): no panic on over-long positional arguments in bun test #38896 are about. The other bun test inputs that reach this buffer family (--coverage-dir, --reporter-outfile, bunfig [test] root, in test_command.rs) still abort on over-long values and are intentionally left to a separate change, as are bun -c <path> (bunfig/arguments.rs) and Module._nodeModulePaths() (resolver_jsc.rs); each has its own reporting path and test location. Per-site conversion to the checked helpers is how the rest of this family has been fixed (resolver: don't abort on a package.json browser map key longer than 1024 bytes #37526, install: stop panicking on workspaces entries longer than the path buffer #37531).
  • Tests: six new cases in test/cli/test/bun-test.test.ts under "test file discovery (scanner)". Absolute (5000 and 100000 bytes) and relative over-long arguments report had no matches and exit 1; an over-long argument next to a valid one still runs the valid file; a tree whose deepest reachable directory contains an over-long test file, symlink and subdirectory runs only the shallow test file, scanned once without a bunfig and once with pathIgnorePatterns (which joins directories on a different path in next()). The three entries cover the file join, the resolver join and the directory joins. On the unfixed build all six abort (range end index 5008 / 5036 / 100008 / 4108 / 4109 ...); with the fix all six pass, and bun-test.test.ts, path-ignore-patterns.test.ts, resolve.test.ts, require.test.ts and resolve-error.test.ts pass (214 pass). cargo check --target x86_64-pc-windows-msvc for bun_resolver and bun_runtime and cargo clippy for both crates are clean.
  • The tests are skipped on Windows, where the buffer is 32767 * 3 + 1 bytes: larger than a command line or an NT path can deliver, so the overflow is not reachable there.

Background

  • PathBuffer is [u8; MAX_PATH_BYTES], the stack buffer bun uses for path syscalls: 4096 bytes on Linux, 1024 on macOS and the BSDs, 98302 on Windows.
  • join_abs_string_buf(cwd, buf, parts) resolves parts against cwd and normalizes the result into buf; it assumes the caller knows the result fits. join_abs_string_buf_checked is the variant for input of arbitrary length: it normalizes into heap scratch when the input might not fit and returns None if the normalized result is longer than buf (a long input that normalizes down through .. still succeeds). FileSystem::abs_buf / abs_buf_checked are the same two functions with the project root as cwd.
  • The resolver's directory cache records each entry's kind from readdir's d_type. Symlinks and DT_UNKNOWN entries have no kind yet, so Entry::kind stats them on first use through RealFS::kind, and on any error keeps the placeholder kind (file). The test scanner calls Entry::kind for every entry before deciding whether to queue or match it.
  • The scanner queues each subdirectory and opens it with openat relative to the parent's fd, so each syscall only sees one component; that is why a tree can legitimately be deeper than the buffer while still being walkable up to the limit.
Relationship to the other PRs and to the first version of this PR
  • test scanner: skip paths exceeding MAX_PATH_BYTES instead of panicking #32412 (June) is the original of the Scanner change, for the deep-tree variant seen in Sentry; it was marked superseded by this PR but left open. Its scenario is covered by the directory-walk tests here (now including the resolver path, which neither version had), and it is closed.
  • fix(test): no panic on over-long positional arguments in bun test #38896 by @deepshekhardas switches the abs_buf_projected sites to the checked join but leaves the two dirs_to_scan joins unchecked, so a deep tree still panics, and its 997 / 1200 byte arguments do not overflow the 4096-byte buffer on Linux (the "no matches" assertion fails there with or without the fix). Closed in favor of this PR.
  • The first version of this PR additionally made join_abs_string_buf itself return an empty slice on overflow. Main has since fixed the other callers of this family individually with the checked helpers, and an empty path silently handed to a caller that does not expect it is harder to reason about than Option at each site, so that part was dropped when the PR was rebased.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 6 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7f20fcfd-ab67-4d96-a32d-e9893ac12d07

📥 Commits

Reviewing files that changed from the base of the PR and between 9fb606f and 78bd4d5.

📒 Files selected for processing (3)
  • src/resolver/lib.rs
  • src/runtime/cli/test/Scanner.rs
  • test/cli/test/bun-test.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:13 PM PT - Aug 15th, 2026

🔄 @robobun, the build for your commit 78bd4d53 (Build #98187) was cancelled — waiting for the next build...

Comment thread src/paths/resolve_path.rs Outdated
Comment thread src/paths/resolve_path.rs Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. bun test: panic on a single positional argument >= 997 bytes (fixed 1023-byte buffer) #35728 - Reports the exact same range end index N out of range for slice of length 1023 panic when bun test receives a positional argument >= 997 bytes, which is the join_abs_string_buf overflow this PR bounds-checks

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #35728

🤖 Generated with Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. test scanner: skip paths exceeding MAX_PATH_BYTES instead of panicking #32412 - Fixes the same test-scanner panic on paths exceeding MAX_PATH_BYTES in Scanner.rs; PR bun test: stop panicking on a path argument or tree entry longer than the path buffer #35863 explicitly supersedes this.
  2. Bun.build: report an error for HTML rooted script src paths >= 4096 bytes instead of aborting #35860 - Addresses the same _join_abs_string_buf overflow in src/paths/resolve_path.rs with a competing approach (spill buffer vs bounds check), and also guards the same resolver call site.

🤖 Generated with Claude Code

Comment thread src/paths/resolve_path.rs Outdated
Comment thread src/paths/resolve_path.rs Outdated
@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

On the duplicate-PR note: #35860 fixes a different caller (the HTML <script src> / --external path through the thread-local join_abs_string wrappers, plus Resolver::load_as_file). This PR fixes the bun test scanner's joins, which #35860 does not touch. The two no longer share any files and merge independently.

Superseded note (from the first version of this PR)

The first version of this PR also bounded _join_abs_string_buf itself in resolve_path.rs, which would have covered #35860's repro as a side effect. That part was dropped when the PR was rebased (see the PR description); per-call-site checked joins are the pattern main has used for the other members of this buffer family since (#37526, #37531), so #35860's own fix is still needed for its callers.

Comment thread src/paths/resolve_path.rs Outdated
Comment thread test/cli/test/bun-test.test.ts Outdated
Comment thread test/cli/test/bun-test.test.ts Outdated
Comment thread src/paths/resolve_path.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three findings from earlier review rounds are addressed in 30ae627 and 7c2d182 (UNC +1 spill, < fast-path gate, Windows argv tests skipped, vacuous stdout checks dropped). I didn't find anything new this pass, but this changes the overflow contract of a core path-join primitive that every resolver/bundler/router caller reaches, and #35860 takes a competing/complementary approach at a different layer — worth a maintainer sign-off on the layering.

Checked: POSIX <= gate is sound (non-Windows normalize with ALLOW_ABOVE_ROOT=false, PRESERVE_TRAILING_SLASH=true never grows input); normalize_spill's +1 covers the Windows buf[vol_len]=sep write; the boundary unit tests exercise both sentinel and non-sentinel; FileSystem::abs_buf_checked exists and the Scanner's Option handling on every join site returns/continues/skips on None. One minor note: the Platform::Nt branch in _join_abs_string_buf returns b"\\\\?\\" (4 bytes) rather than empty on inner-Windows overflow, so the new doc comment isn't strictly true for Nt — but Nt isn't reachable from the user vectors named here and join_abs_string_buf_checked debug-asserts against it.

Extended reasoning...

Overview

This PR bounds _join_abs_string_buf (POSIX and Windows variants in src/paths/resolve_path.rs) so that when the unnormalized concatenation exceeds the caller's fixed output buffer it normalizes into a heap spill and either copies the shrunk result back or returns &buf[..0], instead of panicking at a slice bounds check inside normalize_string_generic_tz. It also switches every path join in the test scanner (src/runtime/cli/test/Scanner.rs) to the existing join_abs_string_buf_checked/abs_buf_checked, mapping overflow to ScanError::DoesNotExist at the entry point and to skip/continue during directory walk. Three integration tests in test/cli/test/bun-test.test.ts cover the bun test <huge-arg> repro, and four cargo unit tests in resolve_path.rs cover the primitive directly (overflow → empty, overflow-normalizes-down, Windows drive/UNC/exact-boundary/sentinel, fits-unchanged).

Security risks

None new. The change replaces a user-triggerable release-build panic (DoS) with a bounded empty-path return that surfaces as ENOENT/no-match downstream. Returning empty rather than truncating means an over-long path cannot accidentally resolve to an unrelated file. The spill vec is sized to input.len()+1 and #[cold], so no unbounded allocation on the hot path.

Level of scrutiny

High. join_abs_string_buf is a core primitive reached by the resolver, bundler, router, HTML scanner, --external, and Bun.build plugin onResolve — the PR itself enumerates these. Changing its overflow contract from panic to empty-slice is the right call per REVIEW.md ("User-reachable failures are recoverable errors, never panics"; "Fix bugs at the layer that owns the violated invariant"), and the hot path is byte-identical apart from one length comparison. But it is still a behavioral contract change to shared infrastructure, and #35860 proposes a complementary spill at the thread-local-wrapper layer; a maintainer should confirm the layering and whether both land.

Other factors

Two prior review rounds by me flagged (1) an undersized Windows spill for the UNC buf[vol_len]=sep write, (2) Windows argv tests that would fail on CreateProcessW's 32K limit, (3) a vacuous stdout panic-string assertion, and (4) the <= fast-path gate letting the exact-boundary UNC case through — all four were fixed in 30ae627 / 7c2d182 with unit tests added for the UNC and exact-boundary cases. This run I re-verified the POSIX gate is correctly <= (non-Windows normalize provably never grows input), that FileSystem::abs_buf_checked exists (src/resolver/lib.rs:348), and that every Scanner call site handles None. The Platform::Nt branch prepends \\?\ after the inner Windows join, so on inner overflow it returns a 4-byte prefix rather than empty — a minor doc-comment inconsistency but not user-reachable via the vectors named here (and _checked debug-asserts against Nt). No outstanding reviewer comments; all inline threads are resolved.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No further issues found after the earlier review rounds — the UNC +1 spill sizing, the Windows < gate, and the argv/CreateProcessW test skip all look correct now. Deferring to a human because this changes the contract of join_abs_string_buf (overflow → empty slice instead of panic) for every caller in the tree, and #35860 takes a competing approach at an overlapping layer; a maintainer should pick/reconcile.

What was reviewed:

  • POSIX and Windows spill gates re-traced against normalize_string_generic_tz's UNC buf[vol_len]=sep write — the < avail gate and input.len()+1 spill now cover the exact-boundary case.
  • empty::<IS_SENTINEL> return path: verified join_abs_string_buf_z still yields a valid ZStr (NUL at buf[0], len 0).
  • Scanner.rs: confirmed FileSystem::abs_buf_checked exists (src/resolver/lib.rs:348) and the [..buf_len-1] reservation leaves room for the manual NUL write.
  • Integration tests: skipIf(isWindows) + comment satisfy the harness rule; positive assertions ("had no matches", exitCode==1) fail on the unfixed build.
Extended reasoning...

Overview

Bounds-checks the output side of _join_abs_string_buf (POSIX + Windows) so an over-long normalized result returns &buf[..0] instead of panicking in normalize_string_generic_tz. Adds a #[cold] normalize_spill heap path so inputs that overflow unnormalized but collapse under normalization still succeed. Switches every path join in the test scanner to the checked variant so bun test <huge-arg> reports "no matches" / exit 1. Four cargo unit tests plus three integration tests.

Security risks

Path resolution is security-adjacent, but the change is strictly narrowing: an input class that previously aborted the process now yields an empty path (which cannot resolve to an unintended file — it fails ENOENT). No truncation, no new resolution behavior for in-bounds inputs. I don't see an exploitable vector introduced here.

Level of scrutiny

High. join_abs_string_buf sits under the resolver, bundler, router, HTML scanner, and Bun.build plugin surface — every one of those callers now observes empty-on-overflow instead of a panic. The PR argues this is safe (empty ≠ any real file), which I agree with, but it's a contract change to a foundational primitive and there's a live competing PR (#35860) that instead spills the thread-local wrappers to preserve the full over-length string. A maintainer should decide which layering wins (or confirm both merge, per robobun's earlier analysis).

Other factors

  • Three prior review rounds from this bot flagged the UNC spill sizing, the <=< gate, and Windows argv delivery — all resolved and now covered by unit tests including the exact-boundary cases.
  • robobun's last CI status shows failures on ba15646; a retrigger commit followed. I did not gate on CI state.
  • The comment-cop bot flagged long comments on earlier revisions; the current diff's comments are concise and load-bearing (each names the invariant it protects).

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Status: rebased onto current main as the Scanner change (9774880), then extended with the resolver's RealFS::kind join (78bd4d5) after a review pass found that a symlink whose path is over-long still aborted through that path; title and description describe the current state. #32412 and #38896, which addressed the same panic, are closed in favor of this PR.

Reproduced on a debug build of main (7d276b9) with the command from #35728: panic: range end index 5008 out of range for slice of length 4095, exit 134. With this branch the same command prints Test filter "/aaa..." had no matches and exits 1. The symlink variant was reproduced against a build of the scanner-only version (b1920bb): panic: range end index 4100 out of range for slice of length 4095; it passes with 78bd4d5.

The six cases in test/cli/test/bun-test.test.ts (-t MAX_PATH_BYTES) all fail on the unfixed build with the panic and pass with the fix; bun-test.test.ts, path-ignore-patterns.test.ts and the resolve / require / resolve-error suites pass (214 pass).

The sibling sites outside the scanner that the review turned up (--coverage-dir, --reporter-outfile, bunfig [test] root, bun -c, Module._nodeModulePaths()) are listed as out of scope in the description and are being handled separately.

CI on 78bd4d5 (build 98187): 177 of 179 jobs passed, 0 failed; every annotation entry is a retry-passed flake on files this PR does not touch. The 2 remaining jobs were the darwin 14 aarch64 test lanes (the only macOS test lane in the current matrix, and the platform #35728 was reported on); they sat queued for 4.5h behind the shared macOS pool and were then canceled by queue cleanup, so the 1024-byte MAX_PATH_BYTES branch of the new tests has not yet run in CI. Retrying just those two jobs is pending.

Jarred-Sumner pushed a commit that referenced this pull request Aug 13, 2026
…ffer (#37531)

## Repro

```sh
mkdir probe && cd probe
bun -e 'require("fs").writeFileSync("package.json", JSON.stringify({ name: "p", workspaces: ["a".repeat(5000)] }))'
bun install
```

```
panic: range end index 5010 out of range for slice of length 4095
```

`bun install` aborts (SIGABRT, exit 134). A glob entry
(`"a".repeat(5000) + "/*"`) aborts the same way with `panic: range end
index 5000 out of range for slice of length 4095`. Reproduced with `bun
1.4.0-canary.1` on Linux, where the buffer is 4096 bytes; it is 1024 on
macOS and 32767 * 3 + 1 on Windows (the glob case is 4096 on every
platform). A 2000 byte entry, which fits the buffer, already fails the
way one would expect:

```
error: ENAMETOOLONG reading package.json for workspace package "aaa..." from "/tmp/probe"
```

## Cause

`WorkspaceMap::process_names_array` joins every entry of the
`workspaces` array with the unchecked join helpers, whose normalizer
indexes past the output buffer when the result does not fit
(`resolve_path.rs`, `buf[buf_i..buf_i + count]`; the "length 4095" is
the buffer minus the leading separator):

* path entries: `join_abs_string_buf_z(project dir, PathBuffer, [entry,
"package.json"])`
* glob entries: `join([pattern, "package.json"])` into the 4096 byte
thread local buffer, before the pattern ever reaches the glob walker
* glob matches: `join_abs_string_buf_z(project dir, PathBuffer, [matched
dir, "package.json"])`. The walker works relative to the project dir, so
it can match a `package.json` whose path relative to the project fits
while the absolute path does not. A member directory whose absolute path
is 4090 bytes long (creatable, it fits PATH_MAX) with `workspaces:
["**"]` panics with `range end index 4102 out of range for slice of
length 4095`.

The folder dependency arm in `lockfile/Package.rs` already uses
`join_abs_string_buf_checked` for the same situation.

## Fix

* The two absolute joins use `join_abs_string_buf_checked`. When it
returns `None` the entry takes the existing error path with
`Error::Sys(ENAMETOOLONG)`, so it prints exactly what an entry the OS
rejects prints today (`ENAMETOOLONG reading package.json for workspace
package "..."`) and `bun install` exits 1. A path that does not fit the
buffer does not fit `PATH_MAX` either, so this is the same answer one
layer earlier. `process_workspace_name` takes `&[u8]` now;
`WorkspacePackageJSONCache::get_with_path` never needed the NUL
terminator.
* The glob pattern join uses `join_spill`, the variant the glob walker
itself uses for long paths, and the pattern is handed to the walker like
any other. The walker already copes with long patterns: a pattern that
matches nothing is skipped, the same as a 2000 byte one is today, and
one that does match (a brace group padded past the buffer size) still
finds its workspace. Rejecting long patterns outright would turn that
second case into an error.

Both joins are checked on the normalized result, so an entry that is
long as written but normalizes to a short path
(`x/../x/../.../pkgs/pkg1`) keeps resolving, same as before.

Related: #37462 fixes the same kind of overflow for local tarball and
`workspace:` dependency specs in other files; #35863 changes the shared
join primitive to return an empty path on overflow, which would turn
these panics into a misleading `Workspace not found`. This PR is
independent of both: the call sites report `ENAMETOOLONG` either way.

## Tests

`test/cli/install/bad-workspace.test.ts` (the existing file for bad
`workspaces` entries), all spawning `bun install` without a registry:

* 100 kB path entry: `ENAMETOOLONG`, exit 1 (every platform)
* path entry whose joined `package.json` path is one byte below /
exactly / one byte above the buffer size: `Workspace not found` (the
path still reaches the OS) / `ENAMETOOLONG` / `ENAMETOOLONG` (POSIX; on
Windows the OS rejects paths long before the buffer size)
* 100 kB brace glob still matches `pkgs/pkg1`; 100 kB glob matching
nothing leaves the other entries alone (every platform)
* 100 kB `x/../` path and glob entries that normalize to `pkgs/pkg1`
resolve (every platform, passes before and after: guards the normalized
length semantics)
* glob match whose absolute `package.json` path does not fit:
`ENAMETOOLONG`, exit 1 (POSIX; the member directory is created at buffer
size - 6 bytes and its `package.json` written relative to the parent
directory)

On the unfixed build (`USE_SYSTEM_BUN=1`) six of these crash with the
panics above (`100035`, `100008`, `100000`, `index 4096` for the NUL
write of the "exactly" row, `4096`, `4102`); the "one byte below" row
and the two normalizing entries pass both ways. With `bun bd test` the
file passes, as do `bun-workspaces.test.ts` (63 tests) and
`test/cli/run/workspaces.test.ts`.
…d of panicking

Scanner joined every path it builds (the positional argument, each
directory it descends into, each candidate test file) into a fixed
PathBuffer through the unchecked join, so an absolute path longer than
MAX_PATH_BYTES aborted the whole run with
"range end index N out of range for slice of length 4095".

Use join_abs_string_buf_checked / abs_buf_checked everywhere. An
over-long positional argument is reported as DoesNotExist, so it prints
"had no matches" and exits 1 like any other missing path; over-long
entries met during the directory walk are skipped. The Windows branch
passes the joined bytes to the opener directly (it transcodes them to
UTF-16 and never used the NUL), which leaves FileSystem::abs_buf_z
without callers, so it is removed.

Fixes #35728
@robobun
robobun force-pushed the claude/farm/93228605/join-abs-overflow-bound branch from 886259c to 9774880 Compare August 15, 2026 08:18
Comment thread src/runtime/cli/test/Scanner.rs Outdated
@robobun robobun changed the title paths: bound join_abs_string_buf on output overflow; test scanner uses checked join bun test: stop panicking on a path argument or tree entry longer than the path buffer Aug 15, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline nit: the diff no longer touches src/paths/resolve_path.rs, so the primitive-level bound, normalize_spill, and the four cargo test -p bun_paths units the PR body describes aren't part of this 3-file change — #35728 is still fixed at the scanner layer via the pre-existing join_abs_string_buf_checked, and abs_buf_z has no remaining callers. Also checked that the deep-tree fixture's over-PATH_MAX entries don't strand tempDir cleanup.

Extended reasoning...

This is my third pass on this PR. The first two rounds (2026-07-26) reviewed a 4-file change that included overflow handling in src/paths/resolve_path.rs; those inline comments are now resolved but reference a file no longer in the diff. The current 3-file change is narrower — Scanner.rs switches every join to the existing join_abs_string_buf_checked and handles None, resolver/lib.rs deletes the now-unused abs_buf_z, and the tests are the same ones already reviewed (with the earlier fixes applied). The PR title and body still describe the dropped primitive work; noting it here so the next reader isn't confused by the mismatch. The one open comment-cop flag on Scanner.rs:52 targets a 3-line doc comment on an error variant, not a workaround justification.

Comment thread test/cli/test/bun-test.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the isLinux ? 4096 : 1024 change and the one-line ScanError doc address the two open notes from the last round.

What was reviewed:

  • Each of the five Scanner.rs join sites now routes through the checked helper with the appropriate fallback (Err(DoesNotExist) for the entrypoint, continue/return for walk entries); the POSIX [..buf_len - 1] slice correctly reserves the NUL slot written immediately after.
  • abs_buf_z has no remaining callers in src/, so its removal is safe; the Windows branch passes the raw &[u8] to open_dir_no_renaming_or_deleting_windows, which never depended on the NUL.
  • The six new tests follow the harness conventions (tempDir, Buffer.alloc, skipIf(isWindows) with the CreateProcessW rationale) and the deep-tree fixture's arithmetic now mirrors the cfg!(linux) branch of MAX_PATH_BYTES.
Extended reasoning...

Overview

The PR converts every path join in src/runtime/cli/test/Scanner.rs from the unchecked join_abs_string_buf / FileSystem::abs_buf / abs_buf_z to the existing checked variants that return Option on overflow, and handles None at each of the five sites: the scan() entrypoint returns ScanError::DoesNotExist, the two dirs_to_scan open loops continue, and the two next() join sites return for that entry. FileSystem::abs_buf_z is removed from src/resolver/lib.rs (grep confirms no remaining callers). Six new integration tests in test/cli/test/bun-test.test.ts cover over-long absolute/relative arguments, a mixed argument list, and a deep directory tree with and without pathIgnorePatterns.

Security risks

None. The change tightens bounds handling on user-supplied CLI paths and directory-walk entries; overflow now produces the same "no matches" outcome the OS would give via ENAMETOOLONG. No auth, crypto, or privilege boundaries are involved.

Level of scrutiny

Moderate. This is a targeted panic fix in the bun test file scanner that follows the same per-call-site pattern already landed in #37526 / #37531. It is not a hot path in the request-serving sense, and the failure mode of the new code (skipping an entry that could not be opened anyway) is strictly better than the old failure mode (abort). The PR has been through four prior review rounds here; all earlier findings (Windows UNC spill sizing, CreateProcessW argv limit, vacuous stdout assertion, isMacOS vs isLinux constant) were addressed and the corresponding threads are resolved or superseded by the current diff.

Other factors

  • The bug-hunting system found nothing on this revision.
  • Commit b1920bb addressed my last nit (isLinux ? 4096 : 1024) and the comment-cop note on the ScanError doc comment.
  • The PR description records that all six new tests abort with the range end index ... out of range panic on the unfixed build and pass with the fix, and that cargo check --target x86_64-pc-windows-msvc and cargo clippy -p bun_runtime are clean.
  • The [..buf_len - 1] slice on the POSIX branch reserves exactly the byte the following open_dir_buf[path2_len] = 0 writes, matching the shape referenced from node_fs.rs; the Windows branch no longer needs the sentinel because open_dir_no_renaming_or_deleting_windows transcodes the slice to UTF-16 itself.

…oes not fit

Entries that readdir reports as symlinks (or as unknown) are stat'ed
lazily through RealFS::kind, which joined dir + name into a PathBuffer
with the unchecked join. The test scanner reaches it before any of its
own joins, so a symlink whose absolute path is longer than the buffer
still aborted the run. Entry::kind and Entry::symlink already treat a
resolve error as "unknown, assume file", which makes the scanner skip
the entry through its checked join.

The directory-walk test now also puts a symlink in the deepest
directory; with only the scanner change it aborted the same way.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — panic-to-error conversion at every over-long path join in the test scanner, following the same checked-join pattern already merged in #37526/#37531.

What was reviewed:

  • All five join sites in Scanner.rs now use join_abs_string_buf_checked/abs_buf_checked; overflow maps to DoesNotExist (entrypoint) or skip (walk), matching how the OS would reject the path anyway.
  • RealFS::kind (new in 78bd4d5) reserves 2 bytes for the trailing NULs and returns ENAMETOOLONG on overflow; Entry::kind already swallows errors to the cached default, and the scanner's file-branch checked join then skips it. Verified open_dir_no_renaming_or_deleting_windows takes &[u8] (not a ZStr), so dropping the NUL on the Windows walk branch is correct, and abs_buf_z has no remaining callers.
  • Six tests cover argv (absolute/relative, mixed with valid) and deep-tree walk (with/without pathIgnorePatterns, plus a symlink to reach the resolver path); all prior review feedback (Windows argv limit, vacuous assertions, isLinux vs isMacOS) is applied.
Extended reasoning...

Overview

This PR fixes #35728: bun test panicking with range end index N out of range for slice of length M when a path argument or a discovered tree entry exceeds MAX_PATH_BYTES. It touches three files: src/runtime/cli/test/Scanner.rs (five unchecked joins → checked), src/resolver/lib.rs (RealFS::kind → checked join returning ENAMETOOLONG, and removal of the now-callerless abs_buf_z), and test/cli/test/bun-test.test.ts (six new POSIX-only tests).

The PR has been through four review rounds. Since my last inline comment (the isMacOSisLinux nit), two commits landed: b1920bb applied that nit and shortened the ScanError doc, and 78bd4d5 added the RealFS::kind change so the deep-tree symlink case (which reaches the resolver's stat path rather than the scanner's own joins) also stops panicking.

Security risks

None. The change replaces panics on user-controllable input (CLI arguments, on-disk directory entries) with typed errors or skips. No new parsing, no privilege boundaries crossed, no resource acquisition.

Level of scrutiny

Medium. The scanner changes are CLI-only and mechanical. The RealFS::kind change touches shared resolver code, which raised the bar — but it is a strict improvement: the function already returned CrateResult, its sole caller Entry::kind already handles Err(_) by falling back to the cached kind, and the previous behavior on this input was an unrecoverable panic. The join_capacity = outpath.len() - 2 correctly accounts for the two NUL bytes written immediately after (the old code would also have panicked on the NUL write at entry_path_len == len - 1). This is the same per-call-site checked-join pattern that #37526 and #37531 already merged for other members of this buffer family.

Other factors

  • All prior review findings (Windows CreateProcessW argv limit, vacuous stdout assertions, UNC spill sizing from the earlier primitive-level version, MAX_PATH_BYTES cfg mirroring) are resolved and the corresponding threads are marked resolved.
  • The Windows walk branch now passes the joined slice directly to open_dir_no_renaming_or_deleting_windows, whose signature is (Fd, &[u8]) — verified it never depended on NUL termination, so abs_buf_z is correctly dead.
  • The POSIX walk branch joins into buf[..len - 1] to reserve the NUL byte written after — same shape as node_fs.rs, and the ZStr::from_raw bounds are unchanged.
  • Test coverage is thorough: three argv shapes, mixed valid+invalid, and two deep-tree configurations with a file, subdirectory, and symlink at the boundary. The PR body documents that all six fail with the panic on the unfixed build and pass with the fix; cargo check --target x86_64-pc-windows-msvc and clippy are clean.
  • The bug-hunting system found no issues on the current revision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bun test: panic on a single positional argument >= 997 bytes (fixed 1023-byte buffer)

2 participants