install: fail the package with ENAMETOOLONG when the isolated linker walks an entry that does not fit the path buffer - #37424
Conversation
…ntry does not fit the path buffer The isolated linker's Hardlinker and FileCopier append each walker entry to CheckLength::ASSUME paths, so an entry longer than the remaining buffer panics in Buf::append instead of failing the package. Build them on length-checked paths (Path::into_checked) and report the overflow as ENAMETOOLONG, guard the cwd + dest join on Windows the same way, and propagate the errors from creating a directory entry and from fstat on a source file that were previously discarded.
|
Status: reproduced on bun 1.4.0-canary.1 (9008ae7) with the repro in the description ( |
WalkthroughThe installer now converts paths to checked-length types. File copying and hardlinking propagate ChangesChecked install path validation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install/isolated_install/FileCopier.rs`:
- Around line 171-179: Update the retry branch in FileCopier to propagate the
result from bun_sys::make_path::make_path::<u16> instead of discarding it with
let _. Return immediately when directory creation fails, and only retry
bun_sys::copy_file::copy_file after successful creation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 978ae1e1-0c41-43ec-b71f-8e3a9b799756
📒 Files selected for processing (7)
src/install/PackageManager/patchPackage.rssrc/install/isolated_install/FileCopier.rssrc/install/isolated_install/Hardlinker.rssrc/install/isolated_install/Installer.rssrc/paths/Path.rssrc/paths/lib.rstest/cli/install/isolated-install-long-paths.test.ts
![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 changes error-propagation behavior in the isolated installer's link/copy loops, adds public surface to bun_paths (into_checked, CheckLength::CHECK, NT_OBJECT_PREFIX), and touches Windows-specific unchecked-buffer arithmetic, a human look would still be worthwhile.
What was reviewed:
into_sep::<AUTO>→into_checked()at the fourInstaller.rscall sites keeps separator behavior (OsAutoAbsPath/OsAutoPath/dest_subpathare alreadyPathSeparators::AUTO).- Checked
appendreturnsErrbefore mutating, so the post-bodyset_length(saved_len)restore is correct even when only the first of two appends succeeds. - Windows
needed_lenbound (Σ(part.len()+1) + NT_OBJECT_PREFIX.len()) covers the join separator, NUL, and\\??\\prefix thatjoin_string_buf_w_same+add_nt_path_prefix_if_neededwrite unchecked. make_pathtreating existing directories as success means the newly-propagated directory errors won't break re-install over an existing tree.
Extended reasoning...
Overview
The PR fixes a panic (index out of bounds: the len is 4096 but the index is 4096) in the isolated linker when a folder dependency contains a directory chain longer than PATH_MAX. It touches src/install/isolated_install/{Hardlinker,FileCopier,Installer}.rs, src/install/PackageManager/patchPackage.rs, src/paths/{Path.rs,lib.rs}, and adds test/cli/install/isolated-install-long-paths.test.ts. The mechanism is: switch the per-entry destination/source paths from CheckLength::ASSUME (which panics on overflow inside Buf::append's slice indexing) to CheckLength::CHECK via a new Path::into_checked() reinterpret, then surface the append failure as ENAMETOOLONG for the package. Alongside, three previously-swallowed errors (directory make_path in both linkers, fstat in the unix FileCopier) now propagate, and the Windows Hardlinker gains a manual pre-check before two unchecked pooled-buffer writes.
Security risks
None identified. The change tightens validation of on-disk directory-walk input (turning a crash into a recoverable per-package error) and does not touch auth, network, or lockfile parsing. Making NT_OBJECT_PREFIX and CheckLength::CHECK pub is crate-internal API surface, not user-facing.
Level of scrutiny
Medium-high. The isolated linker is a production-critical install path, the change is cross-platform with distinct Windows and POSIX loop bodies, and it deliberately changes behavior on three previously-silent error paths. The bun_paths additions (into_checked, public CheckLength) are small but establish a pattern other call sites may adopt. This is not a mechanical fix — the PR reasons carefully about which make_path results to propagate vs. leave best-effort, and adds hand-computed buffer-length arithmetic for the Windows NT-path join.
Other factors
- The
Installer.rscall sites previously used.into_sep::<{ PathSeparators::AUTO }>(); I checked thatOsAutoAbsPath,OsAutoPath, anddest_subpathare all alreadyPathSeparators::AUTO, so replacing with.into_checked()(which preservesSEP_OPT) does not lose the/→\\normalization the Windows FileCopier depends on. Path::appendinCHECKmode returnsErrbefore callingbuf_append_input, so when the first append succeeds and the second fails, the post-bodyset_length(saved_len)correctly restores both paths before the error return.- The Windows
needed_lenbound is conservative-correct:join_string_buf_w_sameneedsΣ part.len() + (parts-1)separators,add_nt_path_prefix_if_neededprepends 4 units and NUL-terminates;Σ(part.len()+1) + 4covers that with at most one unit of slack. - The propagated
make_patherrors should not regress re-installs becausemake_pathreturns success on already-exists; the ENOENT-retrymake_pathcalls that are immediately followed by a re-attempted link/copy remainlet _ =since the retry's error already propagates. - The test builds >PATH_MAX trees via chunked
rename(so no single syscall exceeds the limit), covers file-leaf, directory-only, and fitting-tree cases, usestest.concurrent, drains all pipes, and skips the overflow cases on Windows with a stated reason. robobun confirmed 3 pass on the debug build vs 1 pass / 2 fail on release.
Given the behavior changes on error paths in a critical installer loop and the new bun_paths public surface, deferring to a human reviewer rather than auto-approving.
|
Updated 6:10 AM PT - Aug 11th, 2026
✅ @robobun, your commit 7d99e1d4f139f164e1a9515c499e56908dbf95a8 passed in 🧪 To try this PR locally: bunx bun-pr 37424That installs a local version of the PR into your bun-37424 --bun |
…r; cover the copy backend, the PATH_MAX boundary and the Windows absolute path The unix FileCopier printed the error and exited the process when the destination of an entry could not be created after creating its parent; every caller already reports a returned error as the package failing. Tests now also cover the hardlink and copyfile backends walking a cached package, the destination path one byte below and exactly at PATH_MAX, and on Windows an entry whose cwd-relative store path fits but whose absolute destination does not, which is what the join guard in the Hardlinker is for.
![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 changes error-propagation semantics in the isolated linker (directory make_path and fstat failures now fail the package where they were previously silent), adds public surface to bun_paths, and has eight open comment-cop flags on new/re-indented comments that haven't been addressed yet, a human look is still worthwhile.
What was reviewed:
into_checked()preservesSEP_OPT, so replacing.into_sep::<AUTO>()with.into_checked()at the Installer/patchPackage call sites type-checks only because the source paths were alreadyAUTO— no separator-normalization change.- The saved-length restore in Hardlinker/FileCopier still runs when
appendfails mid-way (both lengths are captured before the labeled block,set_lengthruns unconditionally after). - The Windows
needed_lenbound counts one separator per part plus the\\??\\prefix, which covers both the join and the NT-prefix write; slightly conservative but safe. make_pathtreating existing directories as success means the new directory-error propagation does not break reinstalling over an existing store tree.
Extended reasoning...
Overview
Converts the isolated linker's per-entry path builders from CheckLength::ASSUME to CheckLength::CHECK so an over-long walker entry fails the package with ENAMETOOLONG instead of panicking on an out-of-bounds buffer index. Touches Hardlinker.rs and FileCopier.rs (both POSIX and Windows arms), the four Installer.rs call sites plus patchPackage.rs, and adds Path::into_checked() / public CheckLength::CHECK / public windows::NT_OBJECT_PREFIX to bun_paths. Also tightens three previously-swallowed errors in the same loops: directory make_path, Windows CreateDirectoryExW fallback, and the unix fstat before fchmod. Replaces a Global::exit(1) in the unix FileCopier retry path with ? propagation. New test file exercises the PATH_MAX boundary (fits / one-over), the directory-only crash, and both hardlink/copyfile backends via a poisoned cache entry.
Security risks
None identified. The change hardens a user-reachable panic (folder-dependency contents are user-controlled) into a recoverable per-package error, which is strictly safer. No new untrusted input parsing, no path-traversal surface added.
Level of scrutiny
High. This is native install-path code with divergent Windows/POSIX branches, and it changes whether an install reports success: previously-ignored make_path/fstat failures now fail the package. That is the correct behavior per the repo's "never signal success on a failure" rule, and the PR description argues it matches the hoisted linker, but it is a user-visible behavior change to bun install that a maintainer should sign off on. The bun_paths additions are small but public API.
Other factors
- Eight unresolved
comment-cop(github-actions) inline comments flag paragraph-length comments added or re-indented in this PR (FileCopier.rs:14/139/164/222, Hardlinker.rs:15/148, Path.rs:84/865). Several are legitimate why-comments or public-API docs, but the author has not yet responded to or trimmed them. - CodeRabbit's one finding (retry-arm
make_pathresult discarded) was answered and withdrawn; the reasoning — the retriedcopy_file/linkis authoritative — is sound and consistent with the sibling ENOENT arms. - Test coverage is thorough (boundary-exact, both backends, Windows absolute-path case), uses
tempDir/bunEnv/test.concurrent, and the description confirms it fails on the release binary and passes on the debug build. cargo checkacross linux/windows/darwin targets is reported passing, and robobun confirmed the repro.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes error-propagation behavior in the isolated installer (previously-swallowed make_path/fstat failures now fail the package, and the unix FileCopier retry-fail path changes from Global::exit(1) to ?), adds public surface to bun_paths, and has cross-platform #[cfg] divergence, a human look would still be worthwhile. There are also several unresolved comment-cop findings on the touched files.
What was reviewed:
into_checked()reuses the same field-move asinto_sep(); the dropped.into_sep::<AUTO>()calls inInstaller.rswere no-ops (source paths are alreadyAUTO, enforced by theFileCopier::initsignature).- Checked-append +
set_lengthordering in both loops — saved lengths are captured before the fallible append, so a failed append still restores correctly. - Windows
needed_lenbound covers both pooled buffers (join into buf1, NT-prefix into buf2; both arePATH_MAX_WIDE). - The retained
let _ = make_path(..)sites all have a retried operation immediately after whose result propagates.
Extended reasoning...
Overview
Fixes a panic (index out of bounds: the len is 4096 but the index is 4096) in the isolated linker when a walked package entry's path exceeds PATH_MAX. The fix threads CheckLength::CHECK through the Hardlinker/FileCopier path types so append returns Err(MaxPathExceeded) instead of indexing past the pooled buffer, and surfaces that as ENAMETOOLONG on the package. Along the way it also propagates previously-discarded make_path (directory-entry) and fstat errors, replaces a Global::exit(1) in the unix FileCopier retry-fail path with ?, and adds a Windows-only pre-check for the absolute-destination join into the NT-prefixed buffer. bun_paths gains Path::into_checked(), and CheckLength / CheckLength::CHECK / windows::NT_OBJECT_PREFIX become pub. A new test file covers the fitting case, the PATH_MAX boundary (both sides), the copyfile backend via a mutated cache entry, and the Windows absolute-destination overflow.
Security risks
None identified. The change tightens error handling on paths that come from the local filesystem (folder dependencies / cache entries); no new parsing of untrusted input, no auth/crypto/permissions.
Level of scrutiny
Medium-high. This is production install-path code that runs on every --linker=isolated install, with #[cfg(windows)]/#[cfg(not(windows))] divergence in every touched loop. The error-propagation changes are behavior changes: directory-creation and fstat failures that used to be silently dropped now fail the package, and the unix FileCopier's retry-fail path no longer hard-exits the process. Those are the right calls per the PR description and the hoisted-linker precedent (#37400), but they're the kind of "previously best-effort now hard-fail" semantics a maintainer should sign off on. The bun_paths additions are minimal (one method + visibility), but new public API on a foundational crate.
Other factors
- Test coverage is thorough (fitting case, one-byte-under/at PATH_MAX, both backends, Windows absolute path) and the description documents
USE_SYSTEM_BUN=1failure and cross-targetcargo check. - The dropped
.into_sep::<AUTO>()at the twoFileCopier::initcall sites inInstaller.rsis safe:FileCopier::inittakesAbsPathAutoOs/PathAutoOs(bothSEP = AUTO), so a mismatched source SEP would fail to compile — the old conversion was already the identity. - The one CodeRabbit finding was discussed and withdrawn; the reasoning (retried
copy_fileis authoritative) is sound and consistent with the sibling ENOENT retry arms. - Eight comment-cop inline comments from github-actions remain unresolved on the touched files (posted after the "shorten comments" commit). They appear to target pre-existing multi-line comments the diff brushed rather than the one-liners this PR added, but they are outstanding automated feedback the author may want to address or dismiss.
1 participant
Repro
A
file:dependency containing a directory chain longer than PATH_MAX (4096 bytes on Linux, 1024 on macOS), installed with the isolated linker:Exit code 134, on bun 1.4.0-canary.1 (9008ae7). The same tree without
leaf.txtcrashes the same way, and so does the same tree inside a cached package (--backend=hardlink). The hoisted linker reportsENAMETOOLONG: failed copying files from cache to destination for package pkgand exits 1 for this tree; with this PR the isolated linker reportsand exits 1 in all of these cases.
Cause
isolated_install/Hardlinker.rsandFileCopier.rsbuild the destination (and on Windows the source) of every entry by appending the walker's entry path to abun_paths::Pathin its defaultCheckLength::ASSUMEmode, whereappendskips the length check and over-long input indexes past the pooledPathBuffer.walker_skippableopens each directory relative to its parent and keeps the entry path in a growableVec, so it yields entries of any length; any folder dependency or cache entry with a deep enough tree crashes the install. Thelet _ = self.dest.append(..)on the unix side documented this as "fire-and-forget", but theErrarm is dead underASSUME; the failure mode was the panic.Smaller problems in the same loops (the hoisted counterparts are fixed in #37400):
make_pathin the Hardlinker,CreateDirectoryExW+make_pathin the Windows FileCopier) was ignored and the install reported success without the directory.fstatof the source didcontinueafter the destination had already been created and truncated, leaving an empty file in a package that installed "successfully".--backend=copyfiletake.Fix
bun_paths:Path::into_checked()reinterprets a path asCheckLength::CHECK(the same no-op move asinto_sep), andCheckLength::CHECKis public so the checked OS-unit types can be named. The store paths are still built with the existingASSUME-onlyPathLikehelpers (their inputs are bounded); the conversion happens where the unbounded walker paths get appended. This is the approach cli: report an error for over-PATH_MAX --cpu-prof-dir / --heap-prof-dir instead of panicking #36881 took for the profiler output paths.HardlinkerandFileCopierhold checked paths; an entry that does not fit fails the package withENAMETOOLONG(taggedlink/copyfile), which is what the kernel returns for the same path and what the hoisted linker reports. TheEXDEV/EACCES/EPERMfallback to copying still works, since those errors still come out oflinkat.\??\prefix + NUL fit beforejoin_string_buf_w_same/add_nt_path_prefix_if_needed, which copy intoPATH_MAX_WIDEbuffers unchecked: the checkeddestonly bounds the cwd-relative part, and a package's own paths are shorter than their destinations by the store prefix, so a package that exists on disk can still have destinations that do not fit (panic: range end index 32785 out of range for slice of length 32767without the check).bun_paths::windows::NT_OBJECT_PREFIXis public for that.fstaterror propagate, and the unix FileCopier returns the error from creating the destination instead of exiting; every caller (bothInstallersites andbun patch) already reports a returned error.make_pathtreats an existing directory as success (that is also the path taken whenCreateDirectoryExWfails with "already exists"), so reinstalling over an existing tree is unchanged; what propagates is a directory that really does not exist afterwards. Themake_pathcalls that are immediately followed by a retried link/copy whose error is propagated are left as they were: the retry decides. On unix the copy backend never creates directories on its own (same as the hoisted copyfile backend), so a tree of only over-long directories still installs with it; only the hardlink backend reports that case.Verification
test/cli/install/isolated-install-long-paths.test.tsbuilds the trees withnode:fs(chunks of four directories assembled bottom-up withrename, so nothing handed to the filesystem is near PATH_MAX) and covers:>=in the checkedappendis pinned from both sides; the failing entry there is the file;--backend=hardlink(crash before) and--backend=copyfile(process exit from the worker before, now the package error);ENAMETOOLONGwith it (verified both ways on a Windows debug build).Against the release binary the four failure cases fail and the two fitting cases pass; with the debug build all pass on Linux, and the two Windows cases pass on a Windows debug build. Also run with the debug build:
isolated-install.test.ts(62 pass on Linux; on Windows the backend and folder dependency cases, while three unrelated registry tests fail identically on main on that VM),bun-install-hardlink-fallback.test.ts(copy fallback for folder dependencies, both linkers) andbun-patch.test.ts(FileCopierviabun patch, Linux and Windows).cargo check -p bun_installpasses for the Linux,x86_64-pc-windows-msvcandx86_64-apple-darwintargets.