resolver: restore package_manager.log after resolve to avoid dangling stack pointer#31020
Conversation
… stack pointer resolveMaybeNeedsTrailingSlash swaps the resolver log to a stack-local Log while _resolve runs. If the auto-install path lazily initializes the PackageManager during that call, pm.log is seeded from resolver.log and ends up pointing at the stack Log. The Zig original swaps and restores pm.log (and linker.log) alongside the other log aliases; the Rust port had a TODO and only restored vm.log / resolver.log, so pm.log was left pointing into a dead stack frame. A subsequent resolve at a different stack depth that hits the auto-install task runner dereferences that stale pointer in Log::add_error_fmt. Swap/restore linker.log and (if present) package_manager.log in both copies of the resolve log guard, matching the Zig spec. The restore re-checks resolver.package_manager at drop time so a PM created during _resolve is covered too. Also add the missing <cassert> include in wtf-bindings.cpp now that it is no longer pulled in transitively.
|
Updated 10:30 PM PT - May 18th, 2026
✅ @robobun, your commit 4583009d060254ca3b4fc8eb10af238fe1984320 passed in 🧪 To try this PR locally: bunx bun-pr 31020That installs a local version of the PR into your bun-31020 --bun |
|
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 (2)
WalkthroughSynchronizes package_manager.log with temporary stack-log swaps/restores in VM and resolve hook paths, adds ChangesDangling Log Pointer Fix
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Both follow-ups from my earlier review are addressed in 1d93629 (third log-swap site in transpile_source_code now matches, and the test uses --install=fallback). No further issues found, but this is unsafe raw-pointer plumbing in the core resolver/VM path that leans on the "sole dyn AutoInstaller impl" invariant for the .cast::<PackageManager>() downcast — worth a human pair of eyes before merge.
Extended reasoning...
Overview
Fixes a stack-use-after-scope in the Rust port of the runtime resolver: resolveMaybeNeedsTrailingSlash / resolve_hook / transpile_source_code swap several log pointers to a stack-local Log for the duration of _resolve, but the restore guards weren't touching transpiler.linker.log or resolver.package_manager.log. When auto-install lazily created the PackageManager mid-resolve, pm.log was seeded from the stack-local Log and never restored, leaving a dangling pointer that the next auto-install task dereferenced. The fix swaps/restores linker.log and pm.log at all three sites to match the Zig original, adds a regression test, and includes a trivial <cassert> header fix.
Security risks
This is memory-safety code: three new unsafe blocks doing pm.cast::<bun_install::PackageManager>().as_ptr() to downcast a NonNull<dyn AutoInstaller> and write through it. The cast's soundness rests on the comment-documented invariant that PackageManager is the sole AutoInstaller impl. If that invariant is correct (and it appears to be — the Zig source does the equivalent unconditionally), the change strictly removes a UAF; but the downcast pattern itself is the kind of thing a maintainer should sign off on. No injection/auth/data-exposure surface.
Level of scrutiny
High. This is hot-path runtime resolver code in VirtualMachine.rs and jsc_hooks.rs, with hand-rolled drop guards manipulating raw pointers. The fix is conceptually simple (mirror the Zig swap/restore) and well-explained, but "simple unsafe pointer fix in the module loader" is exactly the category that benefits from a human reviewer confirming the NonNull::cast is layout-valid and that re-reading resolver.package_manager at drop time can't race or alias anything else.
Other factors
- My two earlier inline comments (third unfixed copy in
transpile_source_code; deadBUN_CONFIG_INSTALLenv var in the test) were both addressed in commit 1d93629, and the current diff reflects those fixes. - A regression test is included that exercises the auto-install path at varying stack depths against an unroutable registry; it now uses
--install=fallbackso it won't silently go inert if theautodefault changes. - The bug-hunting system found nothing on this revision.
- github-actions flagged two possibly-duplicate PRs (#28310, #26031) tackling the same root cause — a maintainer should confirm this is the variant they want to land.
There was a problem hiding this comment.
Both earlier comments are addressed in 1d93629 and I have nothing further — deferring final sign-off since this is unsafe raw-pointer plumbing in the VM resolver path and Build #55953 is still showing red.
Extended reasoning...
Overview
The PR completes the Zig→Rust port of the log-swap/restore pattern at three sites: VirtualMachine::resolve_maybe_needs_trailing_slash, resolve_hook, and (after my earlier comment) transpile_source_code_inner. Each site now also swaps transpiler.linker.log and resolver.package_manager.log to the stack-local Log and restores them in the drop guard, with the restore re-checking package_manager so a PM lazily created during _resolve is also pointed back at the VM log. There's a one-line <cassert> include fix in wtf-bindings.cpp and a new regression test that hammers failing auto-install resolves at varying stack depths.
Prior feedback
I raised two points on the first revision: (1) the third copy of the swap in transpile_source_code was left behind a stale TODO(blocked_on), and (2) the test relied on a no-op BUN_CONFIG_INSTALL env var. Commit 1d93629 applied the same cast/swap/restore at the third site and switched the test to --install=fallback. Both threads are resolved and the current diff reflects the fixes; the bug-hunting pass on this revision found nothing.
Security risks
None in the conventional sense (no auth/crypto/input-parsing surface). The risk class here is memory safety: the change writes through pm.cast::<bun_install::PackageManager>().as_ptr() inside unsafe blocks, relying on the invariant that the dyn AutoInstaller stored on the resolver is always the concrete PackageManager. I confirmed that's the sole impl AutoInstaller for in the tree (src/install/auto_installer.rs), so the downcast is sound today, and the fix strictly removes a stack-use-after-scope rather than adding new lifetime hazards.
Level of scrutiny
Moderate-to-high. The diff is small and mechanical — it mirrors VirtualMachine.zig / ModuleLoader.zig line-for-line and applies the identical 4-line pattern at each site — but it's unsafe raw-pointer plumbing in the core runtime resolver path where a mistake is UB rather than a thrown error. That, plus the still-red Build #55953 on the latest commit, is enough that I'd rather a maintainer give it the final nod than auto-approve.
Other factors
No CODEOWNERS gate on these paths. The added regression test is well-targeted (varying stack depths + unroutable registry to force manager.log_mut().add_error_fmt). CodeRabbit had no actionable comments.
|
CI: the only failure across both builds (#55953, #55961) is |
What
resolveMaybeNeedsTrailingSlashswapsvm.log/resolver.logto a stack-localLogfor the duration of_resolve, then restores them via a drop guard. The Zig original also swaps and restorestranspiler.linker.logandresolver.package_manager.log; the Rust port had those behind aTODO(b2-cycle)and only handledvm.log+resolver.log.When auto-install is enabled and the resolver lazily creates the
PackageManagerduring_resolve,Resolver::get_package_managerseedspm.logfromresolver.log— which at that point is the stack-localLog. Because the restore guard never touchedpm.log, it was left pointing into a dead stack frame after the function returned. The next resolve at a different stack depth that routes through the auto-install task runner dereferenced that stale pointer inLog::add_error_fmt, tripping ASAN'sstack-use-after-scope(or segfaulting / executing garbage in release builds).Stack at the fault:
Fix
Swap and restore
linker.logand (when present)package_manager.login both copies of the resolve log guard (VirtualMachine::resolve_maybe_needs_trailing_slashandjsc_hooks::resolve_hook), matchingVirtualMachine.zig. The restore re-checksresolver.package_managerat drop time so a PM that was lazily created during_resolveis also pointed back at the VM log.Also adds the missing
<cassert>include inwtf-bindings.cpp, which stopped being pulled in transitively.Repro
Segfaults on
main, clean after this change.Fixes #14432
Fixes #22407