install: stop panicking on workspaces entries longer than the path buffer - #37531
Conversation
…tries longer than the path buffer Every entry of the root package.json workspaces array was joined into a fixed size path buffer with the unchecked join helpers, so an entry (or a glob match) whose joined path did not fit indexed past the buffer and aborted bun install. Use the checked joins and report the overflow through the existing per-entry error path, and join glob patterns with the spilling variant so long patterns are handed to the glob walker like any other pattern.
|
Status: reproduced and fixed. Reproduced with the released Tests: |
WalkthroughWorkspace package resolution now uses byte-slice paths and checked joins. Direct entries and glob matches propagate ChangesWorkspace path resolution
Possibly related PRs
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: 2
🤖 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/lockfile/Package/WorkspaceMap.rs`:
- Around line 260-263: Update the error-handling branch around the cwd lookup in
the workspace map logic to handle bun_sys::getcwd failure without
expect("unreachable"). Log the workspace error without cwd details when getcwd
fails, or propagate the getcwd error, while preserving the existing formatting
path for successful lookups.
In `@test/cli/install/bad-workspace.test.ts`:
- Around line 279-292: Update the writer subprocess assertions around spawn to
drain stdout, stderr, and exited concurrently, then assert the combined results
together. Preserve the expectation that both streams are empty and the process
exits with status 0, using the existing writer handles.
🪄 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: 68bf407b-6e3a-4cde-ac5c-b1b4e95ec6ac
📒 Files selected for processing (2)
src/install/lockfile/Package/WorkspaceMap.rstest/cli/install/bad-workspace.test.ts
| } else { | ||
| let mut cwd_buf = vec![0u8; MAX_PATH_BYTES]; | ||
| let cwd_len = bun_sys::getcwd(&mut cwd_buf).expect("unreachable"); | ||
| let _ = log.add_error_fmt( |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the getcwd expect() is pre-existing or introduced here.
rg -n -C4 'getcwd' src/install/lockfile/Package/WorkspaceMap.rs
git log -1 --format=%H
git diff --unified=6 "$(git merge-base HEAD origin/main 2>/dev/null || echo HEAD~1)" -- src/install/lockfile/Package/WorkspaceMap.rs | rg -n -C6 'getcwd'Repository: oven-sh/bun
Length of output: 2234
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- WorkspaceMap.rs ---'
sed -n '210,300p' src/install/lockfile/Package/WorkspaceMap.rs
printf '%s\n' '--- getcwd definitions and call sites ---'
rg -n -C3 'fn getcwd|getcwd\(' src | head -200
printf '%s\n' '--- surrounding error handling ---'
rg -n -C5 'process_workspace_name|ENAMETOOLONG|Error::Sys' src/install/lockfile/Package/WorkspaceMap.rsRepository: oven-sh/bun
Length of output: 21198
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- process_workspace_name and workspace-map control flow ---'
sed -n '100,190p' src/install/lockfile/Package/WorkspaceMap.rs
sed -n '420,500p' src/install/lockfile/Package/WorkspaceMap.rs
printf '%s\n' '--- analogous package-manager fallback ---'
sed -n '540,575p' src/runtime/cli/package_manager_command.rs
printf '%s\n' '--- relevant history for the expect ---'
git log --oneline -8 -- src/install/lockfile/Package/WorkspaceMap.rs
git blame -L 250,270 -- src/install/lockfile/Package/WorkspaceMap.rsRepository: oven-sh/bun
Length of output: 11877
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- commit change ---'
git show --format= --unified=12 d42eb4104 -- src/install/lockfile/Package/WorkspaceMap.rs | sed -n '1,220p'
printf '%s\n' '--- getcwd implementation ---'
sed -n '2638,2665p' src/sys/lib.rs
printf '%s\n' '--- read-only control-flow verifier ---'
python3 - <<'PY'
from pathlib import Path
p = Path("src/install/lockfile/Package/WorkspaceMap.rs")
text = p.read_text()
assert 'None => Err(crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG))' in text
assert 'bun_sys::getcwd(&mut cwd_buf).expect("unreachable")' in text
print("ENAMETOOLONG is routed to the branch containing getcwd().expect(\"unreachable\").")
PYRepository: oven-sh/bun
Length of output: 10578
Handle getcwd failure without panicking. The new ENAMETOOLONG result enters this branch, and getcwd can fail when the current directory was removed. Log the workspace error without the cwd, or propagate the getcwd error, instead of calling expect("unreachable").
🤖 Prompt for 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.
In `@src/install/lockfile/Package/WorkspaceMap.rs` around lines 260 - 263, Update
the error-handling branch around the cwd lookup in the workspace map logic to
handle bun_sys::getcwd failure without expect("unreachable"). Log the workspace
error without cwd details when getcwd fails, or propagate the getcwd error,
while preserving the existing formatting path for successful lookups.
Source: Coding guidelines
| await using writer = spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| `require("fs").writeFileSync(process.argv.at(-1) + "/package.json", JSON.stringify({ name: "deep" }))`, | ||
| names.at(-1)!, | ||
| ], | ||
| cwd: dirname(workspaceDir), | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| expect(await writer.stderr.text()).toBe(""); | ||
| expect(await writer.exited).toBe(0); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Drain the writer subprocess streams concurrently and assert the combined result.
The writer subprocess pipes stdout, but the test never reads stdout. The test also awaits stderr.text() and exited sequentially. Read both streams and the exit status concurrently, and assert them together so a failure shows all three values.
As per coding guidelines: "Subprocess tests must drain stdout, stderr, and process exit concurrently and assert the combined result and ordered stage outputs."
🧹 Proposed fix
- expect(await writer.stderr.text()).toBe("");
- expect(await writer.exited).toBe(0);
+ const [writerStdout, writerStderr, writerExitCode] = await Promise.all([
+ writer.stdout.text(),
+ writer.stderr.text(),
+ writer.exited,
+ ]);
+ expect({ stdout: writerStdout, stderr: writerStderr, exitCode: writerExitCode }).toEqual({
+ stdout: "",
+ stderr: "",
+ exitCode: 0,
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await using writer = spawn({ | |
| cmd: [ | |
| bunExe(), | |
| "-e", | |
| `require("fs").writeFileSync(process.argv.at(-1) + "/package.json", JSON.stringify({ name: "deep" }))`, | |
| names.at(-1)!, | |
| ], | |
| cwd: dirname(workspaceDir), | |
| env: bunEnv, | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| expect(await writer.stderr.text()).toBe(""); | |
| expect(await writer.exited).toBe(0); | |
| await using writer = spawn({ | |
| cmd: [ | |
| bunExe(), | |
| "-e", | |
| `require("fs").writeFileSync(process.argv.at(-1) + "/package.json", JSON.stringify({ name: "deep" }))`, | |
| names.at(-1)!, | |
| ], | |
| cwd: dirname(workspaceDir), | |
| env: bunEnv, | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| const [writerStdout, writerStderr, writerExitCode] = await Promise.all([ | |
| writer.stdout.text(), | |
| writer.stderr.text(), | |
| writer.exited, | |
| ]); | |
| expect({ stdout: writerStdout, stderr: writerStderr, exitCode: writerExitCode }).toEqual({ | |
| stdout: "", | |
| stderr: "", | |
| exitCode: 0, | |
| }); |
🤖 Prompt for 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.
In `@test/cli/install/bad-workspace.test.ts` around lines 279 - 292, Update the
writer subprocess assertions around spawn to drain stdout, stderr, and exited
concurrently, then assert the combined results together. Preserve the
expectation that both streams are empty and the process exits with status 0,
using the existing writer handles.
Source: Coding guidelines
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted crash fix that swaps three unchecked path joins for their checked/spilling variants and routes overflow through the existing error branch.
What was reviewed:
- Verified
join_abs_string_buf_checkednormalizes into a scratch buffer before length-checking, sox/../-style entries that normalize short still resolve (guarded by the new "once normalized resolves" tests). - Confirmed
get_with_pathalready takes&[u8], so dropping theZStrwrapper is a no-op at the call site. - Checked that
abs_workspace_dir_pathmoving below the error match is safe — it was only read on the success path. - Tests follow harness conventions (
describe.concurrent,tempDir,Buffer.allocfills, concurrent pipe drain, boundary at exactly/±1 byte) andtest.skipIf(...).each(...)chaining is used elsewhere in the suite.
Extended reasoning...
Overview
The PR fixes three panic sites in WorkspaceMap::process_names_array where workspaces array entries longer than the fixed path buffer would index past it. Two join_abs_string_buf_z calls become join_abs_string_buf_checked (returning None on overflow, mapped to Error::Sys(ENAMETOOLONG) and fed into the existing error-message branch), and one join for glob patterns becomes join_spill so oversized patterns still reach the walker. process_workspace_name drops the &ZStr parameter in favour of &[u8] since the only consumer already took &[u8]. Tests are added to the existing bad-workspace.test.ts.
Security risks
None. The change turns a user-reachable panic (adversarial package.json could abort bun install) into a recoverable error, which is a strict hardening. No new parsing, no privilege boundaries, no network.
Level of scrutiny
Moderate. The diff is mostly reindentation from restructuring two match blocks; the substantive change is three helper swaps plus a signature relaxation. I verified: join_abs_string_buf_checked at resolve_path.rs:1616 normalizes into a heap scratch on the slow path and only returns None when the normalized result exceeds the buffer, matching the PR's claim and the "normalized" test row; join_spill at resolve_path.rs:1408 falls through to join when the input fits, so the common case is unchanged; WorkspacePackageJSONCache::get_with_path at line 137 already accepts &[u8]. The continue inside the Some(...) match arm correctly targets the enclosing for, and abs_workspace_dir_path was only used on the success path so moving its computation below the error handler is behaviour-preserving. The ENAMETOOLONG case lands in the existing catch-all else arm that formats {err.name()} reading package.json ..., which is exactly what a shorter-but-still-too-long entry already prints today (per the repro in the description).
Other factors
Test coverage is thorough: 100 kB entries on every platform, exact ±1-byte boundary on POSIX, long brace-group globs that must still match, long globs matching nothing, long entries that normalize short, and a deep-directory glob match whose absolute path overflows. The PR states six of these panic on the unfixed build. Tests use describe.concurrent, tempDir, Buffer.alloc(...).toString(), and concurrent stdout/stderr/exited draining per the repo conventions; skipIf gates carry a comment naming why. The pattern mirrors the existing join_abs_string_buf_checked usage the description points to in lockfile/Package.rs. No CODEOWNERS entry covers src/install/.
2 participants
Repro
bun installaborts (SIGABRT, exit 134). A glob entry ("a".repeat(5000) + "/*") aborts the same way withpanic: range end index 5000 out of range for slice of length 4095. Reproduced withbun 1.4.0-canary.1on Linux, where the buffer is 4096 bytes; it is 1024 on macOS and 32767 * 3 + 1 on Windows (the glob case is 4096 on every platform). A 2000 byte entry, which fits the buffer, already fails the way one would expect:Cause
WorkspaceMap::process_names_arrayjoins every entry of theworkspacesarray with the unchecked join helpers, whose normalizer indexes past the output buffer when the result does not fit (resolve_path.rs,buf[buf_i..buf_i + count]; the "length 4095" is the buffer minus the leading separator):join_abs_string_buf_z(project dir, PathBuffer, [entry, "package.json"])join([pattern, "package.json"])into the 4096 byte thread local buffer, before the pattern ever reaches the glob walkerjoin_abs_string_buf_z(project dir, PathBuffer, [matched dir, "package.json"]). The walker works relative to the project dir, so it can match apackage.jsonwhose path relative to the project fits while the absolute path does not. A member directory whose absolute path is 4090 bytes long (creatable, it fits PATH_MAX) withworkspaces: ["**"]panics withrange end index 4102 out of range for slice of length 4095.The folder dependency arm in
lockfile/Package.rsalready usesjoin_abs_string_buf_checkedfor the same situation.Fix
join_abs_string_buf_checked. When it returnsNonethe entry takes the existing error path withError::Sys(ENAMETOOLONG), so it prints exactly what an entry the OS rejects prints today (ENAMETOOLONG reading package.json for workspace package "...") andbun installexits 1. A path that does not fit the buffer does not fitPATH_MAXeither, so this is the same answer one layer earlier.process_workspace_nametakes&[u8]now;WorkspacePackageJSONCache::get_with_pathnever needed the NUL terminator.join_spill, the variant the glob walker itself uses for long paths, and the pattern is handed to the walker like any other. The walker already copes with long patterns: a pattern that matches nothing is skipped, the same as a 2000 byte one is today, and one that does match (a brace group padded past the buffer size) still finds its workspace. Rejecting long patterns outright would turn that second case into an error.Both joins are checked on the normalized result, so an entry that is long as written but normalizes to a short path (
x/../x/../.../pkgs/pkg1) keeps resolving, same as before.Related: #37462 fixes the same kind of overflow for local tarball and
workspace:dependency specs in other files; #35863 changes the shared join primitive to return an empty path on overflow, which would turn these panics into a misleadingWorkspace not found. This PR is independent of both: the call sites reportENAMETOOLONGeither way.Tests
test/cli/install/bad-workspace.test.ts(the existing file for badworkspacesentries), all spawningbun installwithout a registry:ENAMETOOLONG, exit 1 (every platform)package.jsonpath is one byte below / exactly / one byte above the buffer size:Workspace not found(the path still reaches the OS) /ENAMETOOLONG/ENAMETOOLONG(POSIX; on Windows the OS rejects paths long before the buffer size)pkgs/pkg1; 100 kB glob matching nothing leaves the other entries alone (every platform)x/../path and glob entries that normalize topkgs/pkg1resolve (every platform, passes before and after: guards the normalized length semantics)package.jsonpath does not fit:ENAMETOOLONG, exit 1 (POSIX; the member directory is created at buffer size - 6 bytes and itspackage.jsonwritten relative to the parent directory)On the unfixed build (
USE_SYSTEM_BUN=1) six of these crash with the panics above (100035,100008,100000,index 4096for the NUL write of the "exactly" row,4096,4102); the "one byte below" row and the two normalizing entries pass both ways. Withbun bd testthe file passes, as dobun-workspaces.test.ts(63 tests) andtest/cli/run/workspaces.test.ts.