bun test: report over-long coverage dir, JUnit outfile and bunfig root instead of panicking - #39104
bun test: report over-long coverage dir, JUnit outfile and bunfig root instead of panicking#39104robobun wants to merge 1 commit into
Conversation
…t instead of panicking --coverage-dir / coverageDir, --reporter-outfile / test.reporter.junit and the bunfig [test] root were each copied or joined into a fixed path buffer without a length check, so a value longer than the buffer aborted the process with an out-of-range slice panic after the tests had already run. The JUnit outfile is now passed straight to File::openat, whose own length check returns ENAMETOOLONG, which the existing report-failed message prints. The lcov.info paths (serial writer and the --parallel merge) are built with AutoAbsPathChecked; an over-long directory is reported through the existing lcov error paths as ENAMETOOLONG. The bunfig root is joined with join_abs_string_buf_checked and a root that does not fit is reported like a root directory that does not exist.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Status: fix pushed, waiting on CI and self-review. Reproduced on the unfixed build (bun 1.4.0 canary, Linux) with a 5000 byte value for each input:
With this branch each prints its existing error ( |
|
Updated 11:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 2dcc7a4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 39104That installs a local version of the PR into your bun-39104 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the inline comment is a test-hygiene nit only (undrained stdout with ~30 bytes of output), not a correctness issue.
Extended reasoning...
Overview
Replaces four unchecked PathBuffer copies/joins in bun test with their length-checked equivalents so an over-long --coverage-dir, --reporter-outfile, or bunfig [test] root reports a clean error instead of panicking on a slice-index out of range. Two Rust files (test_command.rs, parallel/aggregate.rs) plus seven new tests across four existing test files.
Security risks
None. The inputs were already reaching syscalls; the change only converts a Rust slice-index panic into an ENAMETOOLONG error routed through the existing failure arms. No new parsing, no privilege changes.
Level of scrutiny
Moderate. The mechanical parts are direct swaps to AutoAbsPathChecked / join_abs_string_buf_checked / File::openat(&[u8]), all of which are the in-tree helpers designed for exactly this bug class (added in #36881 for --cpu-prof-dir). I verified:
openat_aat src/sys/lib.rs:6154 does the>= buf.len()check and returnsENAMETOOLONG, so passingpath: &[u8]straight through in the JUnit writer is correct.AutoAbsPathChecked::init_top_level_dir()reads the samebun_core::TOP_LEVEL_DIRthatbun_resolver::fs::FileSystem::get().top_level_dir(the oldrelative_dir) does, so the base path for the lcov joins is unchanged.slice_z()writes the NUL atbuf[len]withlen < MAX_PATH, sounlink/move_file_zstill get a valid ZStr; the twomut tmp_path/mut pathbindings are separate owners so both mutable borrows formove_file_zare fine.- The bunfig-root path uses a pooled
PathBuffersliced tolen - 1(room for the scanner's NUL), copies the joined result into the hoistedVec<u8>before the guard drops, and falls back to the raw configured value on overflow so the error message still names what the user typed. - The
LcovOutputstruct refactor computes both paths before opening the temp file, so the rename no longer joins at all — no behavior change on the success path, and the errdefer guard still closes+unlinks the temp on early return.
Other factors
Tests cover both CLI and bunfig entry points at 5000 and 100000 bytes, assert the specific error text and errno, assert signalCode === null (proving the panic is gone rather than just a different exit code), and pin the two pre-existing bunfig-root behaviors the change must not regress. The PR description confirms USE_SYSTEM_BUN=1 fails all seven new cases. The Windows skip conditions and the if (!isWindows) errno guard are explained in comments.
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); |
There was a problem hiding this comment.
🟡 These two tests explicitly set stdout: "pipe" but only await [proc.stderr.text(), proc.exited], leaving stdout undrained — REVIEW.md's subprocess rule requires draining all pipes concurrently, and the sibling test added in parallel.test.ts in this PR does so. There's no actual deadlock risk here since bun test writes only the version banner to stdout, so this is just a hygiene/consistency nit: either add proc.stdout.text() to the Promise.all (matching parallel.test.ts) or drop the explicit stdout: "pipe". Same at test/js/junit-reporter/junit.test.js:584-588.
Extended reasoning...
What the issue is
The new tests in test/cli/test/coverage.test.ts (the describe.concurrent("lcov reporter with a coverage directory longer than the path buffer") block) and test/js/junit-reporter/junit.test.js (the ${source} of ${length} bytes reports the failed write instead of crashing block) both explicitly pass stdout: "pipe" to Bun.spawn, but then only await [proc.stderr.text(), proc.exited] — the stdout pipe is never read.
REVIEW.md's subprocess-test rule states:
Subprocess tests: drain pipes concurrently.
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])— an unread pipe fills the ~64KB OS buffer and deadlocks the child.
The sibling test added in this same PR at test/cli/test/parallel.test.ts does follow the rule correctly:
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);so this is an inconsistency within the PR's own diff.
Step-by-step trace
Take coverage.test.ts:
Bun.spawn({ ..., stdout: "pipe", stderr: "pipe" })creates a stdout pipe.- The child
bun testwrites its version banner (bun test <version> (<revision>), ~30 bytes) to stdout, and everything else (test results, theFailed to create lcov file/ENAMETOOLONGmessages the test asserts on) to stderr. - The test awaits
Promise.all([proc.stderr.text(), proc.exited]). stderr is drained; stdout is not. - Because stdout receives only ~30 bytes — far below the ~64KB OS pipe buffer — the child never blocks on the write, and
proc.exitedresolves normally. await using procdisposes the subprocess and its unread stdout pipe on scope exit.
The identical pattern occurs in junit.test.js around lines 584-588.
Why existing code doesn't prevent it
Nothing does — the tests work as written because the child's stdout output is tiny. But the explicit stdout: "pipe" signals intent to read that pipe, and the repo's stated convention is to drain every pipe you open concurrently with .exited. If a future change to bun test sent more to stdout (e.g. a progress bar or the coverage text table), these tests could deadlock without any change to the assertions.
Impact
None functionally today: neither fixture can produce enough stdout to fill the kernel pipe buffer, so there is no hang or flake risk. This is purely a repo-convention / consistency deviation — the same PR's parallel.test.ts addition follows the rule, so the two other additions should match.
How to fix
Either of:
- Add
proc.stdout.text()to thePromise.all, matching theparallel.test.tstest:const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
- Or drop the explicit
stdout: "pipe"(or set it to"ignore"), since the tests never assert on stdout.
Applies to both test/cli/test/coverage.test.ts (~lines 628-633) and test/js/junit-reporter/junit.test.js (~lines 584-588).
1 participant
Problem
bun test --coverage --coverage-reporter=lcov --coverage-dir <5000 bytes>runs the tests and then aborts withpanic: range end index 5015 out of range for slice of length 4095(exit 134). The same happens withcoverageDirin bunfig, and with--parallel.bun test --reporter=junit --reporter-outfile <5000 bytes>runs the whole suite, then aborts withpanic: range end index 5000 out of range for slice of length 4096; the report is lost. Same for[test.reporter] junitin bunfig.[test] rootof 5000 bytes aborts withpanic: range end index 5021 out of range for slice of length 4095before any test runs.src/runtime/cli/test_command.rsJunitReporter::write_to_file: copied the outfile into aPathBuffer.src/runtime/cli/test_command.rsprint_code_coverage:join_abs_string_buf_zof the coverage dir and temp name into aPathBuffer, andjoin_abs_string_zof the finallcov.infoname into the 4096 byte thread-local buffer.src/runtime/cli/test/parallel/aggregate.rsmerge_coverage_fragments: the same join for the mergedlcov.info.src/runtime/cli/test_command.rsTestCommand::exec:join_absof the bunfig root into the 4096 byte thread-local buffer.Fix
File::openat. It already copies the path into its own buffer and returnsENAMETOOLONGwhen it does not fit, so the existingFailed to write JUnit report to ...message now prints that error, and the exit code stays the run's own result. This is exactly what the--parallelJUnit merge already did for the same input.--parallel): the temp and final paths are built withAutoAbsPathChecked, whosejoinfails instead of overflowing; that failure is fed into the existing open-failure error arm asENAMETOOLONGwith the configured directory, so the serial path printsFailed to create lcov fileand exits 1, and the coordinator printsFailed to write merged lcov.info, as they do for any other unwritable directory. The serial writer now joins both names before creating anything, so the rename at the end no longer joins at all. The path state held across the report is a small struct instead of a tuple, since it now carries two paths.join_abs_string_buf_checkedinto a pooled path buffer, leaving room for the NUL, and a root that does not fit is reported through the existingFailed to scan non-existent root directory for tests:arm with the configured value, exit 1. Output for every root that fits is byte for byte what it was before (including a trailing slash), because the checked join is the same join with a size check in front.ENAMETOOLONG/ non-existent is correct because the buffers are sized to the OS limit (PATH_MAX): any path the check rejects is one the OS would refuse to open withENAMETOOLONGanyway, so the only behavior that changes is the abort.test/cli/test/coverage.test.ts:--coverage-dirof 5000 bytes and bunfigcoverageDirof 100000 bytes.test/cli/test/parallel.test.ts: bunfigcoverageDirof 100000 bytes with--parallel=2.test/js/junit-reporter/junit.test.js:--reporter-outfileof 5000 bytes and bunfigtest.reporter.junitof 100000 bytes.test/cli/test/bun-test.test.ts: bunfig root of 5000 and 100000 bytes, plus a root that works and a root that does not exist (the latter two also pass before this change; they pin the behavior the root change must preserve).USE_SYSTEM_BUN=1) and pass with the fix; the four files pass in full withbun bd test(except four timing-based--parallelscaling tests inparallel.test.tsthat also fail without this change on this machine under ASAN).cargo fmtandcargo clippy -p bun_runtimeare clean.bun_sys's length check currently labels it differently (tracked separately).Background
PathBufferis bun's stack/pool buffer for path syscalls,MAX_PATH_BYTESlong: 4096 on Linux, 1024 on macOS, 98302 on Windows.join_abs_string_buf/join_absresolve parts against a base directory and normalize straight into such a buffer (or a 4096 byte thread-local one) and assume the result fits; a longer result indexes past the buffer, which is the panic.join_abs_string_buf_checkedis the same join that returnsNonewhen the normalized result does not fit.AutoAbsPathChecked(added for the same bug in--cpu-prof-dir, cli: report an error for over-PATH_MAX --cpu-prof-dir / --heap-prof-dir instead of panicking #36881) is a path builder over a pooledPathBufferthat starts from the project root and whosejoinreturnsErr(MaxPathExceeded)instead;slice_z()gives the NUL-terminated form thatunlink/move_file_zneed.File::openat(dir, &[u8])copies the path into aPathBufferto NUL-terminate it and returnsENAMETOOLONGif it does not fit, which is why the JUnit writer does not need a buffer of its own.lcov.infoonce the whole report has been written; the--parallelcoordinator instead receives each worker's lcov text over IPC and writes the merged file directly.