install: stop aborting on linked package names that do not fit the symlink buffers - #38571
install: stop aborting on linked package names that do not fit the symlink buffers#38571robobun wants to merge 3 commits into
Conversation
…not fit the symlink buffers install_from_link copied the node_modules entry name into a 512 byte stack buffer on POSIX, so a workspace or link: package whose name was longer than that aborted the whole install. The name is the tail of destination_dir_subpath, which is already NUL-terminated, so pass that to symlinkat directly. On Windows the name is appended to the absolute node_modules path in a PathBuffer; check that it fits and fail the package with ENAMETOOLONG when it does not.
|
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 (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe link installation path now uses NUL-terminated destination strings, validates Windows destination capacity, and passes destinations directly to Unix symlink creation. Workspace tests cover oversized scoped and unscoped package names. ChangesWorkspace linking
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit fd63e5a has some failures in 🧪 To try this PR locally: bunx bun-pr 38571That installs a local version of the PR into your bun-38571 --bun |
|
Status: reproduced and fixed; the diff is green, the remaining red checks are unrelated to this PR (details below).
|
There was a problem hiding this comment.
LGTM — replaces an unchecked 512-byte buffer copy with a borrow of the already-NUL-terminated tail, and adds the missing bounds check on the Windows path.
What was reviewed:
- Verified
subdir.map_or(0, |dir| dir.len() + 1)produces the same bytes as the oldbasename()for both shapesalias_is_safe_install_targetadmits (nameand@scope/name), so the success path is unchanged. - Confirmed
ZStr::len()/as_bytes()exclude the NUL, so the Windowsoffset + dest.len() >= dest_buf.len()check leaves room for the terminator andcopy_from_slice(dest.as_bytes())is byte-identical to before. - Test covers scoped and unscoped names on both platforms and was verified to abort on the unfixed build.
Extended reasoning...
Overview
The PR fixes a panic in PackageInstall::install_from_link (src/install/PackageInstall.rs) when a linked package name exceeds the stack buffer used to build the symlinkat/junction destination. On POSIX it removes the 512-byte dest_name_buf copy entirely — destination_dir_subpath is already a &ZStr, and alias_is_safe_install_target guarantees it is exactly name or @scope/name, so its tail (after the scope separator, if any) is passed directly to symlinkat. On Windows it adds a length check before appending the name to the absolute node_modules path in dest_buf: PathBuffer, returning ENAMETOOLONG under Step::LinkingDependency instead of panicking on the slice index. A regression test in bun-workspaces.test.ts covers scoped and unscoped names sized to overflow the old buffer on each platform.
Security risks
None. The change converts a user-reachable panic (DoS on adversarial package.json input) into the same recoverable per-package failure that shorter over-length names already produce. The input-validation gate (alias_is_safe_install_target, which rejects .., absolute paths, backslashes, NUL, and multi-component names) is unchanged and still runs before this code.
Level of scrutiny
Moderate. This is a small, mechanical fix in the package installer's link path — no new syscalls, no allocation changes, no unsafe added. I traced dirname::<Auto> on both platforms against the two admitted alias shapes to confirm the new tail-slice offset matches what bun_paths::basename returned before, and checked ZStr::len/as_bytes/from_slice_with_nul semantics to confirm the Windows copy and the new bounds check (>=, leaving room for the NUL at [offset + dest.len()]) are correct. The success path passes byte-identical arguments to symlinkat/symlink_or_junction.
Other factors
The PR description documents empirical verification (USE_SYSTEM_BUN=1 panics, bun bd passes) on Linux x64 and Windows x64, plus cargo check on macOS and Windows targets and clean clippy/fmt. The test follows harness conventions (test.concurrent.each, setupTest, await using on the spawned process, drains both pipes concurrently, asserts stderr/stdout before exit code, Buffer.alloc for the long string). Two adjacent overflow sites (get_installed_package_json_source and the progress bar) are explicitly called out as separate issues rather than silently left. No CODEOWNERS on src/install/, no prior human review comments.
…ry name is the basename
There was a problem hiding this comment.
LGTM — converts a user-reachable panic into the same ENAMETOOLONG failure that shorter over-limit names already produce.
What was reviewed
- POSIX: verified
desttail-slice is equivalent to the removedbasenamecopy for bothnameand@scope/nameshapes (guaranteed byalias_is_safe_install_target); thedebug_assert_eq!locks this in. - Windows: bounds check
offset + dest.len() >= dest_buf.len()correctly reserves the trailing NUL byte;dest.as_bytes()/dest.len()on the new&ZStrmatch the old&[u8]semantics. relative_buf_zis the existing helper that does exactly what the removed manual copy+NUL did for the target path.- The comment-cop bot feedback was addressed in fd63e5a (pushed after the bot fired on a5c4db7).
Extended reasoning...
Overview
Changes are confined to PackageInstall::install_from_link in src/install/PackageInstall.rs plus a two-case regression test in test/cli/install/bun-workspaces.test.ts. The fix removes an unchecked copy of the link entry name into a 512-byte stack buffer (POSIX) and adds a bounds check before the equivalent copy on Windows, so an over-length name fails the one package with ENAMETOOLONG instead of aborting the whole install with a slice-index panic.
Security risks
None. The change tightens handling of adversarial input (a package name from package.json): a name that previously panicked now surfaces as a per-package error, and a name that fits is passed through byte-identical arguments. No new privileges, no new syscalls, no new user-facing surface.
Level of scrutiny
Moderate — this is native install-path Rust with buffer arithmetic, but the change is a net simplification: one buffer copy is deleted (POSIX now borrows the already-NUL-terminated tail of destination_dir_subpath), one bounds check is added (Windows), and the target string is built with the existing relative_buf_z helper instead of a hand-rolled copy+NUL. I traced resolve_path::dirname::<platform::Auto> for both name and @scope/name on POSIX and Windows and confirmed the subdir.map_or(0, |dir| dir.len() + 1) offset yields exactly basename(dest_path), which the added debug_assert_eq! also enforces on every debug run. alias_is_safe_install_target rejects \\, :, \0, empty/./.. components, and anything but 1 or @-prefixed 2 components, so no other shapes reach this code. The Windows check offset + dest.len() >= dest_buf.len() correctly reserves room for the NUL written at dest_buf[offset + dest.len()].
Other factors
- Tests cover both the unscoped and scoped (separately-opened
@scopedirectory) branches on both platforms, and the PR description recordsUSE_SYSTEM_BUN=1failing /bun bd testpassing on Linux and Windows. - The two
comment-copbot comments fired at 18:06:59Z on commit a5c4db7; commit fd63e5a (18:15:38Z) replaced the flagged paragraph comments with the one-line comment +debug_assert_eq!and switched torelative_buf_z, so that feedback is addressed. - The one CI failure so far (
s3.test.tson Windows 2019) is unrelated to install/linking. - REVIEW.md's "user-reachable failures are recoverable errors, never panics" is exactly what this enforces.
Problem
bun install --linker hoistedaborts when a workspace (orlink:) package has to be linked intonode_modulesunder a name longer than 512 bytes:ENAMETOOLONG: failed linking dependency/workspace to node_modules for package .../Failed to install 1 package(exit 1), and the installer accepts names up to one byte below the path buffer size (4095 bytes on Linux, 1023 on macOS), so every length in between aborts.PackageInstall::install_from_link(src/install/PackageInstall.rs) copies the entry name intodest_name_buf: [u8; 512]to get a NUL-terminated string forsymlinkat, without checking the length. At exactly 512 bytes the copy fits but the NUL does not, so a release build handssymlinkata string that runs past the buffer and a debug build trips theZStr::from_bufassertion.node_modulespath in a 98302 bytePathBuffer, so a name within anode_modulespath length of the 98301 byte maximum aborts as well:Fix
destination_dir_subpath, which is already NUL-terminated (alias_is_safe_install_targetguarantees it isnameor@scope/name), so that tail is passed tosymlinkatdirectly. Nothing is left to bounds check: the file system rejects a name overNAME_MAXwithENAMETOOLONG, which is exactly what happens to the names of up to 512 bytes today, so longer names now fail the same way.ENAMETOOLONGunderStep::LinkingDependencywhen it does not fit, the same failure the directory path check a few lines above produces. A destination that does not fit the buffer cannot exist on disk, so this is the error the OS would give for it.test/cli/install/bun-workspaces.test.ts,describe("workspace packages whose name is too long to link"): an unscoped and a scoped (@scope/<name>, which links inside a separately opened scope directory) workspace member, with a 600 byte name on POSIX and a 98282 byte name on Windows. Both cases abort with the panics above on the unfixed binary on Linux (USE_SYSTEM_BUN=1) and on Windows (stock build of current main), and pass withbun bd teston Linux x64 and Windows x64.bun-workspaces.test.ts(70 tests) andbun-link.test.tspass with the debug build, exceptbun-link.test.ts"should link dependency without crashing", which fails on main with any debug build independently of this change (install: make the debug-build stack dump on package install failure opt-in #37335).cargo check -p bun_installforx86_64-pc-windows-msvcandaarch64-apple-darwin,cargo clippy -p bun_installandcargo fmt --checkare clean.Not in this PR
bun install(withnode_modulespresent) of a name within 13 bytes of the path buffer size (4083 to 4095 bytes on Linux) still aborts, from a different place: the verify step appends/package.jsonto the buffer the name was copied into (get_installed_package_json_source). Reported separately.Background
link:packages andfile:dependencies on the project root intonode_modulesas symlinks (junctions on Windows);install_from_linkcreates that one link. The link's name isdestination_dir_subpath, the dependency's name as it appears undernode_modules. It comes from package.json, andalias_is_safe_install_target(PackageInstaller.rs) only lets through a single component or@scope/nameshorter than the path buffer.ZStris bun's borrowed NUL-terminated byte string, the type the syscall wrappers take.as_bytes_with_nul()is the bytes including the terminator andZStr::from_slice_with_nulborrows a slice ending in one, so the tail of an existingZStris aZStrwithout a copy.PathBufferis a stack buffer of the platform's maximum path length: 4096 bytes on Linux, 1024 on macOS, 98302 on Windows (32767 UTF-16 units at up to three UTF-8 bytes each, plus the NUL).NAME_MAX, the longest single component a file system accepts, is 255 bytes on Linux and macOS, so every name this change is about is rejected by the OS once it gets there.Name lengths probed on the unfixed build (workspace member,
CI=1 bun install --linker hoisted)