pack/publish: stop panicking on package.json bin and files entries longer than the path buffer - #38784
pack/publish: stop panicking on package.json bin and files entries longer than the path buffer#38784robobun wants to merge 4 commits into
Conversation
…nger than the path buffer
|
Warning Review limit reached
Next review available in: 28 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 (5)
Comment |
|
Status: ready for review. On both builds of this branch (the original commit and the current head) all 177 lanes that ran are green; the only jobs without a result are the two Reproduced on 1.4.0-canary.1 (b7a0431) with a 5000 byte With this branch the 10 new tests in |
There was a problem hiding this comment.
LGTM — mechanical crash fix following the existing *_spill pattern in resolve_path.rs.
What was reviewed:
- Verified the
str.len() + 2size bound innormalize_buf_or_spillcovers every growth case (empty→.,C:→C:., bare UNC volume +sep, plus the_zNUL); traced Posix normalization to confirm output never exceeds input+1. - Checked all seven call sites clone the borrowed result (
ZBox::from_bytes,Box::from,cli_dupe) before the next loop iteration reborrowspath_buf/path_spill. - Confirmed
path::platform::Posixresolves via the crate-root re-export inbun_paths/lib.rs, and the two new highway test stubs match the real kernels' not-found conventions (haystack_len/usize::MAX). - The
ENAMETOOLONGaddition to thedirectories.binopen-error downgrade is scoped to the one branch that needs it; other errnos still fail loud.
Extended reasoning...
Overview
This PR fixes a slice-index panic in bun pm pack and bun publish when package.json bin/directories.bin/files entries exceed MAX_PATH_BYTES. It touches src/paths/resolve_path.rs (adds normalize_buf_spill / normalize_buf_z_spill plus unit tests and two highway link stubs), src/runtime/cli/pack_command.rs (three sites in get_package_bins, one in the files loop), src/runtime/cli/publish_command.rs (four sites in normalize_bin plus one ENAMETOOLONG errno match), and adds integration tests to both bun-pack.test.ts and bun-publish.test.ts.
Security risks
None. The change replaces a fixed-size buffer write that panicked on overflow with a heap fallback. No new trust boundaries, no path-traversal surface (the normalized result feeds into the same bin_path_escapes_root / Pattern::from_utf8 / exists_at checks as before). The files case is a glob, not a filesystem path, so accepting arbitrary length is correct rather than a new risk.
Level of scrutiny
Medium. It's a crash fix in CLI-only code (not the runtime hot path), and the shape is a direct copy of the established join_z_buf_spill / normalize_string_spill pattern already in the same file. The main thing to get right is the size bound, which I traced through normalize_buf_t → normalize_string_generic_tz for both Posix (all callers) and Windows: normalization is byte-removing except for the three documented +1 cases, and the shared helper reserves +2 to cover the _z NUL on top. The unit tests pin fits/spills/exact-length/empty, and the integration tests use 100 KB inputs so they exceed the buffer on every platform.
Other factors
- All call sites immediately clone the returned slice (
ZBox::from_bytes,Box::<[u8]>::from,cli_dupe) before the next iteration mutably reborrowspath_buf/path_spill, so there's no aliasing concern; the borrow checker would have caught it anyway since the code compiles. - The two new
#[unsafe(no_mangle)]test stubs (highway_last_index_of_char,highway_memrmem16) are needed becausenormalize_buf_treacheslast_index_of_separator_*which links against those kernels; they match the not-found conventions used by the real wrappers (found_attreatshaystack_lenas miss,match_attreatsusize::MAXas miss) and mirror the existing precedent insrc/parsers/native_test_shims.rs. - The
path::platform::Posixtype-path change inpack_command.rsresolves viabun_paths/lib.rs:335(pub use resolve_path::{.., platform}) and the existinguse bun_paths::{self as path, ..}alias. - The PR description documents that the whole test files pass (81 / 44), plus
cargo test -p bun_pathsand Miri. No CODEOWNERS entries cover these paths, and there are no outstanding human review comments.
There was a problem hiding this comment.
LGTM — well-scoped panic fix following the established *_spill pattern.
What was reviewed:
- The
+2size bound innormalize_buf_or_spillagainstnormalize_buf_t's growth cases (empty→.,C:→C:., UNC trailing sep, plus NUL) — correct, and all eight call sites useplatform::Posixwhere output ≤ input anyway. - Import/alias resolution:
path::platform::Posixresolves viabun_paths::platformre-export atsrc/paths/lib.rs:335; the droppednormalize_buf/normalize_buf_zimports have no remaining callers in either file. - The
ENAMETOOLONG→ warning downgrade innormalize_bin'sdirectories.binbranch is scoped to that one errno and matches the existingENOENTtreatment; other errors still crash.
Extended reasoning...
Overview
Fixes a panic in bun pm pack / bun publish when package.json bin, directories.bin, or files entries exceed the platform PathBuffer length. Adds normalize_buf_spill / normalize_buf_z_spill to src/paths/resolve_path.rs (thin wrappers that pick the caller's stack buffer or grow a caller-owned Vec), swaps the 8 affected call sites in pack_command.rs (3 in get_package_bins, 1 in the files loop) and publish_command.rs (4 in normalize_bin), adds ENAMETOOLONG alongside ENOENT in the directories.bin open-error branch, and adds unit tests plus two highway kernel test stubs. Integration tests cover all five input shapes across both commands.
Security risks
None. The values were already being read from package.json and passed downstream; they just panicked on length. After the fix, over-long bins hit the existing "bin does not exist" path (they cannot exist on disk), and over-long files globs are matched as before. No new data flows to the registry or filesystem beyond what shorter inputs already produced. The ENAMETOOLONG downgrade only widens which errno triggers the existing "does not exist" warning; other open errors still fail the publish.
Level of scrutiny
Medium. bun publish writes to registries, but this change only affects how oversized package.json strings are normalized before the existing existence checks / glob matching, and the downstream behavior for a nonexistent bin was already correct. The one non-mechanical piece is the str.len() + 2 size bound; I traced normalize_buf_t → normalize_string_buf_t → normalize_string_generic_tz and confirmed it never grows output beyond input+1 (empty→., drive-relative C:→C:., bare UNC volume gains a trailing separator; ADD_NT_PREFIX is never set from this path), and the _z variant writes one NUL. The bound matches the neighboring normalize_string_spill's +1 (which has no NUL variant). Unit tests pin the exact-length and empty→. boundaries.
Other factors
- Follows the established
join_z_buf_spill/normalize_string_spillpattern already inresolve_path.rs; the PR description notes #37526 took the same approach for the browser-map field. - All eight sibling sites are covered in one PR ("fix the whole class"); the spill
Vecis hoisted per-function so the fits-in-buffer path is unchanged. - Tests use 100 KB inputs to exceed the Windows buffer (98302 bytes) so no platform gating is needed; the publish tests build the tarball with
Bun.Archiveso they don't depend on the pack fix; the--dry-run+ userinfo-in-URL trick matches an existing test in the file. - CI reported green on 177 lanes; the only red was a darwin queue expiration, since re-run. The comment-cop threads (long comments) were addressed in e6d9e19 / 5e5823b and are all resolved.
- The two new
#[unsafe(no_mangle)]highway stubs follow the exact shape of the existing ones in the same test module and are only linked intocargo test -p bun_paths.
Problem
bun pm packandbun publishabort withpanic: range end index 5000 out of range for slice of length 4096(exit 134 plus a crash report) when package.json has abinstring, abinobject value, or adirectories.binlonger than the path buffer (4096 bytes on Linux, 1024 on macOS, 98302 on Windows).bun pm packdoes the same for afilesentry of that length, andbun publish <tarball>also for abinobject key.resolve_path::normalize_buf/normalize_buf_zinto a stackPathBuffer, and the normalizer indexes past the end of whatever buffer it is given (src/paths/resolve_path.rs,buf[buf_i..buf_i + count].copy_from_slice(..)innormalize_string_generic_tz). Sites:get_package_bins(three) and thefilesloop inpack()insrc/runtime/cli/pack_command.rs;normalize_bin(four) insrc/runtime/cli/publish_command.rs.normalize_binruns forbun publish <tarball>too, which never goes through pack, so both files need the change.Fix
resolve_path::normalize_buf_spillandnormalize_buf_z_spill: normalize into the caller's buffer when the result is known to fit, otherwise into a caller-ownedVecgrown to size. Same shape as the existingjoin_z_buf_spill, and the same approach resolver: don't abort on a package.json browser map key longer than 1024 bytes #37526 took for the browser map, which is this bug in another package.json field. The size bound isinput + 2: normalizing only removes bytes, except that""becomes"."and two Windows spellings grow by one byte, and the_zvariant appends a NUL. Unit tests inbun_pathspin the fits / spills / exact-length / empty cases.Vecper function hoisted out of the loops, so inputs that fit are handled exactly as before and longer ones just land in theVec.openat/open_dir/fstatatreturnENAMETOOLONGfor these, I checked the POSIX and Windows wrappers), publish prints its existingbin '...' does not existwarning and keeps the entry in the registry metadata like npm does. Afilesentry, on the other hand, is a glob, and a long one (a brace group with many names) is legitimate and has to keep matching; cutting it off would be wrong, so it is not a "bad input" case at all.normalize_bin'sdirectories.binbranch only downgradedENOENTto the warning;ENAMETOOLONGnow takes the same branch, since likeENOENTit means nothing exists at that path (other errors, where something exists but cannot be opened, still fail the publish). Without this line thedirectories.bintarball test below exits 1 withfailed to open bin directory.test/cli/install/bun-pack.test.ts:bins > longer than the path buffer(string, object value,directories.binare skipped; a normal bin listed next to a long one is still packed executable) andfiles > an entry longer than the path buffer is still matched(a 100 KB brace glob still selectsdist/). The values are 100,000 bytes so the tests also exceed the Windows buffer and are not platform gated. These 5 panic on the current release and pass with this change; the whole file passes (81).test/cli/install/bun-publish.test.ts:bin longer than the path buffer, publishing a tarball (built withBun.Archiveso the test does not depend on the pack fix) with each of the four shapes, asserting the exact set of warnings, plus a directory publish.--dry-runstops before the registry is contacted; the userinfo in the registry URL only satisfies the auth check, the same trick an existing dry-run test uses. These 5 panic on the current release and pass with this change; the whole file passes (44, run with a raised timeout because the debug build is too slow for the file's lifecycle-script tests at the default 5s).cargo test -p bun_paths(22 pass) andbun run rust:miri -p bun_paths(22 pass).--destination) and publish: report ENAMETOOLONG for a tarball path that does not fit the path buffer #38753 (thebun publish <tarball>argument) fix the other two length panics in these commands; they are different buffers and different inputs. publish: report ENAMETOOLONG for a tarball path that does not fit the path buffer #38753 also edits theuse bun_paths::resolve_path::{..}line inpublish_command.rs, so one of the two will need the one-line import merge (union of both lists). Against the current heads of pack: pack a file listed under several bin names once, strip the trailing slash from directories.bin #38720 and pack: skip bins reached through symlinks; publish: do not read the readme through a symlink #38707, which edit other lines ofget_package_bins,src/merges cleanly and only the test additions at the end ofdescribe("bins")conflict.bun installof a dependency whose bin value is this long still aborts, in the bin linker'sjoin_abs_string_z(src/install/bin.rs,resolve_bin_target), a different buffer reached from the lockfile rather than from a package.json being packed. Reproduced separately and filed for its own fix.Background
PathBufferis a[u8; MAX_PATH_BYTES]stack scratch buffer used for paths about to be handed to the OS.MAX_PATH_BYTESis the platformPATH_MAX, which is why the threshold differs per OS. Strings read out of package.json have no such bound.resolve_path::normalize_bufispath.normalizeinto a caller-provided buffer: collapses./,//and.., and here also converts\to/(thePosixplatform parameter), which is why pack and publish run bin paths through it before comparing them against tarball entry names. It assumes the buffer is big enough; the*_spillfunctions inresolve_path.rsare the family of wrappers that remove that assumption by falling back to a heapVec, which stays empty unless it is needed.get_package_binsreadsbin/directories.bin, file bins are pushed onto the pack queue as optional entries (dropped if they cannot be opened) and a bin directory is walked if it can be opened. publish additionally rewritesbininto the object form npm registries expect (normalize_bin), which is where it warns about bins that do not exist;bun publish <tarball>only runs this second part, on the package.json found inside the tarball.Repro on the current release (1.4.0-canary.1)
Same result for
bin: {x: <long>},directories: {bin: <long>},files: [<long>](pack) andbin: {<long>: "x.js"}(tarball publish). With this change all of them exit 0; the publish variants printwarn: bin '<long>' does not exist/warn: bin directory '<long>' does not exist.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