Fix dangling PackageManager.log when JS runs during auto-install sleep_until - #41149
Conversation
…p_until enqueue_dependency_to_root blocks in sleep_until, which ticks the JS event loop between run_tasks polls. Module transpilation that runs in that tick saves transpiler.log and restores pm.log to it on exit. The resolve path did not swap transpiler.log, so this restore pointed pm.log away from the resolve's scoped log. The next run_tasks poll then wrote to the wrong Log, and with further interleaving to a dead stack Log. Swap and restore transpiler.log with the other log pointers in resolve_maybe_needs_trailing_slash. Snapshot pm.log on entry to sleep_until and re-assert it before each run_tasks poll. The regression test queues require() calls with setImmediate so they run during the tick. Without the fix the 404 diagnostics land in the VM log and print to stderr at exit.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe change preserves valid log pointers during module resolution and auto-install event-loop polling. A regression test covers interleaved module loads, missing-package resolution, clean stderr, successful execution, and registry requests. ChangesLog lifetime protection
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized change preserves the caller’s log during auto-install event-loop processing and adds a regression test for the dangling-log failure; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the failure, root cause, fix, regression test, and verification results. It provides the information required by the repository template, although it uses equivalent headings instead of the exact template headings. Comment |
![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's a memory-safety fix for a dangling-pointer bug driven by subtle event-loop reentrancy across three log-swap sites, a human look would still be worthwhile — particularly on whether the is_done re-assert plus the single-site transpiler.log swap is the right shape versus aligning the save/restore sources in transpile_source_code_inner and AsyncModule::resume_loading_module as well.
What was reviewed:
RestoreLogguard:old_transpiler_logis saved and restored independently (not forced toold_log), so it preserves whatever the caller had — no new assumption thattranspiler.log == vm.logon entry.Closure.logsnapshot: taken fromthis.logbeforesleep_until, re-asserted before eachrun_tasks; the raw*mutmatches the existingmanagerfield pattern and the SAFETY comment's aliasing story.- Test: local
port: 0registry,tempDir,bunEnvspread, concurrent pipe drain, stderr/stdout asserted before exit code;stderr === ""checks the observable log-desync symptom (404 diagnostics leaking to the VM log), not a generic crash string, andn > 0/hits > 0confirm the interleaving actually happened.
Extended reasoning...
Overview
The PR fixes an ASAN-reported stack-buffer-overflow where PackageManager.log could point at a dead stack Log after JS runs during sleep_until's event-loop tick inside auto-install resolution. Two source changes: src/jsc/VirtualMachine.rs now swaps/restores jsc_vm.transpiler.log alongside the other four log pointers in the RestoreLog drop guard, and src/install/PackageManager/PackageManagerEnqueue.rs snapshots this.log into the sleep_until closure and re-asserts manager.log = self.log at the top of every is_done() poll. A new test in the existing resolve-autoinstall-log-dangling.test.ts reproduces the interleaving with setImmediate-queued require() calls during a _resolveFilename auto-install against a local 404 registry.
Security risks
None identified. This is an internal memory-safety fix in the package-manager/resolver log-pointer plumbing; no new user-facing surface, no untrusted-input parsing, no auth/crypto/permissions paths touched. The test uses a local Bun.serve({ port: 0 }) — no external network contact.
Level of scrutiny
High. Per REVIEW.md, native memory safety is the most-blocked review category, and this change sits squarely in it: raw *mut pointers, a hand-rolled drop guard, and a fix whose correctness depends on understanding the exact save/restore behavior of three separate log-swap sites (resolve_maybe_needs_trailing_slash, transpile_source_code_inner, AsyncModule::resume_loading_module) under reentrancy. The PR description names all three but patches only one, relying on the is_done re-assert as defense-in-depth for the others. That's a defensible design — it makes run_tasks immune to whatever the tick did — but "fix the whole class in the same PR" invites a human to confirm that leaving the other two sites' asymmetric save/restore in place doesn't leave a sibling bug reachable from a different entry point that doesn't route through this closure.
Other factors
The change is small (~15 source lines), follows the file's existing raw-pointer + drop-guard patterns exactly, and the added old_transpiler_log field restores the saved value rather than forcing it to old_log — a conservative choice that avoids introducing a new invariant. The test follows harness conventions correctly (concurrent pipe drain, stderr before exit code, no sleeps, appended to the existing file for this module) and asserts the observable symptom the PR description says distinguishes fixed from unfixed builds. No CODEOWNERS entries cover the changed paths, and the timeline shows no outstanding third-party objections. The bug hunt ran to dry_streak with no findings. This supersedes a previously stale-closed PR, so prior context exists that a human reviewer may want to compare.
|
A note on scope, for whoever reviews the shape of this change. I kept the swap change to
The |
On a JS thread, PackageManager::sleep_until now calls park_until: it registers in wake_waiters, then waits on wake_count with a futex until is_done is true. wake_raw bumps wake_count and calls Futex::wake only when a thread is parked, so a `bun install` task pays no syscall. Both sides use SeqCst: the waiter registers before it reads the count, the waker bumps the count before it reads the waiters, so no wake is lost. Since #40734, git for a git dependency is a child process on the install thread's event loop. The parked thread cannot reap it, so the wait would never end. The resolver cannot load a git resolution in any case (path_for_resolution serves npm only). The auto-install manager now leaves a git dependency unresolved and does not start git. enqueue_git_task asserts this in debug builds. Remove the PackageManager.log snapshot from #41149: no JS runs inside the wait now, so nothing can swap the log pointer there. Its test now asserts that the queued module loads run after the resolve loop. Tests: one case per entry point (require, require.resolve, Bun.resolveSync, import.meta.resolve, import.meta.resolveSync, import(), Bun.resolve). A Bun.serve handler makes the call while the test holds the manifest request until a second request is written. Two git cases (a dependency of the project, a dependency of an auto-installed package). A module graph that imports a builtin and then an auto-installed package.
2 participants
Fuzzilli found an ASAN stack-buffer-overflow in
run_tasks->Log::add_msg->Vec::pushduring runtime auto-install.What happened
enqueue_dependency_to_rootblocks insleep_until.sleep_untilticks the JS event loop betweenis_donepolls. Each poll callsrun_tasks, which readsmanager.logto log manifest 4xx errors.The event loop tick can run module transpilation or
AsyncModule::resume_loading_module. Both swap the VM's log pointers (jsc_vm.log,transpiler.log,resolver.log,linker.log,pm.log) and restore them on exit. They do not save from the same source:resolve_maybe_needs_trailing_slashsavesjsc_vm.logand swaps every pointer excepttranspiler.log.transpile_source_code_innersavestranspiler.logand restorespm.logto it.AsyncModule::resume_loading_modulesavesjsc_vm.logbut restorestranspiler.logto it.When a module transpile runs during a resolve's
sleep_untiltick, its restore setspm.logto the oldtranspiler.log, not the resolve's scoped log. The 404 diagnostics fromrun_tasksthen land in the wrong log. With more interleaving across callspm.logends up at a dead stackLog, and the nextrun_taskspoll reads it.Fix
transpiler.logwith the other log pointers inresolve_maybe_needs_trailing_slash. It can no longer drift fromjsc_vm.logacross a resolve.pm.logon entry toenqueue_dependency_to_root'ssleep_until. Re-assert it before eachrun_taskspoll.run_tasksalways sees the caller's log, whatever the event loop tick did to it.Test
The test runs a 404 registry in the parent process on
port: 0. The child queuesrequire()calls withsetImmediate, so they run duringsleep_until's event-loop tick and triggertranspile_source_code_inner's log swap. Without the fix, the 404 errors land in the VM log and print to stderr at exit. With the fix, stderr is empty.Verified on current
main(6f27257): the test fails without the source change and passes with it.Supersedes #31120, which the stale bot closed after 90 days. Same change, rebased onto current main; moved to this branch so the fix can be tracked.
[human-review] gate passed · iteration 6 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 6
evidence per changed file