Conversation
….json The pruned-workspace check, the workspace diff and the pnpm migration built `<root>/<workspace path>/package.json` with `Path::append`. `append` accepts only a relative input that fits the buffer. A `workspaces` key in bun.lock and an importer key in pnpm-lock.yaml are unchecked: they can be absolute, and they can have any length. All of these sites now call `lockfile::workspace_package_json_path`. It joins the path with the length-checked `AutoAbsPathChecked::join`, the resolution that the workspace diff used before #21059. The join also normalizes `..`, so the diff reads a workspace outside the root through the same cache entry that `add --filter` and `remove --filter` edit, and bun.lock now follows those edits.
|
Status
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesWorkspace path validation
Suggested reviewers: Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the unchecked abs_path.join(&[path]) at src/install/pnpm.rs:881 — every non-root importer key passes the checked workspace_package_json_path join (same path plus /package.json) in the importer loop above before this runs, so the shorter join cannot overflow. The workspace_is_missing_on_disk change returning "missing" on an over-long key is consistent with the PR's stated intent (a path that does not fit counts as missing) and only affects pruning on a plain install.
Extended reasoning...
The one posted finding is a pre-existing, same-class overflow on the pnpm link: value, which the inline comment covers. Separately I verified the comment on the unchecked join at pnpm.rs:881: path is an importer key that equals a workspace_paths entry, and every such key was already built with workspace_package_json_path (a strictly longer path) in the importer loop, with an error return on overflow, so the claim in the comment holds. The Diff early break 'update_mapping false and workspace_is_missing_on_disk returning true on None both degrade to "workspace removed/changed" rather than a panic, matching the described behavior. The new tests exercise absolute keys, an over-long key, and add --filter outside the root; I did not independently run them.
|
This PR also fixes a reported abort that comes in through
On 1.4.0, 1.4.2 and main (9b7c982): I verified this branch (d2c4e8a) with a debug build:
I wrote five test cases for this door before I found this PR. They are on
Four of the five fail on bun 1.4.3 (the child aborts). Cherry-pick the commit if you want them here. I am not opening a separate PR. |
|
A note for the rebase, in case #43537 lands first. The two branches are each clean onto main (8cc0397). Together they conflict in one hunk of The side of this PR keeps the unchecked The combined merge with that resolution is already built and tested: #43537 (comment). The test files of both PRs pass on it. |
Problem
bun.lockorpnpm-lock.yamlaborts a debug build:panic: assertion failed: !is_input_absolute(input)(src/paths/Path.rs:911). A release build looks under<root>/<absolute path>, so--frozen-lockfiletreats a workspace on disk as pruned.panic: index out of bounds: the len is 4096 but the index is 4096. Andbun add --filterleavesbun.lockunchanged for a workspace outside the root.<root>/<workspace path>/package.jsonwithAutoAbsPath::append, which takes only a relative input that fits and keeps..as written.Fix
lockfile::workspace_package_json_path, which usesAutoAbsPathChecked::join. The diff had a resolving join until fix(install): fix resolving duplicate dependencies #21059. cli: --heap-prof node parity, and fix absolute profile paths panicking (+2 tests) #34842 fixed this assertion for--cpu-prof-namethe same way.frozen-lockfile-pruned,pnpm-lock-migration,bun-add-filter) on Linux debug/ASAN and Windows x64.Background
workspaceskey inbun.lock, an importer key inpnpm-lock.yaml.turbo prune) keeps the fullbun.lockbut only some members.--frozen-lockfileaccepts a member that is missing on disk.Path::appendadds a relative component.Path::joinresolves likepath.resolveand normalizes...add --filteredits a cachedpackage.json, keyed by its normalized path. The diff must ask for the same key.Notes
Origin. Found while testing #43322. An absolute
workspaceskey stopped its debug build before its own check ran. No user has reported these symptoms.History. Until #21059 the workspace diff built this path with
joinAbsStringBuf(top_level_dir, [path, "package.json"]), which resolves. #21059 replaced it withAbsPath.appendin a larger refactor, with no stated reason. #38333 copied the new shape intopruned_workspaces.rs. The pnpm migration has three more copies (pnpm.rs744, 878, 2244), and twoappend(workspace_path)calls next to ajoin(1006, 1013).Reproduction (no registry, no lifecycle script).
$d/myappis on disk and the root does not list it:bun.lockabove,--frozen-lockfileassertion failed: !is_input_absolute(input)note: skipped 1 workspace listed in bun.lock but not on disk: "myapp", exit 0error: lockfile had changes, but lockfile is frozen, exit 1, the same as for the key../myappindex out of boundsindex out of bounds(Windows: exit 3)Saved lockfile, exit 0pnpm-lock.yamlimporter'<root>/packages/a'error: pnpm-lock.yaml lists importer '...' but '.../package.json' does not exist,Ignoring lockfilemigrated lockfile from pnpm-lock.yaml, and the samebun.lockas forpackages/apnpm-lock.yamlimporter of 100,000 bytesindex out of boundsindex out of boundsdoes not existerror,Ignoring lockfile, exit 0"workspaces": ["../x","../y"],bun add --filter x y@workspace:*x/package.jsongets the dependency,bun.lockdoes notSaved lockfile,installed y@workspace:../y.remove --filterthe same wayWhy
add --filterwas affected.add --filterandremove --filteredit thepackage.jsoninworkspace_package_json_cache.WorkspaceMapput it there under the normalized path<parent>/x/package.json. The diff asked for<root>/../x/package.json, missed, read the file from disk before the edit was written, and saw no change. For a path inside the root the two spellings are the same bytes, so nothing changes there.Why the parser does not reject an absolute key. On Windows,
relative()fromC:\rootto a directory on another drive returns the absolute path. With"workspaces": ["X:\\pkgx"](checked withsubst X:) the release build writes"X:/pkgx": {...}and"pkgx": ["pkgx@workspace:X:/pkgx"], and the next install reads it back with no changes. A parser that rejects the key would printInvalidLockfileandIgnoring lockfileon every install of such a project. #39357 and #39361 are about that setup. It is also why this PR adds no length rule to the parser: anIgnoring lockfiledrops every pin, and the length of the key alone does not bound<root>/<key>/package.json.The over-long key and #43322. #43322 refuses a workspace path that is too long with exit 1, in a pass that runs after the diff. So this PR only keeps the diff from aborting, and its test uses a plain install, where the diff removes the workspace before that pass can see it. The outcome under
--frozen-lockfileis left to #43322.Sites that still mishandle such a path. Not changed here. They run after the diff, in the linkers or in the
--filterselection. #43322 puts its check in front of the linkers, and whether a workspace outside the root is legal at all needs a maintainer decision first.isolated_install.rs:1771and:1881buildAutoRelPath::from(workspace_path)to move an old<workspace>/node_modulesaside. With this PR, a pruned workspace at an absolute or over-long key still aborts here when the isolated linker finds anode_moduleswithout.bun.isolated_install/Installer.rs:2661and:2756append the path of a workspace that is being installed. Abun.lockthat gives a path-spec workspace a dependency onworkspace:<absolute path>reaches:2756.PackageInstaller.rs:1510andPackageManagerDirectories.rs:1005copy the path into a fixed buffer, andworkspace_selection.rs:106and:424andadd_remove_with_filter.rs:166,:186and:196use the uncheckedjoin_abs_string_buf.install --frozen-lockfile --filter <name>with the 100,000-byte key still aborts inworkspace_selection.rs. Report a path that does not fit a path buffer instead of aborting #43067 covers unchecked joins in general.debug_assert!(!bun_paths::is_absolute(path.slice(buf)))(Package.rs:2017), when it readspackage.json.Self-review. A review of the first draft found that the same two panics still fired from the pnpm copies of the idiom, that the
Package.rschange alteredadd --filterandremove --filterwith no test, that a test pinned an over-long outcome which #43322 decides differently, and that two claims in the draft were false (that the linker sites only see paths frompackage.jsonon POSIX, and that the problem there is Windows-only). All four are fixed in this version. It also proposed to land this as the base of #43322. The two diffs do not overlap insrc/, so either order works.Tests. Release build before (
USE_SYSTEM_BUN=1): 5 of the 7 fail. The two that pass (absolute key and missing from disk, absolute key and plain install) fail only on a debug build, where the child aborts. Debug build before: all 7 fail. After: all 7 pass on Linux debug/ASAN and on Windows x64 debug. On Windows theadd --filtertest uses the hoisted linker, because the isolated linker there fails to link the dependencies of a workspace outside the root (ENOENT, also on 1.4.3-canary with a plain install).Suites run on the Linux debug build with this change:
frozen-lockfile-pruned.test.ts(105 pass),bun-add-filter.test.ts(124),migration/pnpm-lock-migration.test.ts(7),migration/pnpm-lock-v9.test.ts(84),migration/pnpm-comprehensive.test.ts(5),migration/pnpm-migration.test.ts(3),migration/pnpm-migration-complete.test.ts(1),migration/migrate.test.ts(129),bun-workspaces.test.ts(82),bun-lock.test.ts(40),bad-workspace.test.ts(14),catalogs.test.ts(89),bun-prune.test.ts(121),bun-workspaces-self-contained.test.ts(24),bun-remove.test.ts(14),isolated-install.test.ts(85). On Windows x64 debug: the three changed files (105, 124, 7).no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/frozen-lockfile-pruned.test.ts, test/cli/install/bun-add-filter.test.ts