install: stop reading dangling dependency pointer in enqueueDependencyToRoot#29483
Conversation
…yToRoot enqueueDependencyToRoot passed &lockfile.buffers.dependencies.items[dep_id] into enqueueDependencyWithMainAndSuccessFn. When the manifest for that package is already cached (on disk or in memory) but the tarball is not, getOrPutResolvedPackageWithFindResult calls Lockfile.Package.fromNPM which grows buffers.dependencies via ensureUnusedCapacity, reallocating the backing storage. The subsequent .extract branch then read dependency.behavior from the freed buffer (ASAN: use-after-poison). All other callers already copy the Dependency to the stack before passing its address; do the same here. Also switch the one post-resize read to use the behavior parameter that was already copied by value.
|
Updated 5:33 PM PT - Apr 19th, 2026
❌ @robobun, your commit a7d5b22 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 29483That installs a local version of the PR into your bun-29483 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCopied dependency values to local stack variables before passing their addresses into enqueue routines to prevent dangling-pointer/read-after-resize issues; fixed a Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Same hazard as enqueueDependencyToRoot: the overrides_changed and catalogs_changed loops in install_with_manager.zig captured |*dependency| directly from buffers.dependencies.items and passed it to enqueueDependencyWithMain, which can call Lockfile.Package.fromNPM and reallocate that buffer. Both the captured pointer and the for-loop's own slice would then be dangling. Iterate by index against a snapshot of the original length and copy each entry to the stack, matching the add/update loop immediately below and every other caller.
![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 the current code and only fix it if needed.
Inline comments:
In `@test/cli/run/autoinstall-cached-manifest.test.ts`:
- Around line 50-55: The test currently removes non-manifest files from cacheDir
without asserting any were present; update the test around the entries variable
so it first filters non-manifest entries (entries that do NOT endWith(".npm")),
assert that there is at least one non-manifest entry to remove
(expect(nonManifestEntries.length).toBeGreaterThan(0)), then perform the rmSync
loop on those non-manifest entries and optionally assert that the remaining
directory contains only .npm manifests; refer to the entries array and the
cacheDir variable in your changes.
- Around line 30-31: The test is swallowing require() failures (the script
`const script = ...` catches errors and still prints "ok"), which yields false
positives; update the `script` strings (the `script` variable occurrences around
the failing cases) so that a failed require either is not caught or the catch
rethrows/logs and exits non‑zero (e.g., rethrow the error or call
process.exit(1) in the catch) so that the test fails when auto-install did not
succeed.
🪄 Autofix (Beta)
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: d98a7150-6686-4e6f-b2cc-3c45eab217a9
📒 Files selected for processing (1)
test/cli/run/autoinstall-cached-manifest.test.ts
| const script = `try { require("is-even"); } catch {} console.log("ok");`; | ||
|
|
There was a problem hiding this comment.
Don’t swallow require() failures; this can produce false positives.
The current script prints "ok" even if require("is-even") fails, so the test can pass without validating that auto-install actually succeeded.
Suggested fix
- const script = `try { require("is-even"); } catch {} console.log("ok");`;
+ const script = `require("is-even"); console.log("ok");`;Also applies to: 44-45, 69-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cli/run/autoinstall-cached-manifest.test.ts` around lines 30 - 31, The
test is swallowing require() failures (the script `const script = ...` catches
errors and still prints "ok"), which yields false positives; update the `script`
strings (the `script` variable occurrences around the failing cases) so that a
failed require either is not caught or the catch rethrows/logs and exits
non‑zero (e.g., rethrow the error or call process.exit(1) in the catch) so that
the test fails when auto-install did not succeed.
There was a problem hiding this comment.
Keeping the try/catch intentionally. This test is about whether the process crashes during auto-install resolution (use-after-poison → SIGABRT), not whether is-even loads — the crash happens inside the resolver before the module is evaluated, so a bare require that throws for an unrelated reason would just add flakiness.
The false-positive concern is addressed instead by d415c6e, which asserts the first run actually produced both manifests and extracted tarballs in the cache. If auto-install didn't reach the registry, the test now fails at that assertion rather than silently skipping the interesting path.
There was a problem hiding this comment.
LGTM — the overrides/catalogs loop fixes from my prior review are in place.
Extended reasoning...
Overview
The PR fixes a use-after-poison in (PackageManagerEnqueue.zig) and a second set of equivalent UAF bugs in the overrides_changed and catalogs_changed loops in install_with_manager.zig. All three fixes apply the same stack-copy pattern already used everywhere else in the code: copy the value to the stack before taking its address and passing it into . A regression test covers the original crash path (warm manifest cache, missing tarball, runtime auto-install).
Security risks
No security-sensitive surfaces are touched. The bugs fixed are memory-safety issues (use-after-poison), not authentication or permission concerns.
Level of scrutiny
This is a targeted, minimal bug fix in the package manager install path. Each change is mechanical (for-loop pointer capture → index-based loop with stack copy, or single parameter substitution), and the pattern is already established by the surrounding code. The fix is low risk relative to leaving the UAF in place.
Other factors
My previous review (at 10:56 UTC) identified the additional bugs in install_with_manager.zig. The author addressed them in commit 8a3d0c7. The only open item from the bug tracker is a pre-existing memory leak with no user-visible impact (CLI process; OS reclaims at exit). All substantive concerns have been resolved.
| if (manager.summary.overrides_changed and all_name_hashes.len > 0) { | ||
| for (manager.lockfile.buffers.dependencies.items, 0..) |*dependency, dependency_i| { | ||
| const dependencies_len = manager.lockfile.buffers.dependencies.items.len; | ||
| var dependency_i: usize = 0; | ||
| while (dependency_i < dependencies_len) : (dependency_i += 1) { | ||
| const dependency = manager.lockfile.buffers.dependencies.items[dependency_i]; | ||
| if (std.mem.indexOfScalar(PackageNameHash, all_name_hashes, dependency.name_hash)) |_| { | ||
| manager.lockfile.buffers.resolutions.items[dependency_i] = invalid_package_id; | ||
| manager.enqueueDependencyWithMain( | ||
| @truncate(dependency_i), | ||
| dependency, | ||
| &dependency, | ||
| invalid_package_id, | ||
| false, | ||
| ) catch |err| { | ||
| addDependencyError(manager, dependency, err); | ||
| addDependencyError(manager, &dependency, err); | ||
| }; | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟣 Pre-existing memory leak: all_name_hashes is allocated with bun.default_allocator.alloc at line 242 of install_with_manager.zig when overrides_changed is true and hashes_len > 0, but there is no corresponding defer bun.default_allocator.free(all_name_hashes) anywhere in the function. The fix is to add a deferred free immediately after the allocation block, guarded on all_name_hashes.len > 0.
Extended reasoning...
What the bug is and how it manifests
The all_name_hashes slice is conditionally allocated via bun.default_allocator.alloc(PackageNameHash, hashes_len) inside the brk block around line 242 of install_with_manager.zig, when both manager.summary.overrides_changed is true and hashes_len > 0. The two early-exit paths (overrides_changed == false and hashes_len == 0) return &.{} (empty slices that require no free), but the actual allocation path has no corresponding defer bun.default_allocator.free(all_name_hashes). Grep confirms no free of this variable exists anywhere in the file.
The specific code path that triggers it
- bun install runs with changed overrides (e.g., adding or removing an override in package.json).
- manager.summary.overrides_changed is true; hashes_len is non-zero.
- bun.default_allocator.alloc(PackageNameHash, hashes_len) succeeds and allocates a small heap buffer.
- The buffer is used by the new overrides_changed loop (lines 368-384) added by this PR.
- installWithManager returns; the buffer is leaked — no free is ever called.
Why existing code does not prevent it
The three early-return arms inside the brk block all produce &.{}, which correctly requires no cleanup. However, the successful allocation arm does not register a defer. The PR touched this exact code to fix the dangling-pointer bug in the loops below, but did not add the missing cleanup.
What the impact would be
For a short-lived CLI process like bun install, the OS reclaims all memory at exit, so there is zero user-visible effect in practice. However, under leak-detection builds (ASAN, Valgrind, or custom allocator tracking), the missing free will surface as a definite leak. The allocation is proportional to the number of override entries (a few u64 values), so it is always small.
How to fix it
Add a deferred free immediately after the allocation block, guarded to only run when actual allocation occurred:
This mirrors the standard Zig idiom and is consistent with the CLAUDE.md guidance to use defer for allocator cleanup.
Step-by-step proof
- User runs bun install after adding an override to package.json.
- overrides_changed = true, hashes_len = 2 (one old + one new override key).
- bun.default_allocator.alloc(PackageNameHash, 2) returns a 16-byte heap buffer at address A.
- The overrides_changed loop at lines 368-384 reads from address A.
- installWithManager returns normally; no free is called for address A.
- Under ASAN, the allocator reports a 16-byte leak from this allocation site. Under normal builds, the OS reclaims it at process exit.
There was a problem hiding this comment.
Noted — this one's pre-existing and unrelated to the dangling-pointer fix (the allocation was already there before this PR touched the loops below it). Leaving it out to keep the diff focused on the UAF; it's a few u64s in a short-lived CLI process so there's no user-visible impact.
Ensures the test actually exercises the cached-manifest + missing-tarball path instead of silently becoming a no-op if the first auto-install run did not reach the registry.
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
There was a problem hiding this comment.
LGTM — the author addressed the pointer-safety issue I flagged in the overrides/catalogs loops; the remaining test nits are minor and already called out by other reviewers.
Extended reasoning...
Overview
The PR fixes a use-after-poison in the runtime auto-install path: enqueueDependencyToRoot was passing a raw interior pointer into lockfile.buffers.dependencies.items to enqueueDependencyWithMainAndSuccessFn, which could call Lockfile.Package.fromNPM, reallocating the backing storage and making the pointer dangling. Two changes: copy the Dependency to the stack before passing its address in enqueueDependencyToRoot, and use the already-passed behavior parameter instead of re-dereferencing dependency in the .extract branch.
A follow-up commit (8a3d0c7) applied the same stack-copy fix to the overrides_changed and catalogs_changed loops in install_with_manager.zig, which had the same vulnerability. Both commits are now on main.
Security risks
No security-sensitive code paths (auth, crypto, permissions). The bug was a memory-safety issue (UAF) triggered through the package install path. The fix is safe and mechanical.
Level of scrutiny
The core logic change is small and follows an established pattern already used by every other caller of enqueueDependencyWithMainAndSuccessFn. The test is a new regression file that exercises the specific scenario (warm manifest cache, missing tarball). Scrutiny is appropriate for a focused memory-safety fix with clear intent.
Other factors
The remaining open items (test try/catch swallowing require() failures, missing assertion that tarballs were actually removed, pre-existing all_name_hashes leak) are nits already flagged by CodeRabbit and the bug hunter. None affect the correctness of the core fix. The memory leak is pre-existing and inconsequential in a short-lived CLI process.
There was a problem hiding this comment.
LGTM — all prior concerns addressed; the stack-copy fixes are correct and consistent with existing patterns throughout the codebase.
Extended reasoning...
Overview
The PR fixes a use-after-poison crash (Fuzzilli-found) in three places: enqueueDependencyToRoot in PackageManagerEnqueue.zig, and the overrides_changed/catalogs_changed loops in install_with_manager.zig. All three now copy the Dependency value to the stack before passing its address to enqueueDependencyWithMainAndSuccessFn/enqueueDependencyWithMain, which can call Lockfile.Package.fromNPM and reallocate buffers.dependencies. A secondary fix uses the behavior parameter (already passed by value) instead of re-dereferencing the potentially-stale dependency pointer in the .extract branch.
Security Risks
No security-sensitive code (auth, crypto, permissions) is touched. The bug itself was a memory-safety issue (use-after-poison → crash) with no exploitation surface.
Level of Scrutiny
The changes are mechanical stack-copy fixes that match the pattern used by every other caller in the same file (processDependencyListItem, the add/update loop at line 407, etc.). The diff is small, focused, and self-evidently correct.
Other Factors
All reviewer feedback from prior rounds has been addressed: the two additional UAF loops in install_with_manager.zig were fixed in response to my earlier inline comments; the test now asserts both manifests and extracted tarballs are present before pruning; the intentional try/catch is justified because the regression manifests as a native SIGABRT (not a JS exception). The pre-existing all_name_hashes memory leak is out of scope and has no user-visible impact in a short-lived CLI process.
There was a problem hiding this comment.
LGTM — all UAF fixes are correct, the stack-copy pattern matches existing callers, and all prior feedback has been addressed.
Extended reasoning...
Overview
The PR fixes a use-after-poison in the runtime auto-install path. Three targeted changes: (1) enqueueDependencyToRoot now copies the Dependency to the stack before passing its address; (2) a subsequent read in getOrPutResolvedPackageWithFindResult now uses the already-passed-by-value behavior parameter instead of re-dereferencing dependency after a potential realloc; (3) the overrides/catalogs loops in install_with_manager.zig now iterate by index against a snapshot length and copy each entry to the stack. A regression test was added that warms the cache, prunes extracted tarballs, and asserts the second run succeeds.
Security risks
No security implications. The change eliminates UB/memory-safety issues rather than introducing new ones. No auth, crypto, or permission-sensitive paths are touched.
Level of scrutiny
Medium. This touches the package-manager install hot path, but the changes are mechanical and minimal -- identical stack-copy patterns are already used throughout the same file by processDependencyListItem, processPeerDependencyList, and the add/update loop. The fix is consistent with the code's own existing defensive comments.
Other factors
All prior reviewer concerns were addressed: the overrides/catalogs loop fix landed in a follow-up commit, the test now asserts both manifests and extracted tarballs are present before pruning, and the intentional try/catch was explained clearly (ASAN crashes produce non-zero exit regardless of JS exception handling). No bugs found by the automated system.
robobun
force-pushed
the
farm/90e5a7e6/install-enqueue-dangling-dep
branch
from
9f36465 to
d415c6e
Compare
|
Dropped the merge of main (9f364653 → back to d415c6e). The merge pulled in At d415c6e (build 46478) all Linux test jobs including
My test ( |
There was a problem hiding this comment.
LGTM — all previously flagged issues have been addressed.
Extended reasoning...
Overview
This PR fixes a use-after-poison in the runtime auto-install path. Three changes: (1) now copies the to the stack before passing its address; (2) the branch uses the parameter instead of re-dereferencing after may have reallocated the buffer; (3) both the and loops in now iterate by index against a snapshot length and copy each entry to the stack — matching the safe pattern already used by the add/update loop below them.
Security risks
No new security surface introduced. This is a memory-safety fix that eliminates a use-after-free (use-after-poison in ASAN builds) reachable via the runtime require() auto-install path. The fix reduces attack surface rather than expanding it.
Level of scrutiny
The changes are small and mechanical: copy-to-stack before taking address, and use a value parameter instead of re-dereferencing a potentially-stale pointer. The pattern is already established throughout the codebase. The regression test exercises the specific code path (warm manifest + missing tarball → reallocation) that triggered the original crash.
Other factors
All three rounds of review feedback have been addressed: the overrides/catalogs loop UAF was fixed, the test now asserts both manifests and extracted tarballs exist before pruning, and the try/catch is intentionally kept since a native SIGABRT cannot be suppressed by JS exception handling. The pre-existing memory leak was acknowledged and reasonably deferred (pre-existing, tiny allocation in a short-lived CLI process).
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Style nit: for (0...dependencies_len) |dep_id| is better
…yToRoot (oven-sh#29483) Fuzzilli found a use-after-poison in the runtime auto-install path. `enqueueDependencyToRoot` passed `&lockfile.buffers.dependencies.items[dep_id]` into `enqueueDependencyWithMainAndSuccessFn`. When the manifest for the requested package is already cached (on disk or in memory) but the extracted tarball is not, control reaches `getOrPutResolvedPackageWithFindResult`, which calls `Lockfile.Package.fromNPM`. That grows `buffers.dependencies` via `ensureUnusedCapacity` to make room for the package's own dependencies, reallocating the backing storage. The subsequent `.extract` branch then read `dependency.behavior.isRequired()` from the freed buffer. ``` #0 getOrPutResolvedPackageWithFindResult PackageManagerEnqueue.zig:1520 dependency.behavior.isRequired() #1 getOrPutResolvedPackage PackageManagerEnqueue.zig:1778 #2 enqueueDependencyWithMainAndSuccessFn PackageManagerEnqueue.zig:523 #3 enqueueDependencyToRoot PackageManagerEnqueue.zig:321 #4 Resolver.enqueueDependencyToResolve resolver.zig:2356 ... oven-sh#14 Bun__resolveSync oven-sh#15 functionImportMeta__resolveSyncPrivate (runtime require() path) ``` Two changes: - `enqueueDependencyToRoot` now copies the `Dependency` to the stack before taking its address, matching every other caller of `enqueueDependencyWithMainAndSuccessFn` (`processDependencyListItem`, `processPeerDependencyList`, etc.). - The one read that ran after `fromNPM` now uses the `behavior` parameter that was already passed by value, instead of re-dereferencing `dependency`. Repro (debug/ASAN only): auto-install a package with a warm on-disk manifest but no extracted tarball — `fromNPM` appending even a single dependency forces a realloc of the one-entry buffer. The new test warms the cache, removes the extracted tarballs, and runs `require()` via `-e` so it goes through `Bun__resolveSync` → `enqueueDependencyToRoot`.
…yToRoot (oven-sh#29483) Fuzzilli found a use-after-poison in the runtime auto-install path. `enqueueDependencyToRoot` passed `&lockfile.buffers.dependencies.items[dep_id]` into `enqueueDependencyWithMainAndSuccessFn`. When the manifest for the requested package is already cached (on disk or in memory) but the extracted tarball is not, control reaches `getOrPutResolvedPackageWithFindResult`, which calls `Lockfile.Package.fromNPM`. That grows `buffers.dependencies` via `ensureUnusedCapacity` to make room for the package's own dependencies, reallocating the backing storage. The subsequent `.extract` branch then read `dependency.behavior.isRequired()` from the freed buffer. ``` #0 getOrPutResolvedPackageWithFindResult PackageManagerEnqueue.zig:1520 dependency.behavior.isRequired() oven-sh#1 getOrPutResolvedPackage PackageManagerEnqueue.zig:1778 oven-sh#2 enqueueDependencyWithMainAndSuccessFn PackageManagerEnqueue.zig:523 oven-sh#3 enqueueDependencyToRoot PackageManagerEnqueue.zig:321 oven-sh#4 Resolver.enqueueDependencyToResolve resolver.zig:2356 ... oven-sh#14 Bun__resolveSync oven-sh#15 functionImportMeta__resolveSyncPrivate (runtime require() path) ``` Two changes: - `enqueueDependencyToRoot` now copies the `Dependency` to the stack before taking its address, matching every other caller of `enqueueDependencyWithMainAndSuccessFn` (`processDependencyListItem`, `processPeerDependencyList`, etc.). - The one read that ran after `fromNPM` now uses the `behavior` parameter that was already passed by value, instead of re-dereferencing `dependency`. Repro (debug/ASAN only): auto-install a package with a warm on-disk manifest but no extracted tarball — `fromNPM` appending even a single dependency forces a realloc of the one-entry buffer. The new test warms the cache, removes the extracted tarballs, and runs `require()` via `-e` so it goes through `Bun__resolveSync` → `enqueueDependencyToRoot`.
Detects `const X = &list.items[i]; list.append(...); use(X.*)` — taking a pointer to a specific element in an ArrayList's backing storage, then growing the list (which may reallocate), then using the now-dangling pointer. Companion to `arraylist-items-slice` (full-slice borrow) and `hashmap-getptr-rehash`. Evidence: oven-sh/bun#29483, where `&lockfile.buffers.dependencies.items[dep_id]` was passed to a function that internally called `fromNPM` → `ensureUnusedCapacity`, reallocating the buffer (ASAN: use-after-poison). Includes 5 unit tests covering the TP shape, pre-grow use (safe), different-receiver (safe), insert variant, and AssumeCapacity (safe).
2 participants
Fuzzilli found a use-after-poison in the runtime auto-install path.
enqueueDependencyToRootpassed&lockfile.buffers.dependencies.items[dep_id]intoenqueueDependencyWithMainAndSuccessFn. When the manifest for the requested package is already cached (on disk or in memory) but the extracted tarball is not, control reachesgetOrPutResolvedPackageWithFindResult, which callsLockfile.Package.fromNPM. That growsbuffers.dependenciesviaensureUnusedCapacityto make room for the package's own dependencies, reallocating the backing storage. The subsequent.extractbranch then readdependency.behavior.isRequired()from the freed buffer.Two changes:
enqueueDependencyToRootnow copies theDependencyto the stack before taking its address, matching every other caller ofenqueueDependencyWithMainAndSuccessFn(processDependencyListItem,processPeerDependencyList, etc.).fromNPMnow uses thebehaviorparameter that was already passed by value, instead of re-dereferencingdependency.Repro (debug/ASAN only): auto-install a package with a warm on-disk manifest but no extracted tarball —
fromNPMappending even a single dependency forces a realloc of the one-entry buffer. The new test warms the cache, removes the extracted tarballs, and runsrequire()via-eso it goes throughBun__resolveSync→enqueueDependencyToRoot.