Conversation
… path buffer `bun publish <tarball>` resolved the positional argument with join_abs_string_buf_z, which indexes past the fixed PathBuffer when the resolved path does not fit, so an over-long argument aborted with a bounds panic and a crash report. Resolve it with join_abs_string_buf_checked and report ENAMETOOLONG through the existing "failed to read tarball" error instead, which is what the open syscall wrapper already reports for a path that fills the buffer.
|
Warning Review limit reached
Next review available in: 5 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 (2)
Comment |
|
Status: fix and tests pushed, waiting on CI. Reproduced on the current release binary (1.4.0-canary.1) and on main at 056491f with a package.json in the cwd and Tests are in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped crash fix that swaps an unchecked path join for the checked variant.
What was reviewed:
join_abs_string_buf_checkedreturnsOption<&[u8]>;File::read_fromandZStr::boxedboth take&[u8], so dropping the_zflavour and.as_bytes()is type-correct.- Error construction matches the existing
build_command.rsENAMETOOLONG pattern and the siblingFile::read_fromerror arm, so all overflow lengths now report the same message. - Tests cover the boundary (5000 bytes, buffer+1, exactly buffer) and pin that
..-normalized over-long args still resolve; hermetic via--dry-runagainst a closed port, Windows-skipped with a stated reason.
Extended reasoning...
Overview
The PR replaces join_abs_string_buf_z with join_abs_string_buf_checked in Context::from_tarball_path (src/runtime/cli/publish_command.rs:144) and reports ENAMETOOLONG via Output::err + Global::crash() when the resolved tarball path does not fit the PathBuffer. The only other source change is dropping .as_bytes() at the ZStr::boxed call site since the checked variant already returns &[u8]. A new describe("tarball path") block in bun-publish.test.ts covers three overflow lengths and one over-long-but-normalizes-down argument.
Security risks
None. The change converts a user-reachable panic (exit 134) into a recoverable error with exit 1 — strictly a hardening. No new input is trusted, no new I/O paths, and --dry-run in the tests keeps them off the network.
Level of scrutiny
Low-to-medium. It is a ~10-line targeted fix in a CLI error path, mirroring an existing pattern in build_command.rs. I verified the helper signatures: join_abs_string_buf_checked returns Option<&[u8]> (None when joined.len() > buf.len()), File::read_from(dir, &[u8]), and ZStr::boxed(&[u8]), so the type flow after dropping _z is correct. The exactly-buffer-size case (4096 on Linux) still returns Some from the checked join and is caught one layer down by openat_a's len >= MAX_PATH_BYTES synthesis, which the third test case exercises — both paths format the same message.
Other factors
Tests follow the repo conventions closely: describe.concurrent, tempDir, Buffer.alloc(n, fill) over .repeat, exit code asserted last, Windows skip commented, and a positive test pinning that a naive raw-argument length check would be wrong. The PR description records the unfixed-binary panic output for each case, satisfying the "prove the test fails for the right reason" bar. No prior human reviews, no CODEOWNER-gated files, no outstanding comments.
The Windows buffer is sized for the longest command line as UTF-8, so an argument made of three byte characters, resolved against a directory made of them as well, still overflows it. Run the one byte over case on every platform and keep the two cases that cannot be built there POSIX only.
There was a problem hiding this comment.
LGTM — converts a user-reachable panic into the same ENAMETOOLONG error the shorter-by-one case already reports.
What was reviewed:
join_abs_string_buf_checkedreturn type (Option<&[u8]>) and thatZStr::boxed/File::read_fromboth accept&[u8], so dropping the_zflavour is safe.- Error construction matches the precedent in
build_command.rs:1335and the existingFile::read_fromfailure branch, so the message is identical regardless of which layer catches it. - The exact-buffer-length case:
_checkedreturnsSome(no NUL write), thenopenat_a'slen >= MAX_PATH_BYTEScheck produces the same errno — test covers this path too. - Tests: concurrent, hermetic (
--dry-run+ closed port 1), boundary cases at buf/+1/way-over plus the negative case pinning that normalization still shrinks long args; Windows skips are documented.
Extended reasoning...
Overview
Two files: src/runtime/cli/publish_command.rs swaps join_abs_string_buf_z for join_abs_string_buf_checked in Context::from_tarball_path and adds a let-else that reports ENAMETOOLONG via Output::err + Global::crash() when the resolved tarball path overflows the PathBuffer. The abs_tarball_path local changes from &ZStr to &[u8], so ZStr::boxed(abs_tarball_path.as_bytes()) becomes ZStr::boxed(abs_tarball_path). test/cli/install/bun-publish.test.ts gains a describe.concurrent("tarball path") block with four cases.
Security risks
None. This tightens input validation on a CLI positional argument — a user-reachable panic (which the review guide flags as a DoS) becomes a recoverable error with exit 1. No new trust boundaries, no auth/crypto/network changes.
Level of scrutiny
Low-to-medium. The Rust change is ~12 lines and follows the exact pattern already used at build_command.rs:1333-1337 for over-long asset paths. join_abs_string_buf_checked is the documented helper for user-controlled path parts and is used the same way at half a dozen other call sites (WorkspaceMap, node_fs, node_fs_watcher, BunObject). I verified its signature returns Option<&[u8]>, that ZStr::boxed takes &[u8], and that the fast/slow path in _checked normalizes before bounding so x/../-padded arguments still resolve.
Other factors
The tests are well-constructed per the repo guide: describe.concurrent for independent spawns, tempDir from harness, hermetic via --dry-run against a closed port with embedded credentials (so the auth check passes without a request), exact-message assertions on stderr before exit code, boundary coverage at exactly-the-buffer / +1 / 5000, and a negative test pinning that the bound applies to the resolved path. Windows skips are commented with the concrete reason (command-line length limit vs 96 KiB path buffer; ENOSYS/ENAMETOOLONG errno-table mismatch on the exact-length case). The PR description shows the three failing cases panic on the unfixed binary. No prior review comments to address.
|
Closing in favor of #43067. It fixes this trigger with the shared checked path helpers and carries the tests from this pull request. |
1 participant
Problem
bun publish <tarball>aborts with a crash report when the tarball argument does not fit the path buffer.bun publish "$(head -c 5000 /dev/zero | tr '\0' d).tgz" --dry-runprintspanic: range end index 5017 out of range for slice of length 4095and exits 134.panic: index out of bounds: the len is 4096 but the index is 4096.Context::from_tarball_path(src/runtime/cli/publish_command.rs:144) resolves the raw positional withjoin_abs_string_buf_zinto aPathBuffer. That helper assumes the result fits and indexes past the buffer when it does not.bun publishwas added. It is reachable on every platform: on Windows the buffer is sized for the longest command line as UTF-8, so an argument of three byte characters overflows it as well (panic: range end index 98303 out of range for slice of length 98302, exit code 3, reproduced on Windows Server 2019 with the current canary).ENAMETOOLONG: File name too long: failed to read tarball: '...' (open), exit 1. Only the lengths past that point crash.Fix
join_abs_string_buf_checked, which returnsNonewhen the normalized result does not fit, and in that case reportENAMETOOLONGthrough the existingfailed to read tarballerror and exit 1.MAX_PATH_BYTES): a resolved path that does not fit the buffer is exactly a pathFile::read_fromwould have refused withENAMETOOLONG(bun_sys::openat_asynthesizes that errno forlen >= MAX_PATH_BYTESbefore calling the OS). Reporting it one layer earlier gives the same message the lengths that already fail today get, so what the user sees no longer depends on which layer happened to catch the overflow.build_command.rsbuilds the same error for an over-long asset path.x/../x/../.../pkg.tgzlonger than the buffer still resolves and publishes (as before). The test pins that, so a plain length check on the argument is not an acceptable fix.File::read_fromtakes&[u8]and the context stores aZStr::boxedcopy, so nothing needed the NUL-terminated_zflavour; the unchecked join was the only thing providing it.ENOENTfor an empty path; this change keepsENAMETOOLONGfor it, so the two are complementary and either order works. publish: resolve a relative tarball path against the invoking directory #38704 (resolve against the invoking directory) and pack/publish: stop panicking on package.json bin and files entries longer than the path buffer #38784 (bin and files entries) touch the same import line and the same test file; the conflicts are textual. pack: report an error instead of panicking when --destination does not fit the path buffer #38749 adds a sharedMAX_PATH_BYTESto the test harness; this test keeps a local constant rather than adding a competing export, and can switch once that lands.describe("tarball path"):ENAMETOOLONG ... failed to read tarballand exits 1 on every platform. On Windows the argument and the directory it is resolved against are made of three byte characters; the test asserts the command line stays within the 32767 units CreateProcess accepts (about 140 units are left for the path of bun.exe).ENOSYStoday (it takes the errno number from the C runtime, whose numbering differs from bun's table; pre-existing and reported separately, this change uses bun's own constant and is not affected).Background
PathBuffer/MAX_PATH_BYTES: bun resolves paths into fixed stack buffers (4096 bytes on Linux, 1024 on macOS, 32767 * 3 + 1 on Windows, i.e. the longest UTF-16 command line at three UTF-8 bytes per unit) instead of allocating. The uncheckedjoin_abs_string_buf*helpers are for parts known to fit;join_abs_string_buf_checkedis the variant documented for user-controlled parts of arbitrary length. It normalizes into a heap scratch when the concatenation is too big and only then checks whether the result fits, which is why..segments still work. When the input would have fit anyway it takes the same path as the unchecked helper.bun_sys::openat_acopies a&[u8]path into aPathBufferto NUL terminate it and returns a synthesizedENAMETOOLONGwith syscall tagopenwhenlen >= MAX_PATH_BYTES.Output::errprints abun_sys::Erroras<errno>: <description>: <message> (<syscall>), which is the format the tests match.Global::crash()is the CLI's print-an-error-and-exit-1 helper used by every other error path infrom_tarball_path.New tests on the unfixed binary (Linux)
5041 rather than the 5017 in the report above because the test resolves against a longer temporary directory.
Shared case on Windows Server 2019 x64
Directory: 195 UTF-16 units, 495 bytes. Argument: 32603 units, 97807 bytes. Resolved path: 98303 bytes against a 98302 byte buffer.
no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-publish.test.ts