install: stop panicking on package labels longer than the stack buffers - #37469
Conversation
The hoisted installer formats each package's version label (its version, or the spec a tarball/folder/git package was resolved from) into a 512 byte stack buffer and treated overflow as unreachable, so any dependency with a long enough spec aborted `bun install`. Keep the stack buffer for the common case and spill into a Vec when the label does not fit. `bun patch` and `bun patch --commit` formatted the same labels into 1024 byte buffers at four sites with the same failure mode; format them into a Vec instead.
|
Warning Review limit reached
Next review available in: 1 minute 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 (4)
Comment |
|
Status: reproduced on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — replaces reachable .expect() panics on fixed-size label buffers with a spill-to-Vec fallback, keeping the allocation-free fast path.
What was reviewed:
print_package_versionspill pattern matchesresolve_path::join_z_buf_spill;ArgumentsisCopyso re-formatting into the Vec afterbuf_printfails is sound, andresolution_spillis fresh at each call site.do_patch_commitrefactor is byte-for-byte equivalent forpatch_keyandstring_hash; the.patchfilename now built via.concat()also removes a latent slice-index panic at ~1018-byte labels.- Checked all four
patchPackage.rssites and bothPackageInstaller.rsbranches; no remaining fixed-buffer.expect()on the resolution label. - Tests cover each site independently, use
x/../so nothing long touches disk, follow harness conventions (VerdaccioRegistry,port: 0, concurrent pipe drain,Buffer.alloc).
Extended reasoning...
Overview
The PR fixes a user-reachable panic in bun install and bun patch when a package's version label (tarball spec, remote URL, folder path, git ref, or workspace prerelease tag) exceeds the fixed 512/1024-byte stack buffers it was formatted into with .expect("unreachable"). Two Rust files are touched: PackageInstaller.rs gains a print_package_version helper that tries the stack buffer first and spills into a caller-owned Vec on overflow; patchPackage.rs gains print_resolution_label (clear-and-reuse Vec) applied at four sites, plus a small refactor of do_patch_commit to build patch_key as a Vec directly and derive the patch filename from it. Seven new tests across two files exercise every changed site.
Security risks
None. The change replaces a panic with correct handling of long user-supplied strings; nothing new is written to disk with the long label (the tests deliberately use x/../ so paths normalize short), and the label is only compared/hashed. No parsing, auth, or trust boundary is touched.
Level of scrutiny
Medium. This is package-manager code on the install path, but the change is a targeted crash fix with a well-understood shape (buf-then-spill, matching resolve_path::join_z_buf_spill). The do_patch_commit refactor was checked for byte equivalence: the old code formatted name@resolution into a stack buffer, hashed it, appended .patch in place, then copied the prefix into a fresh Vec for patch_key; the new code formats directly into the patch_key Vec, hashes it, and builds the filename via [patch_key, b".patch"].concat(). The bytes and hash input are identical, and the in-place suffix write (which itself would have panicked at ~1018 bytes) is gone.
Other factors
core::fmt::ArgumentsisCopy, so passingargsto bothbuf_printandwrite_fmtis sound.resolution_spillis declared fresh before the workspace/non-workspace branch and only one arm runs, so the helper never sees a non-empty spill.print_resolution_labelclears its Vec first, so reuse across candidate loops is correct and allocation-free after the first grow.- The
.expect("formatting into a Vec is infallible")calls are true invariants:Vec<u8>: io::Writecannot fail, and theDisplayimpls only propagate writer errors. - Tests follow repo conventions: added to existing files,
test.concurrent,Buffer.alloc(n, fill)instead of.repeat(), localBun.serve({ port: 0 })for the remote-tarball case, VerdaccioRegistry for npm packages, pipes drained concurrently, content asserted before exit codes. The PR description confirms each test fails on the unfixed build and that the patch tests fail independently with only the installer fix applied. - No prior review comments; no CODEOWNER-gated paths.
|
Updated 3:56 AM PT - Aug 11th, 2026
✅ @robobun, your commit 26900a1f1843a7f5bec01d6817e610a428c306fd passed in 🧪 To try this PR locally: bunx bun-pr 37469That installs a local version of the PR into your bun-37469 --bun |
|
The |
) ### Problem - `bun pm ls --all` (and `bun list --all`) aborts with `panic: buf_print: buffer too small` (exit 134) as soon as it reaches a package whose resolution is longer than 512 bytes, for example a dependency installed from a tarball URL with a ~600 character path. Plain `bun pm ls` prints the same package fine. - Reproduced with bun 1.4.0-canary.1 and main: serve a `.tgz` from a local `Bun.serve`, depend on it as `http://127.0.0.1:PORT/aaaa...(600 chars)/bar-0.0.2.tgz`, `bun install` (succeeds), `bun pm ls --all` prints the root line and `├── ` and panics. - Cause: the `--all` tree printer (`print_node_modules_folder_structure`, `src/runtime/cli/package_manager_command.rs`) formatted each resolution into `let mut resolution_buf = [0u8; 512]` with `buf_print_infallible`, which panics on overflow. Two sites: the header line of a package that owns a nested `node_modules` folder (formerly line 764) and the line of every other package (formerly line 890). - A tarball, folder or git resolution repeats the user supplied spec verbatim, so there is no length a stack buffer can be sized to. #37469 fixed the same class of panic in the installer and `bun patch`; this printer was left out. ### Fix - Pass the resolution's `Display` formatter (`resolutions[id].fmt(string_bytes, PathSep::Auto)`) straight to `prettyln!` at both sites and delete the buffer and the now unused `buf_print_infallible` import. - Correct because the formatted bytes were only ever printed: the buffer added no information, and removing it removes the only length limit on the path. - Output is unchanged for every label that used to fit. The old code formatted the same `Display` impl into the buffer and re-printed it through `BStr`, and `BStr` prints the valid UTF-8 that `Display` produces byte for byte, so the only observable difference is on labels that used to panic. - This is already how the other two `pm ls` paths print the label: the top-level listing (`bun pm ls`) and `--all --trusted` (`print_trusted_dependencies_flat`) both pass the formatter directly, which is why they never had the problem. - Verified with `test/cli/install/bun-pm.test.ts`, "should list all dependencies when a resolution is longer than 512 bytes": the root depends on two tarballs with ~620 byte URLs plus `baz@0.0.5`; one tarball (`moo`) depends on versions of `bar` and `baz` that conflict with the root's, so it owns a nested `node_modules` and its label goes through the header site, while the other tarball (`bar`) goes through the per-package site. The test asserts the full tree. It fails on the unfixed binary with the panic above and passes with the fix; the rest of `bun-pm.test.ts` and `test/regression/issue/24502/bun-pm-ls-all-invalid-package-id.test.ts` still pass. `cargo fmt --check` and `cargo clippy -p bun_runtime` are clean. ### Background - A package's resolution is the lockfile's record of what it was installed from: a version for registry packages, otherwise the spec itself (tarball URL or path, folder path, git reference). `Resolution::fmt` returns a `Display` formatter that renders it as the `@...` label `bun pm ls` prints after each package name. - `bun pm ls --all` walks the lockfile's hoisted tree one `node_modules` folder at a time. Each folder's header names the package that owns it, and each package inside the folder that does not own a folder of its own gets a line of its own; those are the two print sites. - `buf_print_infallible` (`src/bun_core/fmt.rs`) formats into a caller supplied slice and panics if the output does not fit; it is meant for buffers whose contents have a known bound. The Zig version of this code used `std.fmt.bufPrint` with `try`, so the same overflow used to surface as an error instead of a panic, but `--all` failed either way.
2 participants
Repro
bun installaborts (SIGABRT, exit 134) after the tarball has been resolved and extracted. The spec normalizes to./bar-0.0.2.tgz, which exists; a real 618 byte relative path behaves the same (that is how this was found, see #37462). The same abort happens for a remote tarball whose URL is longer than 512 bytes, afile:folder at a long path, and a workspace package whose version has a ~500 byte prerelease tag.--lockfile-onlysucceeds, so it is the link step. Reproduced withbun 1.4.0-canary.1and main.Top frame:
Result::expectinPackageInstaller::install_package_with_name_and_resolution(src/install/PackageInstaller.rs:1340), called frominstall_package/hoisted_install::install_hoisted_packages.Cause
While linking each package, the hoisted installer formats the package's version label into
let mut resolution_buf = [0u8; 512]withbuf_print(..).expect("unreachable"). For npm packages the label is the version; for tarball, folder and git packages it is the spec they were resolved from (stored verbatim), and for workspace packages it is the workspace's own version. Those are user supplied and have no length bound, so the overflowbuf_printreports is reachable and theexpectturns it into the panic. Both branches (workspace version and resolution) have it.The isolated linker already builds this label in a
Vec(Installer::package_patch_info) and is not affected by this panic. It fails such installs withENAMETOOLONGinstead, because the store directory name embeds the spec; that is a separate problem and not touched here.Fix
print_package_versionkeeps the 512 byte stack buffer as the allocation-free path and, only when the label does not fit, formats it into aVecthat lives next to the buffer (the same shape asresolve_path::join_z_buf_spill). The linking loop runs once per installed package, so the common case still does not allocate.Spilling is the only correct behavior for this label: it is compared against the installed
package.jsonversion and hashed as the version half of thename@versionpatchedDependencieskey, so a truncated label would silently fail verification or miss a patch, and rejecting the package would refuse a valid install (nothing on disk is named after the label; the tarball cache folder is a hash of it). The new patchedDependencies test below checks the spilled label byte for byte by keying a patch with it.bun patchformats the same labels into 1024 byte buffers with the sameexpectat four sites insrc/install/PackageManager/patchPackage.rs, all reproducible the same way once the install succeeds (verified with a build that only had the installer change):pkg_info_for_name_and_version(bun patch <name>@<version>compares the label of every package with that name; panicked withResolution name too long),prepare_patchanddo_patch_commit(bun patch <path>when another package with the same name has a long label),name@labelkey indo_patch_commit, which is also the patch file name.These format into a
Vec(print_resolution_label, reused across a candidate loop). Indo_patch_committhe keyVecis returned as thepatch_keydirectly and the file name is built from it, replacing the re-slicing of the shared buffer; the bytes are unchanged (the key was already valid UTF-8, so the formerBStrround trip was a copy). Committing a package whose label is this long still cannot succeed, since the patch file would be named after the label, but it now exits 1 with an error instead of aborting; today it fails before that at the diff step for tarball packages (#37124), which this does not change either way.Tests
test/cli/install/bun-workspaces.test.ts, "packages whose version label is longer than 512 bytes" (hoisted linker): local tarball, remote tarball served from a localBun.serve, workspace package with a long prerelease version (the other branch), and apatchedDependenciesentry keyed by the long spec that must actually be applied.test/cli/install/bun-patch.test.ts, "packages whose label is longer than 1024 bytes":bun patch <name>@<label>,bun patch <path>plus--commitwhen a same-named package has a long label (the long one is listed first, so the loops format it), and--commitof a long-labeled package exiting 1 without touchingpackage.json. The long specs usex/../repeated so nothing long is ever created on disk.All seven fail on the unfixed build (
panic: unreachable: Errorduring the install) and pass with the fix; with only the installer change applied, the threebun patchtests fail at thebun patchstep instead, so each patch site is covered on its own. Both files pass in full, as doesbun-install-patch.test.ts;cargo clippy -p bun_installis clean andcargo check -p bun_installpasses forx86_64-pc-windows-msvcandx86_64-apple-darwin.