Conversation
|
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 (4)
WalkthroughChangesThe pnpm migration now copies pnpm migration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:30 PM PT - Aug 21st, 2026
❌ @robobun, your commit dad74aa has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38775That installs a local version of the PR into your bun-38775 --bun |
|
Status: reproduced on bun 1.4.0 with |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![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 user-facing bun pm migrate behavior (leaving the pnpm block in place instead of pruning it) and reworks the package.json cache-entry write path to fix a use-after-free, a human look would still be worthwhile.
Checked: copy_object gives the root a distinct property list so rewrite_bare_patch_keys and the workspace-yaml merge cannot mutate the pnpm block; copy_into_root preserves the old merge-vs-create semantics and now correctly skips announcing when the existing root field is not an object; the print_package_json_into_cache_entry + reparse_root sequence matches bun add/bun remove/add_catalog; wrote_workspaces still gates the write via copied.
Extended reasoning...
Overview
This PR changes update_package_json_after_migration in src/install/pnpm.rs so that migrating from pnpm copies pnpm.overrides / pnpm.patchedDependencies to root-level fields without removing them from the "pnpm" block (previously the block was pruned and deleted when empty). It also replaces the hand-rolled BufferPrinter + source.contents = Cow::Owned(...) sequence — which freed the old contents while the cached root tree still borrowed them — with the shared print_package_json_into_cache_entry + reparse_root pattern. Four near-identical merge-or-create blocks are collapsed into copy_into_root, and copy_object is added so edits to the root copy cannot alias back into the pnpm block. Docs and two test files are updated; three new tests cover the preserved block, the yaml-merge aliasing case, and the bun update UAF.
Security risks
None identified. No untrusted-input parsing paths change; the modified code operates on the already-parsed root package.json and pnpm-workspace.yaml trees.
Level of scrutiny
Medium-high. The functional half is a deliberate product decision (non-destructive migration so pnpm install --frozen-lockfile keeps working for teammates still on pnpm) — reasonable and well-argued, but a user-visible behavior change to bun install that a maintainer should sign off on. The memory-safety half touches AST-node lifetime rules (Store reset, source.contents borrowing, stale_contents retention) and, while the fix reuses the exact pattern from add_remove_with_filter.rs / add_catalog.rs / updatePackageJSONAndInstall.rs, it is the kind of change the repo review guide flags for careful reading.
Other factors
I traced shallow_clone_prop and confirmed copy_object builds a fresh PropertyList, so rewrite_bare_patch_keys reassigning prop.key on the copy cannot touch the original block's list. copy_into_root's Ok(false) branch (existing root field is not an object) is a slight behavior change from the old code — previously the message was printed and the pnpm block was still pruned even though nothing was merged; now nothing is announced or written. That is an improvement in a degenerate case, not a regression. The old silent return Ok(()) on print_json failure becomes a Global::crash() via the shared helper, matching every other caller. No dead imports remain (bun_js_printer is no longer referenced in pnpm.rs). Test coverage looks thorough, including the ASAN repro for bun update and a check that the bare patch key survives in the block while the root gets the versioned key.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Confirmed #23694 is this use-after-free: |
|
Independent reproduction of the same two problems landed on the same fix, so no second PR: branch Two notes for whoever merges:
|
|
Re the note above: the block-scoped yaml arena is fixed here too as of cb7af74 (the yaml is parsed into the function's |
There was a problem hiding this comment.
The arena-lifetime issue I flagged is fixed in cb7af74 (the yaml parse now uses the function-scope bump, and quoted pnpm-workspace.yaml scalars are written to package.json covers it). No further issues found this pass. Given this reworks arena/Store lifetimes in update_package_json_after_migration, changes the migration's user-visible contract for the pnpm block, and overlaps with #38754, a human look is still worthwhile.
What was reviewed:
&bumpnow backs the yaml parse;bumpat pnpm.rs:2348 outlivesprint_package_json_into_cache_entry, so quoted-scalarExprs stay valid through the print.copy_objectbuilds a freshE::Objectsorewrite_bare_patch_keysand workspace-yaml merges cannot mutate the retainedpnpmblock via the aliasedgetresult.copy_into_rootpreserves the old merge-vs-create semantics, including returningfalse(no "copied" line) when a non-object root field already exists.print_package_json_into_cache_entrystashes the old contents instale_contentsandreparse_rootfollows immediately, matching the patternbun add/--cataloguse.
Extended reasoning...
Overview
The PR touches src/install/pnpm.rs (update_package_json_after_migration and two new helpers), docs/pm/cli/install.mdx, and two test files. It (1) stops deleting pnpm.overrides/pnpm.patchedDependencies from the "pnpm" block during migration and copies them to root-level fields instead, (2) fixes a use-after-free where the migration replaced source.contents on the cached root package.json entry without rebuilding root (crashes bun update/bun update -i on first run in a pnpm project — #23694), (3) fixes a second UAF I flagged last pass where pnpm-workspace.yaml was parsed into a block-scoped arena whose quoted-scalar allocations were read after drop, and (4) collapses four copy-pasted merge blocks into copy_into_root plus a copy_object that severs the alias between the root copy and the pnpm block.
Security risks
None identified. No untrusted-input parsing was added; the yaml/json inputs were already being parsed on this path. The change reduces memory-safety risk by replacing a hand-rolled source.contents = Cow::Owned(...) (which left entry.root dangling) with the shared print_package_json_into_cache_entry + reparse_root sequence, and by extending the yaml arena's lifetime to cover its consumers.
Level of scrutiny
Medium-high. This is package-manager code on the migration path with two memory-lifetime fixes whose correctness depends on bun_alloc::Arena scope, the resettable Expr Store, and WorkspacePackageJSONCache invariants (stale_contents, reparse_root). The behavior change (keep the pnpm block) is a product decision — well-argued in the description, but still a user-visible contract change. The refactor is a net simplification (~130 lines deleted) and matches an in-tree helper pattern, but the aliasing subtlety that motivates copy_object is exactly the kind of thing a maintainer should sign off on.
Other factors
My previous inline finding was addressed in cb7af74 with a dedicated test using single- and double-quoted yaml scalars. Test coverage is thorough: new tests for both UAF repros (including the #23694 update -i shape driven via piped stdin), the block-kept invariant in both the mixed-keys and only-migrated-keys cases, the workspace-yaml-merges-into-root-not-pnpm-block case, and existing moved→copied assertions updated across two files. The comment-cop bot flags were resolved (comments are now single-line). The PR description notes a rebase will be needed against #38754 whichever lands second. CI build #97096 is in progress per the robobun comment.
cb7af74 to
43efc06
Compare
|
Rebased on main (43efc06). #39789 landed the pnpm-workspace.yaml arena fix in the meantime (it re-interns the escaping subtrees into DATA_STORE and has a deterministic MIMALLOC_PURGE_DELAY=0 test), so this PR's version of that fix (cb7af74, parse into |
There was a problem hiding this comment.
I re-reviewed after the rebase and didn't find any bugs. My earlier finding (the block-scoped pnpm-workspace.yaml arena) is resolved — that fix landed on main as #39789 and this diff no longer touches that code; the escaping subtrees are re-interned via data_store_dupe_expr_strings before the arena drops. Because this pairs a user-facing behavior change with a memory-lifetime refactor in the migration path, a human look would still be worthwhile.
What was reviewed:
copy_objectbuilds a fresh property list sorewrite_bare_patch_keysand workspace-yaml merges cannot alias into the survivingpnpmblock.print_package_json_into_cache_entry+reparse_rootmatches the pattern inadd_catalog.rs/add_remove_with_filter.rs; the helper stashes the old buffer intostale_contentsbefore swapping, so printing reads valid bytes and the cachedrootis rebuilt afterwards.copy_into_rootreturningfalsewhen a root-level field exists but is not an object drops the old code's spurious "moved …" line for that edge case; behavior otherwise preserved.- The four new tests cover the two UAF shapes (#23694's
update -idouble-migration and plainbun update) and both pnpm-block-survival variants; existingmoved→copiedstring assertions updated.
Extended reasoning...
Overview
Changes update_package_json_after_migration in src/install/pnpm.rs to (1) copy pnpm.overrides/pnpm.patchedDependencies to the root of package.json while leaving the "pnpm" block untouched, deleting ~90 lines of block-pruning code; (2) collapse four near-identical merge-or-create blocks into two helpers (copy_object, copy_into_root); (3) replace the hand-rolled source.contents = Cow::Owned(...) write-out with the shared print_package_json_into_cache_entry + reparse_root sequence, fixing a use-after-free (#23694) where bun update / bun update -i edited a cached AST whose strings pointed into a freed buffer. Docs updated to say "copied" and explain why the block is kept. Two test files updated: four new tests, several existing assertions changed from "moved" to "copied" and to expect the surviving pnpm block.
Security risks
None. This is package-manager migration logic operating on the user's own package.json and lockfile; no auth, crypto, network parsing, or new untrusted-input surface. The memory-safety fix removes a UAF rather than adding attack surface.
Level of scrutiny
Medium-high. The repo's review guide names native memory safety as the most-blocked category, and this touches AST-node lifetime across the WorkspacePackageJSONCache boundary. I traced print_package_json_into_cache_entry — it prints from the tree while the old contents are still live, then mem::replaces and pushes the old buffer into stale_contents, and reparse_root rebuilds entry.root from the new bytes; that matches the three other callers. copy_object allocates a new PropertyList via Expr::init (thread-local Store), which is fine because the tree is printed and re-parsed before the next initialize_store(). My earlier inline finding on this PR — the block-scoped yaml arena freeing quoted-scalar bytes before use — is now handled on main by re-interning the escaping subtrees (data_store_dupe_expr_strings, #39789), and this diff no longer touches that block.
Other factors
The behavior change (keep the pnpm block so pnpm install --frozen-lockfile keeps working for teammates still on pnpm) is a product decision a maintainer should sign off on, though the rationale is well-argued and bun ignoring the block is already pinned by bun-workspaces.test.ts. The PR description flags a known merge overlap with #38754 in the same function tail. All prior bot feedback (comment-cop, my yaml-arena finding) is resolved. Test coverage includes both ASAN-reproducing UAF shapes plus the block-survival and yaml-merge-isolation invariants; the --frozen-lockfile follow-ups guard idempotence.
There was a problem hiding this comment.
I reviewed this PR and found no bugs. My earlier finding (the block-scoped pnpm-workspace.yaml arena UAF) was addressed — #39789 landed the fix on main and this PR is rebased on it, so the yaml block is no longer touched here. Because this changes user-visible migration behavior (the pnpm block is now kept, root-level fields duplicated) and the description flags a merge-order overlap with #38754 in the same function, a human sign-off on the product decision and merge coordination would still be worthwhile.
What was reviewed:
copy_object/copy_into_rootagainst the four call sites they replace — same merge semantics, and the fresh property list meansrewrite_bare_patch_keysand yaml merges no longer alias thepnpmblock.print_package_json_into_cache_entry+reparse_rootmatches the exact pattern inadd_catalog.rs,add_remove_with_filter.rs, andupdatePackageJSONAndInstall.rs;stale_contents.push(old)keeps the printed-from buffer alive until reparse.- Checked that
copied.is_empty()gates the write the same wayneeds_updatedid — every path that pushed tomovednow pushes tocopied, andwrote_workspacesis still covered.
Extended reasoning...
Overview
This PR changes update_package_json_after_migration in src/install/pnpm.rs to copy pnpm.overrides / pnpm.patchedDependencies to the root of package.json instead of moving them, leaving the "pnpm" block untouched. It deletes ~130 lines of block-pruning logic, extracts the four identical merge-or-create blocks into a shared copy_into_root helper, and adds copy_object so the root copy is a distinct property list (the previous code aliased the same E::Object, so rewrite_bare_patch_keys on the root copy would have mutated the pnpm block too now that the block is kept). It also swaps the hand-rolled print-and-assign-contents for print_package_json_into_cache_entry + reparse_root, which is the established pattern used by bun add --catalog, bun add --filter, and the update-then-install path — this fixes a heap-use-after-free (#23694) where bun update on a pnpm project edited a cached tree whose strings pointed into the freed previous source.contents. Docs and two test files are updated; four new tests cover the kept block, the yaml-vs-block merge, and the two bun update UAF shapes.
Security risks
None identified. The change reads and rewrites the user's own package.json and pnpm-workspace.yaml; no network, no untrusted-archive paths, no auth. The memory-safety change moves from a bespoke buffer swap to the shared helper that already stashes the old contents in stale_contents, which is strictly safer.
Level of scrutiny
Moderate-to-high. The function edits an arena-backed AST whose string nodes borrow from a buffer that gets replaced, which is exactly the class of bug this PR fixes; any writer here needs to get the print → stash-old → reparse sequence right. I verified the new sequence matches the three existing callers byte-for-byte (including the Global::crash() on reparse failure). The behavior change itself — keeping the pnpm block — is a product decision: it changes what a migrated package.json looks like and is announced differently ("copied" vs "moved"). The reasoning is sound (bun ignores the block, pnpm requires it, removing it breaks pnpm install --frozen-lockfile for teammates), but it's a user-visible policy change that a maintainer should ack.
Other factors
- My prior review on this PR flagged the block-scoped
pnpm-workspace.yamlarena dropping before its subtrees were consumed. That was fixed here in cb7af74, then independently on main as #39789 (which re-interns escaping strings viadata_store_dupe_expr_strings); this PR is now rebased on #39789 and no longer touches that block. The current diff'scopy_into_rootcalls forworkspace_overrides_obj/workspace_patched_deps_objrun afterdata_store_dupe_expr_stringshas re-interned every string, so the arena concern is closed. - The
if !copied.is_empty()gate is equivalent to the oldneeds_update: every branch that previously setneeds_update = truealso pushed tomoved, and each of those now pushes tocopied(includingwrote_workspaces). The only semantic tweak is thatcopy_into_rootreturnsfalse(and does not push) when a root-leveloverrides/patchedDependenciesexists but is not an object — the old code silently did nothing but still setneeds_updateand printed "moved …", so the new behavior is more honest. - Tests look solid: the two UAF tests are described as failing on main with ASAN and passing here; the block-kept tests use
toStrictEqualon the wholepackage.json; existing "moved" assertions in both test files were updated to "copied". - The description explicitly calls out that #38754 rewrites the same function tail and whichever lands second needs a rebase; that's a merge-coordination item for a human.
…ites it (#40029) ### Problem - `bun add` in a project that still has `pnpm-lock.yaml` and `pnpm-workspace.yaml` crashes or writes garbage into `package.json`. Sentry BUN-4NQS (1.4.0): `Segmentation fault at address 0xF3E9AD2800000000` in `extend_from_slice` under `EString::resolve_rope_if_needed`, `print_property`, `edit_after_resolve`. `bun update -i` hits the same bug (#23694). - The cause is `update_package_json_after_migration` (`src/install/pnpm.rs:2334`). It edits the cached root `package.json` tree in place, replaces `entry.source.contents`, and returns. The tree now points into the freed contents, and its new nodes live in the thread-local AST `Store`, which the install resets (`src/install/lockfile/Package.rs:1676`) before the write-back prints the tree. ### Fix - Print the edited tree into the cache entry and call `MapEntry::reparse_root`, as `store_entry` (`add_remove_with_filter.rs:280`) does. When the migration returns, the entry owns its tree again. - A print or re-parse failure now crashes like the other editors do. Before, a print failure left the dangling tree in the cache. - Verified: two new tests in `test/cli/install/migration/pnpm-lock-v9.test.ts` (`bun add`, and the `bun update -i` shape from #23694). Both fail on main with the debug build and with the 1.4.0 release. The other `pnpm-*` files pass. ### Background - `WorkspacePackageJSONCache` keeps one `MapEntry` per `package.json`: the contents, a parsed tree, and the arena that owns the tree. `bun add` prints the root entry again after the install (`package_json_write_back.rs:63`). - `Expr::init` allocates nodes in a thread-local `Store`. `initialize_store()` resets it before every `package.json` parse. `reparse_root` rebuilds the tree in an arena the entry owns. - String nodes point into the entry's contents, so a writer that replaces the contents has to rebuild the tree. <details><summary>Notes</summary> - Repro without a registry: a `package.json`, a `pnpm-lock.yaml` with `lockfileVersion: '9.0'` and empty importers, a `pnpm-workspace.yaml` with a `packages:` list, then `bun add ./some-folder`. The 1.4.0 release writes `"\x00\x00\x00e"` in place of the `name` key, so `package.json` is no longer valid JSON. The debug build reports `heap-use-after-free` in `EString::eql_bytes` under `edit_after_resolve`, freed at `pnpm.rs:2759` (the `source.contents` assignment on main). Any yaml field the migration moves (`packages`, `catalog`, `overrides`, `patchedDependencies`) or a `pnpm.overrides` block triggers it, so every pnpm workspace does. - Why the two builds differ: the debug `Store` reset poisons its blocks with `0xAA`, so the second print crashes every time. In release, the freed contents buffer gets a free-list pointer written over its first bytes (the `name` key), and the `Store` slots are reused by the install's parse of the root `package.json`, which gives the rope walk in the Sentry stack. - Readers of the root entry's tree after the lockfile load, all covered by the one re-parse: the `bun add` / `bun update <name>` / `bun link` write-back and `sync_lockfile` (the Sentry crash), `bun update -i` (#23694), `bun update` with patches (`warn_orphaned_patches`), `bun dedupe` with an empty root, `bun audit --fix`, and the `--frozen-lockfile` error message (`overrides_field_name`). Plain `bun install` only reads `entry.source` after the migration, so it does not crash. - The open #38775, #38754, and #38804 each add this same print and re-parse to `update_package_json_after_migration` as part of larger behavior changes (all three are in the fold branch #39403). None of them is in 1.4.0. This PR is only the crash fix, so it can land on its own. Whichever lands second gets a conflict in this block that resolves to the same code. - Not changed here: the `patch --commit` branch in `update_package_json_and_install_with_manager_with_updates` (`updatePackageJSONAndInstall.rs:573`) replaces the root entry's contents the same way without a re-parse. Nothing reads that entry's tree afterwards today, and #38754 reworks that block. Also separate: `bun remove` during a pnpm migration writes its pre-install print back to disk and loses the fields the migration moved. That is a different bug and is tracked on its own. - Suites run with the debug build: `pnpm-lock-v9` (84 pass), `pnpm-migration`, `pnpm-lock-migration`, `pnpm-comprehensive`, `pnpm-migration-complete` (the last one is a single test that sits near the 5 s default timeout when run next to other files, unrelated to this change). `complex-workspace` and `yarn-lock-migration` need network access and fail here with the release build too. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 2 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/cli/install/migration/pnpm-lock-v9.test.ts" bun test v1.4.1 (4448a2e) test/cli/install/migration/pnpm-lock-v9.test.ts: (pass) pnpm-lock.yaml v9 > v9 git and userinfo-tarball references migrate [372.39ms] (pass) pnpm-lock.yaml v9 > v9 alias in snapshot optionalDependencies gets the npm: prefix [531.51ms] (pass) pnpm-lock.yaml v9 > reports the missing packages entry of a workspace importer [188.78ms] (pass) pnpm-lock.yaml v9 > reports the missing packages entry of a transitive dependency [164.22ms] (pass) pnpm-lock.yaml v9 > reports the missing packages entry [227.90ms] (pass) pnpm-lock.yaml v9 > reports an importer whose package.json is missing [170.19ms] (pass) pnpm-lock.yaml v9 > registry-qualified dep path resolves from the configured registry with a warning [258.83ms] (pass) pnpm-lock.yaml v9 > named registries > built-in npmjs: entries record the npmjs registry [214.16ms] (pass) pnpm-lock.yaml v9 > named registries > namedRegistries entry pointing at the configured registry needs no warning [445.93ms] (pass) pnpm-lock.yaml ... (truncated) release without fix: 2 FAILED bun test v1.4.0-canary.1 (4448a2e) test/cli/install/migration/pnpm-lock-v9.test.ts: (pass) pnpm-lock.yaml v9 > v9 git and userinfo-tarball references migrate [9.21ms] (pass) pnpm-lock.yaml v9 > v9 alias in snapshot optionalDependencies gets the npm: prefix [49.92ms] (pass) pnpm-lock.yaml v9 > reports the missing packages entry [5.92ms] (pass) pnpm-lock.yaml v9 > reports the missing packages entry of a workspace importer [4.76ms] (pass) pnpm-lock.yaml v9 > reports the missing packages entry of a transitive dependency [3.30ms] (pass) pnpm-lock.yaml v9 > reports an importer whose package.json is missing [3.40ms] (pass) pnpm-lock.yaml v9 > registry-qualified dep path resolves from the configured registry with a warning [13.91ms] (pass) pnpm-lock.yaml v9 > named registries > built-in npmjs: entries record the npmjs registry [9.93ms] (pass) pnpm-lock.yaml v9 > named registries > namedRegistries entry pointing at the configured registry needs no warning [20.87ms] (pass) pnpm-lock.yaml v9 > named registries > namedRegistries entry pointing at another registry is used for the tarballs [19.37ms] (pass) pnpm-lock.yaml v9 > named registries > two packages from one unknown re ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/cli/install/migration/pnpm-lock-v9.test.ts" bun test v1.4.1 (4448a2e) test/cli/install/migration/pnpm-lock-v9.test.ts: (pass) pnpm-lock.yaml v9 > v9 git and userinfo-tarball references migrate [309.70ms] (pass) pnpm-lock.yaml v9 > v9 alias in snapshot optionalDependencies gets the npm: prefix [405.45ms] (pass) pnpm-lock.yaml v9 > reports the missing packages entry [167.07ms] (pass) pnpm-lock.yaml v9 > reports the missing packages entry of a workspace importer [174.28ms] (pass) pnpm-lock.yaml v9 > reports the missing packages entry of a transitive dependency [172.30ms] (pass) pnpm-lock.yaml v9 > reports an importer whose package.json is missing [157.78ms] (pass) pnpm-lock.yaml v9 > registry-qualified dep path resolves from the configured registry with a warning [197.44ms] (pass) pnpm-lock.yaml v9 > named registries > built-in npmjs: entries record the npmjs registry [193.97ms] (pass) pnpm-lock.yaml v9 > named registries > namedRegistries entry pointing at the configured registry needs no warning [335.05ms] (pass) pnpm-lock.yaml ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 2a65176 features baseline 23 deps, 129 codegen, 1172 objects in 1101ms ninja: Entering directory `/workspace/bun/build/release' [1/1244] install /workspace/bun bun install v1.4.0-canary.1 (4448a2e) Checked 26 installs across 63 packages (no changes) [7.00ms] [2/1244] install /workspace/bun/packages/bun-error bun install v1.4.0-canary.1 (4448a2e) Checked 1 install across 2 packages (no changes) [2.00ms] [3/1244] gen ErrorCode+*.h [4/1244] fetch tinycc [tinycc] up to date [5/1243] install /workspace/bun/src/node-fallbacks bun install v1.4.0-canary.1 (4448a2e) Checked 111 installs across 104 packages (no changes) [10.00ms] [6/1243] gen bindgenv2 [7/1243] fetch zlib [zlib] up to date [8/1243] fetch libjpeg-turbo [libjpeg-turbo] up to date [9/1216] gen node-fallbacks/react-refresh.js Bundled 1 module in 10ms react-refresh.js 4.81 KB (entry point) [10/1216] gen .bind.ts → GeneratedBindings.cpp [11/1216] gen ProcessBindingConstants.lut.h Generating /workspace/bun/bu ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/install/pnpm.rs | 37 ++------ test/cli/install/migration/pnpm-lock-v9.test.ts | 113 ++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 29 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/install/pnpm.rs 4 3 0 test/cli/install/migration/pnpm-lock-v9.test.ts 3 2 0 ``` </details> <!-- robobun:evidence:end -->
The pnpm-lock.yaml migration copied pnpm.overrides and pnpm.patchedDependencies to the root of package.json and then deleted them from the pnpm block (deleting the block itself when nothing else was in it). pnpm only reads its configuration from that block, so after one bun install, pnpm install --frozen-lockfile failed for everyone still on pnpm with ERR_PNPM_LOCKFILE_CONFIG_MISMATCH, and a plain pnpm install dropped the overrides from pnpm-lock.yaml. The entries are now copied and the block is left as it was. The copy written to the root is a separate object, so rewriting bare patch keys to name@version and merging in pnpm-workspace.yaml entries no longer shows up inside the block. The four merge-into-root blocks share one helper, and the message says "copied" instead of "moved". The rewritten package.json now goes through print_package_json_into_cache_entry and reparse_root like the other package.json editors. Before, the cache entry kept the edited tree, which pointed into the contents buffer that had just been freed and into Store nodes that the next initialize_store() resets, so bun update run directly on a pnpm project read freed memory when it edited the root package.json after the migration.
972d2a5 to
dad74aa
Compare
|
Rebased on main again (dad74aa). #40029 landed the print + re-parse write-back that this PR also carried, so that half is dropped here and the conflict in the tail of |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs — the earlier yaml-arena UAF I flagged is resolved by the rebase onto #39789. Because this changes user-facing migration behavior (keeping vs. deleting the pnpm block), refactors Store-allocated AST manipulation in update_package_json_after_migration, and has an acknowledged overlap with open #38754, a human look would still be worthwhile.
What was reviewed
copy_object/copy_into_root: verified all four call sites pass objects (guarded byis_non_empty_object/get_object), and the non-object-existing-root case now correctly returnsfalsewithout announcing a copy.copied.is_empty()gate replacingneeds_update: confirmed no case writes to the tree without pushing tocopied, so the print/reparse still fires whenever the tree changed.- Store-allocated
Expr::initincopy_object: nodes are consumed byprint_package_json_into_cache_entrythen discarded byreparse_rootbefore anyinitialize_store(), matching the pattern inbun add.
Extended reasoning...
Overview
This PR changes update_package_json_after_migration in src/install/pnpm.rs to copy pnpm.overrides and pnpm.patchedDependencies into root-level fields instead of moving them (the pnpm block is left intact). It deletes ~130 lines of block-pruning logic, replaces four duplicated merge-or-create blocks with two helpers (copy_object, copy_into_root), updates the user-facing message from moved to copied, and updates docs/tests accordingly. It also carries a fix for an ASAN-confirmed use-after-free in bun update on a pnpm project (#23694), covered by two new regression tests that fail on main.
Security risks
None. The change reads user-controlled JSON/YAML that is already parsed by existing code paths; no new untrusted-input parsing, no filesystem/network/permission changes.
Level of scrutiny
Moderate-to-high. The Rust side manipulates borrowed AST nodes across arena and Store allocators — the same class as the two UAFs already found and fixed during this PR's review cycle. The helpers are straightforward and the print-then-reparse pattern is the established one, but AST-lifetime code in the installer has bitten before. The behavior change (keep the pnpm block) is a product decision: the argument is convincing (bun ignores the block, pnpm requires it, deleting it breaks teammates' pnpm install --frozen-lockfile), but a maintainer should sign off on it.
Other factors
- Test coverage is thorough: 86 + 144 passing tests across the two touched files, new tests verified to fail under ASAN on main and pass with the fix, plus a
bun update -iregression test for #23694. - My earlier inline finding (block-scoped yaml arena dropping before its Exprs were printed) was resolved: #39789 landed on main and this PR is rebased on it, so that code is no longer touched here.
- The PR description flags an overlap with open #38754 in the tail of this function — whichever lands second needs a small rebase, and #38754's
moved ...test strings becomecopied .... That coordination is a reason for a maintainer to sequence the two. - All prior bot feedback (comment-cop paragraph-comment warnings) has been addressed; comments in the touched function are now single-line.
This branch has not been deployed
2 participants
Problem
bun install/bun pm migrateon a project with apnpm-lock.yamlcopiespnpm.overridesandpnpm.patchedDependenciesto the root ofpackage.jsonand then removes them from the"pnpm"block (update_package_json_after_migration,src/install/pnpm.rs; the block itself is removed when nothing else is in it). Since install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333 this is announced asmoved pnpm.overrides to overrides in package.json; the removal itself dates back to the original migration (implement pnpm migration #22262)."pnpm"block and ignores the root-level fields. After onebun installis committed,pnpm install --frozen-lockfilefails for everyone still on pnpm withERR_PNPM_LOCKFILE_CONFIG_MISMATCH, and a plainpnpm installdrops theoverrides:section frompnpm-lock.yaml, so the pins stop applying."pnpm"block (OverrideMapandPackageonly read the root-leveloverrides/resolutions/patchedDependencies;bun-workspaces.test.tspins that rootpnpm.overridesis ignored), so leaving the block in place costs nothing.Fix
pnpm.overrides/pnpm.patchedDependenciesare copied and the"pnpm"block is left as it was. The block-pruning code is deleted.copy_objectgives the root a separate object. TheExprreturned bygetaliases the object inside the block, so without the copy,rewrite_bare_patch_keys(barenamekeys becomename@versionfor bun) and the merge ofpnpm-workspace.yamlentries would now show up inside the"pnpm"block.pnpm-workspace.yaml, overrides and patchedDependencies) sharecopy_into_root.needs_updateis gone: the file is written when the list of copied items is not empty. The message readscopied ... in package.json, since nothing it lists is modified at its source. Themovedassertion that install: re-parse the root package.json after the pnpm migration rewrites it #40029 added tobun add migrates pnpm-workspace.yaml ...is updated with it.docs/pm/cli/install.mdxsays the fields are copied and the"pnpm"block is kept.test/cli/install/migration/pnpm-lock-v9.test.ts(88 pass) andtest/cli/install/nested-overrides.test.ts(144 pass). Fail on main, pass here:the pnpm block in package.json is left as it was(a block with other settings, and a block that holds only the migrated keys),pnpm-workspace.yaml overrides go into the root copy, not into the pnpm block,bare hash whose path is only in package.json pnpm.patchedDependencies(the bare key stays in the block, the root getsno-deps@1.0.1), the twobun updatetests (the block survivesbun update's own re-print of package.json, andupdate -i's second migration pass merges into the copies instead of duplicating them), and the existing assertions that encoded the removal. Also ranpnpm-lock-migration,migrate,lockfile-only,pnpm-migration,pnpm-comprehensive,pnpm-migration-complete.Background
"pnpm"key ofpackage.json; pnpm 10 also accepts them inpnpm-workspace.yaml, and the lockfile records the merged set. bun uses npm's root-leveloverridesand its own root-levelpatchedDependencies, which is why the migration writes root-level copies. The migration leavespnpm-lock.yamlandpnpm-workspace.yamluntouched; the"pnpm"block was the one place it edited pnpm's own configuration.update_package_json_after_migrationedits the parsed tree of the rootpackage.jsonin place and prints it. Since install: re-parse the root package.json after the pnpm migration rewrites it #40029 it then re-parses the printed text into the cache entry, so the copies made here only have to live until that print.patchedDependencieskey ("no-deps") is legal for pnpm;bun.lockkeys patches byname@version, so the migration rewrites bare keys in the root copy using the version the lockfile resolved.moved ...test strings becomecopied ...once this is in.Notes
History of this PR: it originally also carried the
print_package_json_into_cache_entry+reparse_rootwrite-back (a use-after-free hit bybun updateand by #23694'sbun update -i, found while testing this change) and a fix for the block-scopedpnpm-workspace.yamlarena raised in review. Both landed on main on their own while this was open (#40029 and #39789) and were dropped from here on rebase, so this PR is only the behavior change again. #23694 is fixed by #40029.Reproduction on bun 1.4.0 (release):
With this change the
pnpmblock is unchanged, the root-leveloverridesis added, and the line printed iscopied pnpm.overrides to overrides in package.json.bun update -iprints that line twice because it runs the migration twice; the second pass merges identical values and leaves the file as the first pass wrote it.[review] gate passed · iteration 6 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 6
evidence per changed file