install: stop panicking on bin values longer than the path buffer - #38954
install: stop panicking on bin values longer than the path buffer#38954robobun wants to merge 7 commits into
Conversation
The bin linker joined each bin target onto the package directory with join_abs_string_z, which writes into a fixed 4096 byte buffer without a bounds check, so a dependency whose package.json bin value (or directories.bin) did not fit aborted bun install, bun link and bun unlink. The joins of a bin directory's entries and of the temporary file used to rewrite a CRLF shebang overflowed the same way once the directory itself was close to PATH_MAX. Join into a PathBuffer with join_abs_string_buf_checked instead. A path that does not fit takes the branch the sys layer already takes for such a path: file targets are skipped like a missing bin (sys::exists reports them missing), a bin directory is reported as ENAMETOOLONG (what sys::open_dir_absolute returns for it), and the shebang rewrite, which ignores every other failure, is skipped.
|
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 (8)
Comment |
|
Status: reproduced and fixed.
|
|
Updated 12:05 PM PT - Aug 15th, 2026
❌ @robobun, your commit eecb876 has some failures in 🧪 To try this PR locally: bunx bun-pr 38954That installs a local version of the PR into your bun-38954 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it refactors resolve_bin_target's control flow and changes buffer ownership in the bin linker (a path that runs on every install and sits next to the CVE-2019-16775 traversal guards), a human look would still be worthwhile.
Checked: the candidates loop preserves the original probe order and empty-skip conditions; join_z_checked reserves the NUL byte correctly against join_abs_string_buf_checked; the pooled resolved_target_buf outlives every abs_target borrow so the removed ZStr::from_raw detaches are sound to drop; bin_target_escapes_package_dir still runs before every changed join. Tests cover File/NamedFile/Map/Dir × both linkers, lockfile round-trip, the normalizing-long-value case, and bun link/unlink.
Extended reasoning...
Overview
This PR replaces six unchecked path joins in src/install/bin.rs (Linker::resolve_bin_target, the Dir arms of link/unlink, the per-entry join inside a bin directory, and the CRLF-shebang temp file) with a single join_z_checked helper built on the existing resolve_path::join_abs_string_buf_checked. resolve_bin_target now returns Option<&ZStr> and writes into a caller-supplied pooled PathBuffer instead of the thread-local join buffer, which lets three unsafe { ZStr::from_raw(...) } lifetime detaches in link and one raw-pointer package_dir detach in unlink be deleted. ~260 lines of new tests in bun-install-registry.test.ts cover every bin shape across both linkers, plus bun link/unlink and a POSIX-only near-PATH_MAX directory.
Security risks
Bin linking is security-adjacent: it creates symlinks into .bin and lchmods targets, and this file already carries the CVE-2019-16775 traversal guard. I verified bin_target_escapes_package_dir and bin_target_needs_resolved_containment_check still gate every changed call site before the join runs, and that join_z_checked normalizes (so the long-but-normalizes-short test case does not open a bypass). The change narrows behavior — a value that used to panic now takes the same branch the OS's ENAMETOOLONG/missing-file answer would have taken — rather than accepting anything new. No new unsafe is added; four unsafe blocks are removed.
Level of scrutiny
Medium-high. The fix itself is a straightforward "use the checked join" swap, but it comes with a control-flow refactor of resolve_bin_target (sequential ifs → candidate array + loop) and a buffer-ownership change (thread-local → pooled local). I traced the candidate order (target, bin_name, basename-if-has-dir-component, bin_name+.exe) against the old code and it matches, including the fall-through-to-primary when nothing exists; the ZStr::from_buf(buf, len) re-derivation inside the loop is the standard NLL workaround and the NUL at buf[len] is written by join_z_checked. The pooled buffer's PoolGuard lives for all of link(), so abs_target never dangles. Still, this runs on every bun install and the refactor is non-mechanical enough that a maintainer should confirm the shape.
Other factors
The PR description is unusually thorough — every outcome is justified against what the OS already reports for a shorter-but-still-rejected path, and the test matrix is comprehensive (including the second-install-from-lockfile round trip and the near-limit directory that exercises the entry-join and shebang-temp-file overflows). The bug hunting system found nothing. The one behavioral nuance worth a maintainer's eye is that in the native-binlink-redirect path, exe_name is now allocated unconditionally (as an empty Vec when unused) rather than lazily; this is cold-path and harmless but is the kind of micro-shape a reviewer might want to weigh in on.
XFS rejects symlink targets of 1 KiB or more, so the Alpine lanes failed to link the entries that fit. Put most of the depth into the project directory instead of the package so the relative link targets stay short while the bin directory still ends up 64 bytes below the limit.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it refactors buffer ownership and unsafe lifetime handling in the bin linker (a hot path shared by every install, bun link, and bun unlink), a human look would still be worthwhile.
What was reviewed:
join_z_checkedreserves one byte for the NUL andjoin_abs_string_buf_checkedbounds the result tobuf.len()-1, sobuf[len] = 0is always in bounds;ZStr::from_bufinvariants hold.resolve_bin_targetcandidate order and gating (target → bin_name → basename-only-if-target-has-a-dir → bin_name.exe) is preserved;sys::exists_zis whatsys::existsalready delegated to.- The pooled
resolved_target_bufis disjoint fromself, so the removedZStr::from_rawdetaches forabs_targetare no longer needed; the remainingfrom_rawin theTag::Direntry loop still points intoabs_target_bufand its SAFETY note is unchanged. Noneoutcomes route to the same branches the OS'sENAMETOOLONGalready reached (skipped_due_to_missing_binfor file targets,self.errfordirectories.bin, silent give-up for the shebang temp file).
Extended reasoning...
Overview
The PR replaces six unchecked path joins in src/install/bin.rs (resolve_bin_target, the Tag::Dir arms of link/unlink, the Tag::Dir entry loop, and the CRLF-shebang temp file) with a single join_z_checked helper that uses resolve_path::join_abs_string_buf_checked and returns None when the normalized result would not fit a PathBuffer. resolve_bin_target is refactored from an unrolled sequence into an array of candidates and now writes into a caller-supplied buffer (a pooled PathBuffer in link, a fresh pool buffer in unlink's Dir arm) instead of the thread-local join buffer, which lets three unsafe ZStr::from_raw lifetime detaches be dropped. Six new tests in bun-install-registry.test.ts cover the string/single-entry/map bin shapes plus a normalizing-to-short value across both linkers and from the lockfile, directories.bin across both linkers, a bin directory near PATH_MAX (POSIX-only), and bun link/bun unlink.
Security risks
None introduced. The change tightens bounds checking on untrusted bin values from package.json/registry manifests/lockfiles — previously a value ≥4 KiB triggered a slice-index panic (a DoS on any project depending on such a package). The existing path-traversal guards (bin_target_escapes_package_dir, resolved_target_parent_escapes_package_dir) run before/after the join and are untouched. The None path maps to "treat as nonexistent" for file targets (matching npm's bin-links) and ENAMETOOLONG for directories.bin (matching what open_dir_absolute already returned for a fitting-but-too-long path).
Level of scrutiny
High. This is the bin linker shared by hoisted install, isolated install, bun link and bun unlink; it runs for every package with a bin field. The file carries substantial unsafe (raw-pointer detaches for disjoint borrows, ZStr::from_raw, union field reads) and the PR both adds a pooled buffer to link() and rewrites the candidate-probing loop. I verified the helper's bounds arithmetic, that sys::exists_z is semantically identical to the previous sys::exists on these inputs, that the candidate order/gating is preserved bit-for-bit, and that each None branch reaches the same terminal state the OS would have produced — but the combination of buffer-ownership reshuffling and control-flow refactor in memory-safety-adjacent code warrants a maintainer's eyes.
Other factors
- The comment-cop bot flagged long comments on earlier commits; the author trimmed them in
ea679068ande70b86b9and all eight threads are marked resolved. - Test coverage is thorough: each shape × each linker, lockfile round-trip, the near-
PATH_MAXdirectory case that exercises the entry join and shebang-tmpfile join, andbun link/bun unlink. The author reports the rest ofbun-install-registry.test.ts,bun-install-native-binlink.test.ts,symlink-path-traversal.test.ts, andshebang-normalize.test.tspass on the debug build. - The pooled buffer is acquired once per
link()call (not per entry), so no per-bin allocation overhead in theMap/Dirloops.
|
Follow-ups since the PR was opened:
|
The .bin link is relative to the bin directory, and each component of a global bin directory that is not shared with the target adds a "..", so the link target can be longer than the target's absolute path. When the absolute path was within a few bytes of the buffer, relative_buf_z overflowed rel_buf and bun link / bun add -g aborted. Compute into a heap buffer when the upper bound does not fit; symlink(2) then reports ENAMETOOLONG for a target that is really too long, like any other link error.
…e path buffer Adds a 4.0.0 shape to the altpath fixtures: the parent's bin value is 8 KiB, so the first candidate cannot be built and the redirect has to move on to <pkg>/<bin_name>. Only the 4.0.0 tarballs and manifest entries are added; the existing fixtures are unchanged.
|
Two additions after going over the change again (PR description updated accordingly):
|
Same XFS limit as before: the depth now goes into the global directory, which both the bin directory and the target share, so the link that is meant to be created has a 765 byte target while the link itself is still 20 bytes below the limit.
|
eecb876 fixes the one CI failure from the previous push (the review above spotted the same thing): the new |
1 participant
Problem
bun installaborts withpanic: range end index 5038 out of range for slice of length 4095(exit 134 plus a crash report) when a dependency's package.json has abinvalue longer than 4 KiB: the string form, a value in the object form, ordirectories.bin. Both linkers are affected, and so arebun linkandbun unlinkfor a package with such a value. Repro below.Linker::resolve_bin_target(src/install/bin.rs:1479-1519 on main) and thedirectories.binarms oflink(bin.rs:1772) andunlink(bin.rs:1940) join the value onto the package directory withresolve_path::join_abs_string_z, which normalizes into a fixed 4096 byte thread-local buffer with no bounds check (normalize_string_generic_tzin src/paths/resolve_path.rs, top frame of the crash). The linker length-checks bin names (the.bin/<name>side) but never the value, and values are stored as written, from the registry manifest or the lockfile.PATH_MAX: every entry of the directory is joined onto it intoabs_target_buf(bin.rs:1805), and the temporary file for rewriting a CRLF shebang, 27 bytes longer than the target, is joined the same way (bin.rs:1045). Panicsrange end index 4160and4111respectively with the test below.create_symlink(bin.rs:1266) computes the relative link target intorel_buf, anotherPathBuffer, unchecked. The relative form gains a..per component of the bin directory that the target does not share, so with a global bin directory (BUN_INSTALL_BIN,install.globalBinDir) on another branch of the tree it can be longer than the absolute target; a target within a few bytes of the limit that the joins above accept then abortsbun link/bun add -gwithrange end index 4198 out of range for slice of length 4096. Pre-existing as well, found while reviewing this change.Fix
Linker::join_z_checked, which uses the existingresolve_path::join_abs_string_buf_checkedinto aPathBuffersized buffer and returnsNonewhen the normalized path does not fit. The resolved target gets a pooled buffer, becauseabs_target_bufholds the package directory the value is joined onto.resolve_bin_targetreturnsOptionaccordingly.PathBufferis exactly the size thebun_syswrappers copy a path into, so "does not fit" is precisely the set of paths they already refuse (sys::existsreturns false for them,sys::open_dir_absolutereturnsENAMETOOLONG). Each caller takes the branch it takes for that answer, one step earlier, so a value that does not fit now behaves exactly like a value that fits but that the OS rejects (for example a 300 byte single component) behaves on the current release:lstat.directories.bin:ENAMETOOLONG, which thelinkarm already reports for every open failure other thanENOENT. Hoisted printserror: Failed to link <pkg>: ENAMETOOLONGand exits 1 with everything else installed and linked, isolated prints itsfailed to link binaries for packageline,bun linkprintsfailed to link bin due to error ENAMETOOLONG, andbun unlink(which ignores linker errors) still unlinks the package.x/../x/../.../cli.js, 100 kB as written) still links; the test pins this, it passes before and after..exe); a candidate that does not fit is skipped and the next one is probed, and when nothing is found the usual retry without the redirect happens. Because the result now borrows a local buffer instead of the thread-local one, the unsafe lifetime detaches forabs_targetinlinkand forpackage_dirinunlinkare gone.create_symlinkcomputes an upper bound for the relative target (target length + 3 per component of the bin directory) and uses a heap buffer instead ofrel_bufwhen that bound does not fit, which only happens with a target within a few hundred bytes of the limit. Nothing else changes: a relative target that really is too long is handed tosymlink(2), which reportsENAMETOOLONGlike any other link error (bun linkprintsfailed to link bin due to error ENAMETOOLONG), and one that fits after all is created. The Windows shim writer has the same call, but its buffer is about 96 KiB, larger than any target NTFS can hold unless the whole path is 3 byte characters, and install(windows): store an absolute target in .bunx shims when the package dir is on another drive #38018 is reworking that code, so it is left alone.test/cli/install/bun-install-registry.test.ts, new blockbinaries > bin values longer than the path buffer(7 tests): the three file shapes plus the normalizing value, with each linker, installed twice (the second time from the lockfile);directories.binwith each linker; a bin directory 64 bytes below the limit holding a short entry (linked), a CRLF entry whose temp file would not fit (linked, shebang left alone) and an entry that does not fit (skipped);bun linkwith a bin directory below the global directory (the bound is exceeded but the target fits, so it is linked, and the link is checked) and then with one far enough away that the target does not fit (ENAMETOOLONG, exit 1, nothing linked);bun linkandbun unlinkof a package with each shape. All seven fail on the current release (six panic in the joins, thebun linkone with thelength 4096panic above) and pass with this change. The values are 100 kB so the Windows buffer is exceeded as well; the two near-the-limit tests are POSIX only, since the Windows buffer is larger than any path the filesystem accepts, and they keep most of the depth in the project directory rather than inside the package because the.binlinks are relative and XFS (the Alpine CI lanes) rejects symlink targets of 1 KiB or more.test/cli/install/bun-install-native-binlink.test.tsgets a fourth altpath shape (fixture version 4.0.0, only the new tarballs and manifest entries are added): the parent's bin value is 8 KiB, so in redirect mode the first candidate cannot be built and the bin has to be linked from<platform package>/<bin name>. Both linker variants panic on the current release (range end index 8271) and pass with this change; on Windows the value fits and the shape degrades to an ordinary miss of the first candidate.bun pm pack/bun publishand left the install side for a separate change; Bun.build: report an error for HTML rooted script src paths >= 4096 bytes instead of aborting #35860 and bun test: stop panicking on a path argument or tree entry longer than the path buffer #35863 change the shared join primitive. This change does not depend on them and is unaffected by them: it sizes the path before the primitive runs and reports what the OS would have reported, instead of handing an oversized or emptied path on to the next syscall.Background
bin,Linker::linkin src/install/bin.rs createsnode_modules/.bin/<name>(a.bunxplus.exeshim on Windows, or a global bin) pointing at<package dir>/<value>. After parsing,binhas one of four shapes: a string (File), a one entry object (NamedFile), a larger object (Map), ordirectories.bin(Dir, which links every file in that directory). The hoisted and isolated installers,bun linkandbun unlinkall share this code.PathBuffer/MAX_PATH_BYTES: bun's buffer for a path that is about to be handed to the OS, sized to the platformPATH_MAX(4096 on Linux, 1024 on macOS, about 96 KiB on Windows). Thebun_syswrappers that accept a byte slice copy it into one and refuse anything longer.join_abs_string_z, the join used here before, instead writes into a 4096 byte thread-local on every platform and assumes the result fits.join_abs_string_buf_checkedis the join variant for parts of arbitrary length: it normalizes (into a heap scratch when necessary) and returnsNoneonly when the normalized result does not fit the caller's buffer. It is what folder dependency paths (lockfile/Package.rs) and, since install: stop panicking on workspaces entries longer than the path buffer #37531, workspace entries use.resolve_bin_targetjoins several candidate paths and checks which exists.../<pkg>/<value>for a project, whateverpath.relative(bin dir, target)gives for a global bin directory), computed byresolve_path::relative_buf_zinto the caller's buffer; that is the computation thecreate_symlinkbullet is about.skipped_due_to_missing_bin: set by the linker when a target does not exist on disk. The installers use it to retry once without the redirect and otherwise ignore it, which is what makes "treat an unbuildable path as missing" fit in without new plumbing.Repro on the current release (1.4.0-canary.1) and output with this change
Release:
panic: range end index 5038 out of range for slice of length 4095, exit 134, with--linker isolatedas well. Same withbin: longand withdirectories: { bin: long }, and forbun link/bun unlinkrun insidedep.With this change:
bin: long,bin: { cli: long },bin: { cli: long, ok: "ok.js" }oklinkedoklinkedoklinkeddirectories: { bin: long }error: Failed to link dep: ENAMETOOLONG, exit 1, other packages and bins installedENAMETOOLONG: failed to link binaries for package: dep@../dep, exit 1error: failed to link bin due to error ENAMETOOLONG, exit 1;bun unlinkafterwards succeedsThis is the same output the release produces today for
directories: { bin: "b".repeat(300) }orbin: { cli: "b".repeat(300) }, where the OS is the one rejecting the path.