install: fail verification instead of panicking when <alias>/package.json does not fit the path buffer - #38575
install: fail verification instead of panicking when <alias>/package.json does not fit the path buffer#38575robobun wants to merge 1 commit into
Conversation
…json does not fit the path buffer The hoisted installer verifies an existing node_modules entry by appending "/package.json" (or "/.bun-tag" for git dependencies) to the alias inside the PathBuffer that holds it. The alias is only checked to fit the buffer on its own, so an alias within 13 bytes of MAX_PATH_BYTES made the append index past the end of the buffer and abort the install. Build both paths through one helper that returns None when the suffixed path does not fit. Verification then fails and the package is reinstalled, which reports ENAMETOOLONG from the OS the same way a fresh install of that alias does.
|
Status: reproduced on the released build ( CI (build 96204): all 177 build and test jobs that ran are green; the only annotations are retried-and-passed flakes in unrelated files. The build is marked failed only because the two macOS 14 aarch64 test shards never got an agent (four attempts each expired unstarted over four hours; that queue is currently backed up for every PR). Nothing in this diff is platform specific, and |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The bounds check and RAII helper look correct and the tests cover both affected paths, but since this reworks buffer handling in the hoisted install verify path, a human look would still be worthwhile.
What was reviewed:
DestinationSubpath::newbounds check (len >= buf.len()) leaves room for the NUL atbuf[len]; bytes written match the old code exactly in the fitting case, andDroprestoresbuf[alias_len] = 0(always in-bounds sincealias_is_safe_install_targetguaranteesalias.len() < MAX_PATH_BYTES).- Confirmed
SEP_STRhas no remaining uses in the file after the two removed sites. - Test alias length (buffer − 6) overflows both
/.bun-tagand/package.jsonon Linux and macOS;createDumbHttpGitRepo/serveDirectoryare existing local helpers, pipes drained concurrently, Windows skip is justified.
Extended reasoning...
Overview
The PR fixes a panic in bun install --linker hoisted when a dependency alias is within ~13 bytes of MAX_PATH_BYTES and node_modules already exists. Two sites in src/install/PackageInstall.rs (verify_git_resolution and get_installed_package_json_source) appended /.bun-tag or /package.json plus a NUL to the alias inside destination_dir_subpath_buf with no length check. Both are refactored to a shared DestinationSubpath RAII helper that returns None when the suffixed path would not fit, in which case verification fails and the reinstall path reports the same ENAMETOOLONG a fresh install already does. Two new tests in test/cli/install/bun-install.test.ts cover the file: and git dependency variants.
Security risks
None. The alias validation predicate alias_is_safe_install_target is unchanged; the fix only adds a bounds check before an in-place buffer write and returns "not verified" instead of panicking. No new external input is processed and no path-escape surface changes.
Level of scrutiny
Medium-high. The install pipeline is production-critical and this reworks how two verify helpers borrow and write into destination_dir_subpath_buf, replacing raw-pointer + scopeguard with a safe &mut [u8] reborrow held by an RAII type with Deref<Target = ZStr> and Drop. I traced the bounds arithmetic (name_start = alias_len + 1, len = name_start + name.len(), guard len >= buf.len(), write NUL at buf[len]) and confirmed it writes byte-for-byte the same content as before in the fitting case and safely returns None in the overflowing case. The Drop restore is always in-bounds because the alias's own NUL was already at buf[alias_len]. The pre-existing destination_dir_subpath / destination_dir_subpath_buf field aliasing (noted in the struct's TODO) is neither introduced nor worsened here — the old code took the same two-field borrow.
Other factors
- The PR description explains why the fix lives here rather than tightening
alias_is_safe_install_target(that predicate is shared with the isolated linker, which has no such suffix, and would misreport the alias as "unsafe"). That's a design call a maintainer should confirm. - Tests use existing in-file helpers (
createDumbHttpGitRepo,serveDirectory),tempDirwithusing,Promise.allon stdout/stderr/exited, an isolatedBUN_INSTALL_CACHE_DIR, and force--backend hardlinkso the failure step is reported the same way on macOS and Linux. Windows is skipped with a stated reason (buffer larger than any accepted OS path). cargo clippy -p bun_installand cross-targetcargo checkwere reported clean by the author; robobun confirmed the repro on the released build and that the new tests fail unfixed / pass fixed.
|
Thanks for the look. Nothing to change from the review; the bounds check and the restore-on-drop are the whole behavioral change, and the two new tests are the fail-before / pass-after proof for both verify paths. Waiting on the Buildkite run. |
Problem
bun install(hoisted linker) aborts withpanic: range end index 4103 out of range for slice of length 4096(exit 134) whennode_modulesalready exists and a dependency alias is betweenMAX_PATH_BYTES - 13andMAX_PATH_BYTES - 1bytes long (4083..4095 on Linux, 1011..1023 on macOS). An alias in the last bytes of that window panics withindex out of bounds: the len is 4096 but the index is 4096instead; git and github dependencies panic the same way from 9 bytes below the buffer size.alias_is_safe_install_target(src/install/PackageInstaller.rs:412) only requires the alias itself to fit thePathBufferit is copied into.verify()then appends/package.json(get_installed_package_json_source,src/install/PackageInstall.rs:875) or/.bun-tag(verify_git_resolution,src/install/PackageInstall.rs:794) plus a NUL to the alias in that same buffer with no length check.verify()only runs whennode_modulesalready existed before the install (a freshnode_modulesskips it), which is why a first install of such an alias fails cleanly withENAMETOOLONGand any later install, or the first install into a project that already has anode_modules, aborts.Fix
DestinationSubpathhelper, which returnsNonewhen<alias>/<name>plus its NUL does not fit the buffer, and restores the alias's NUL terminator when dropped. The helpers treat that like any other unreadablepackage.json/.bun-tag: verification fails.ENAMETOOLONG: failed opening node_modules/package dir for package <name>/Failed to install 1 package/ exit 1 that a fresh install of the same alias already reports, so an alias a few bytes short of the buffer now behaves like one that is 600 bytes long. Rejecting the alias inalias_is_safe_install_targetinstead would report it as an unsafe name, and that predicate is also used by the isolated linker, which has no such suffix.destination_dir_subpath_buffor as long as the path is in use, which replaces the raw pointer +scopeguardrestore the two helpers each had; the bytes written are the same as before.test/cli/install/bun-install.test.ts, "dependency alias that fills the path buffer": afile:dependency and a git dependency (served from a local dumb-http repo) aliased under aMAX_PATH_BYTES - 6byte name, installed into a pre-creatednode_modules. Unfixed, they abort with the two panics above (4103and4099out of range); fixed, both reportENAMETOOLONGand exit 1. Skipped on Windows, where the buffer (98302 bytes) is larger than any path the OS accepts.bun-install.test.ts(git / folder / workspace / file: subsets),bun-workspaces.test.ts,bun-install-patch.test.tsandbun-link.test.tswith the debug build; the only failures are the bitbucket/gitlab tests (no network here) andbun-link"should link dependency without crashing", which fails on main too because the debug build prints a stack trace on install failure (install: make the debug-build stack dump on package install failure opt-in #37335).cargo clippy -p bun_installis clean andcargo check -p bun_installpasses forx86_64-pc-windows-msvcandx86_64-apple-darwin.install_from_link, which install: stop aborting on linked package names that do not fit the symlink buffers #38571 fixes separately; with both fixes a workspace named this way fails the same way as the two cases tested here.Background
node_modules/<alias>, where the alias is the key independencies(or the workspace package's name).PackageInstallercopies the alias intodestination_dir_subpath_buf, a stackPathBufferofMAX_PATH_BYTES(4096 on Linux, 1024 on macOS, 98302 on Windows), andPackageInstallworks with the NUL-terminated prefix of that buffer as the install destination.verify()decides whether an entry already innode_modulescan be kept: for most resolutions it reads<alias>/package.jsonand compares name and version, for git/github resolutions it reads the<alias>/.bun-tagfile bun writes on checkout and compares the commit. Both build the file path by temporarily appending to the alias in the buffer and putting the NUL back afterwards.falsefromverify()means the package is (re)installed.Repro on an unfixed build (Linux)
With a 4083 byte name the suffix itself fits and the NUL write panics with
index out of bounds: the len is 4096 but the index is 4096; a git dependency aliased under the 4090 byte name panics withrange end index 4099.