fix: sync PackageManager.log with resolver.log in resolveMaybeNeedsTrailingSlash#28310
fix: sync PackageManager.log with resolver.log in resolveMaybeNeedsTrailingSlash#28310robobun wants to merge 10 commits into
Conversation
|
Updated 7:33 PM PT - Mar 25th, 2026
❌ @robobun, your commit a35c071 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 28310That installs a local version of the PR into your bun-28310 --bun |
|
Verification: CI pending (build #40234). Code fix confirmed correct: mirrors the identical pm.log sync/restore pattern already present in |
|
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:
WalkthroughAdds propagation and deferred restoration of a package manager log pointer used during resolution, and introduces a regression test that spawns a Bun subprocess which requires a nonexistent package and asserts the subprocess exits without crashing. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/js/bun/resolve/resolve-require-nonexistent-no-crash.test.ts`:
- Line 10: Replace the hardcoded "/tmp/bun-test-resolve-crash" used for
BUN_INSTALL_GLOBAL_DIR in the env object of the test with an isolated temp
directory created via harness.tempDir(); locate the env spread in
resolve-require-nonexistent-no-crash.test.ts where BUN_INSTALL_GLOBAL_DIR is set
and call harness.tempDir() to generate a unique directory for each run, then
assign that value to BUN_INSTALL_GLOBAL_DIR so tests do not share state or leak
across parallel runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9790e155-c0ad-4c1b-8518-398f289aaef4
📒 Files selected for processing (2)
src/bun.js/VirtualMachine.zigtest/js/bun/resolve/resolve-require-nonexistent-no-crash.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/js/bun/resolve/resolve-require-nonexistent-no-crash.test.ts`:
- Line 15: The test is capturing stdout into the unused variable `stdout` from
the Promise.all call with `proc.stdout.text()`; either remove `stdout` from the
destructuring and drop `proc.stdout.text()` from the Promise.all array, or keep
it and add an assertion against `stdout` (for example assert it is empty) so the
variable is actually used; update the destructuring that assigns `[stdout,
stderr, exitCode]` and the Promise.all arguments accordingly to match the chosen
approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e70f8376-60b1-4c4e-9db5-9e4b8f8da7ae
📒 Files selected for processing (1)
test/js/bun/resolve/resolve-require-nonexistent-no-crash.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/js/bun/resolve/resolve-require-nonexistent-no-crash.test.ts`:
- Around line 17-20: Remove the forbidden panic-string assertion (the
expect(stderr).not.toContain("AddressSanitizer") line) and replace it with a
deterministic assertion about the process outcome using the existing variables:
assert a concrete exit code (e.g., expect(exitCode).toBe(<expectedCode>)) or
assert stderr contains a known, expected error message; keep the existing checks
on exitCode (expect(exitCode).not.toBe(null); expect(exitCode).not.toBe(134);)
or replace them with a single explicit expected-value assertion using exitCode
so the test validates a deterministic outcome rather than the absence of a panic
string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fefa6b9-0f8c-4359-b938-78950d6cb05f
📒 Files selected for processing (1)
test/js/bun/resolve/resolve-require-nonexistent-no-crash.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM - the pm.log sync/restore correctly mirrors the existing pattern in ModuleLoader.zig.
Extended reasoning...
Overview
This PR adds 6 lines to resolveMaybeNeedsTrailingSlash in VirtualMachine.zig to sync and restore package_manager.log alongside resolver.log when using a stack-allocated temporary log. It also adds a regression test. The fix exactly mirrors the identical pattern already present in ModuleLoader.zig:188-198 (transpileSourceCode).
Security risks
This is a memory safety fix - it prevents a dangling pointer (use-after-free / stack-buffer-overflow) when PM writes to a stale log pointer during auto-install. The fix reduces security risk; it does not introduce any new attack surface.
Level of scrutiny
Low scrutiny needed. The change is minimal (6 lines of Zig), mechanically copies an established pattern from a sibling function in the same codebase, and the bug report for the pre-existing setLog() issue is informational only and not introduced by this PR.
Other factors
- CI failures are all in unrelated vendor code (libuv, tinycc) and build infrastructure, not caused by this PR.
- The coderabbitai suggestions about test patterns (tempDir, unused stdout) have already been addressed in follow-up commits.
- The one remaining unresolved coderabbitai comment about not.toContain(AddressSanitizer) is a minor nit - the test also asserts exit code != 134, providing a deterministic check.
- The pre-existing bug in Transpiler.setLog() flagged by the bug hunter is valid but not introduced or worsened by this PR.
There was a problem hiding this comment.
LGTM — correctly mirrors the existing pm.log sync pattern from transpileSourceCode.
Extended reasoning...
Overview
This PR adds 6 lines to resolveMaybeNeedsTrailingSlash in VirtualMachine.zig to sync and restore package_manager.log alongside resolver.log when swapping to a stack-allocated log. A regression test spawning a subprocess that requires a nonexistent package is also added.
Security risks
This is a memory safety fix — it prevents a dangling pointer (stack-buffer-overflow) when pm.log references a dead stack frame. No new attack surface is introduced. The fix strictly improves safety.
Level of scrutiny
Low scrutiny needed. The change is a 6-line mechanical addition that exactly mirrors the identical pattern already present in ModuleLoader.zig:188-197 (transpileSourceCode). The test is straightforward and all CodeRabbit suggestions (tempDir, unused stdout, deterministic assertions) have been addressed in follow-up commits.
Other factors
All inline review comments from CodeRabbit are resolved. CI build failures are in unrelated vendor code (libuv, tinycc) and build infrastructure, not in this PR's changes. My previous review flagged a pre-existing instance of the same bug class in Transpiler.setLog() as informational — that is not blocking for this PR.
There was a problem hiding this comment.
LGTM — targeted fix mirrors the existing pm.log sync pattern in ModuleLoader.zig, and the problematic setLog() change was correctly reverted.
Extended reasoning...
Overview
This PR adds 6 lines to resolveMaybeNeedsTrailingSlash in VirtualMachine.zig to sync PackageManager.log with the temporary stack-allocated logger.Log, and restore it in the defer block. It also adds a clean regression test. The Transpiler.setLog() modification was added and then reverted (commit 2853fed), so the final diff only touches VirtualMachine.zig and the new test file.
Security risks
This is a memory safety fix — it prevents a dangling pointer (stack-buffer-overflow) when pm.log references a dead stack frame. The fix eliminates a use-after-free class bug. No new security risks are introduced.
Level of scrutiny
The change is small (6 lines of Zig) and follows an identical established pattern already present in ModuleLoader.zig:186-198 (transpileSourceCode). The pattern is: save old log, set pm.log to temp log, defer restore pm.log to old log. This is mechanical and easy to verify by comparing the two sites.
Other factors
- All reviewer feedback has been addressed: CodeRabbit comments about
tempDir, unusedstdout, and deterministic assertions were fixed in follow-up commits. - My previous concern about
setLog()introducing a new dangling pointer through sharedPackageManagerpointers was addressed by reverting that change (2853fed). - The regression test is appropriate — it spawns a subprocess, requires a nonexistent package, and asserts exit code 0.
- CI had failures on an earlier commit (
5fa3480) which included the since-revertedsetLog()change; the current state should be clean.
There was a problem hiding this comment.
LGTM — the pm.log sync/restore in resolveMaybeNeedsTrailingSlash correctly mirrors the existing pattern in transpileSourceCode, and the setLog() concern from my earlier review was addressed by reverting that change.
Extended reasoning...
Overview
This PR touches two files: src/bun.js/VirtualMachine.zig (6 lines added to sync pm.log alongside resolver.log and linker.log in resolveMaybeNeedsTrailingSlash) and a new regression test test/js/bun/resolve/resolve-require-nonexistent-no-crash.test.ts. The fix addresses a real stack-buffer-overflow crash found by Fuzzilli where PackageManager.log pointed at a stale stack-allocated log during auto-install.
Security risks
The fix is a memory safety improvement — it prevents a dangling pointer that could cause a stack-buffer-overflow. No new security risks are introduced. The change is purely additive (syncing an additional pointer alongside existing ones) and follows an established safe pattern with proper save/restore in a defer block.
Level of scrutiny
This is a small, mechanical change that mirrors an identical pattern already present in transpileSourceCode in the same codebase. The fix is self-evidently correct: when temporarily redirecting log pointers, all aliases must be updated and restored. The previous iteration included a setLog() change that I flagged as introducing a new dangling pointer via transpiler copies — that was correctly reverted (commit 2853fed), leaving only the safe direct mutation on the real jsc_vm.transpiler.
Other factors
- All three CodeRabbit review comments (tempDir usage, unused stdout, panic-string assertion) were addressed in follow-up commits.
- My own previous bug report about
setLog()was addressed by reverting the transpiler.zig change. - The test follows project conventions: uses
tempDirfrom harness, assertsexitCode === 0, and avoids forbidden negative assertion patterns. - CI has been run (build #40298).
- The
claudelabel indicates this is an AI-generated PR, but the fix is straightforward and correct.
There was a problem hiding this comment.
LGTM — correct fix mirroring the established pm.log sync pattern in transpileSourceCode, and the setLog() concern from my earlier review has been addressed.
Extended reasoning...
Overview
This PR fixes a stack-buffer-overflow crash by syncing PackageManager.log with resolver.log in resolveMaybeNeedsTrailingSlash (VirtualMachine.zig). The fix adds 6 lines: 3 to set pm.log to the stack-local log and 3 to restore it in the defer block. A regression test spawns a subprocess that require()s a nonexistent package and asserts exit code 0.
Security risks
This PR fixes a memory safety bug (use-after-free / stack-buffer-overflow via dangling pm.log pointer). No new security risks are introduced. The pattern exactly mirrors the existing safe implementation in ModuleLoader.zig:188-198.
Level of scrutiny
The VirtualMachine.zig change is small, mechanical, and follows an established pattern already present in the codebase. The setLog() change I previously flagged as introducing a new dangling pointer bug has been reverted — transpiler.zig:setLog() no longer touches pm.log, so the fix is correctly scoped to only the resolveMaybeNeedsTrailingSlash callsite which properly saves and restores pm.log in a defer block. All prior reviewer feedback (CodeRabbit and my own) has been addressed.
Other factors
The test is clean: uses tempDir from harness, asserts deterministic exit code 0, and uses await using for proper cleanup. The only remaining nit (stderr piped but not consumed) is inconsequential for this test. CI build failures visible in the timeline are from unrelated infrastructure issues (libuv clang warning, musl build failures on a different commit).
There was a problem hiding this comment.
LGTM — targeted fix correctly mirrors the existing pm.log sync pattern in transpileSourceCode.
Extended reasoning...
Overview
This PR fixes a stack-buffer-overflow crash in resolveMaybeNeedsTrailingSlash (VirtualMachine.zig) by syncing PackageManager.log alongside resolver.log when swapping to a stack-allocated log, and restoring it in the defer block. A regression test spawning a subprocess that require()s a nonexistent package is also added.
Security risks
No security risks. The change is a memory safety fix — it prevents a dangling pointer that could cause a stack-buffer-overflow when pm.log.addErrorFmt() writes to a stale stack frame during auto-install error handling.
Level of scrutiny
The Zig change is 6 lines of straightforward pointer management that exactly mirrors the identical pattern already present in ModuleLoader.zig:186-198 (transpileSourceCode). The earlier iteration of this PR included a risky change to Transpiler.setLog() that would have introduced the same class of dangling pointer bug via shared pointer mutation — that change has been correctly removed, leaving only the targeted VirtualMachine.zig fix.
Other factors
All prior review feedback (from myself and CodeRabbit) has been addressed: the transpiler.zig change was reverted, the test now uses tempDir from harness, stderr: "inherit" instead of unused piped stderr, and deterministic expect(exitCode).toBe(0) assertions. The test file follows project conventions. CI failure shown in the timeline (v8-heap-snapshot.test.ts SIGKILL) is unrelated to this PR.
…ailingSlash resolveMaybeNeedsTrailingSlash temporarily sets resolver.log to a stack-allocated Log but did not update PackageManager.log. When auto-install triggers network activity, sleepUntil spins the JS event loop. If runTasks encounters an HTTP error, it calls manager.log.addErrorFmt on the stale pointer, causing a stack-buffer-overflow. Mirror the pattern already used in transpileSourceCode: update pm.log alongside resolver.log and restore it in the defer block.
Same bug class as the main fix — setLog() updates resolver.log but not package_manager.log, leaving pm.log as a potential dangling pointer for callers like web_worker.zig that swap to a stack-local temp log.
setLog() is called on transpiler copies (RuntimeTranspilerStore, JSTranspiler TransformTask, ThreadPool) where package_manager is a shared pointer. Writing pm.log through a copy corrupts the shared PM's log pointer with a stack-local address that becomes dangling. The VirtualMachine.zig fix already handles pm.log sync directly with proper save/restore on the original transpiler.
robobun
force-pushed
the
farm/fef8b0f1/fix-pm-log-stack-overflow
branch
from
ed2cd9b to
a35c071
Compare
|
Build #42043 running. Previous builds (#40280, #40299, #40311) had all individual checks pass — the aggregate "failed" status was from internal buildkite infrastructure jobs not related to this PR's changes (only |
There was a problem hiding this comment.
LGTM — the test file is clean, all previous review concerns have been resolved, and the core fixes are already on master.
Extended reasoning...
Overview
The only remaining diff is a new regression test file (test/js/bun/resolve/resolve-require-nonexistent-no-crash.test.ts). The actual bug fixes in VirtualMachine.zig and transpiler.zig are already on master (commits 1fd9ae4, 378d660).
Security risks
None. This is a pure test file addition with no production code changes.
Level of scrutiny
Low. The test file is self-contained, adds no dependencies, and follows established patterns. It correctly uses tempDir from harness, stderr: "inherit", and a deterministic expect(exitCode).toBe(0) assertion.
Other factors
All CodeRabbit and prior reviewer concerns were addressed:
- Hardcoded
/tmppath replaced withtempDir() - Unused
stdoutvariable removed - Forbidden negative-assertion pattern replaced with
toBe(0) - Uncollected stderr pipe replaced with
stderr: "inherit"
My earlier concern aboutsetLog()introducing a new dangling pointer was addressed in commits on master.
1 participant
Fixes a flaky stack-buffer-overflow crash (fingerprint
0182409cb972ff92) found by Fuzzilli.Root Cause
resolveMaybeNeedsTrailingSlashinVirtualMachine.zigtemporarily pointsresolver.logat a stack-allocatedlogger.Log, but does not updatePackageManager.logto match. The companion functiontranspileSourceCodealready does this correctly.When
require()triggers auto-install and the PackageManager'ssleepUntilspins the JS event loop viaevent_loop.tick(), any HTTP error duringrunTaskscallsmanager.log.addErrorFmt(). Sincepm.logwas never updated, it still points at a previous (possibly dead) stack frame, causing a stack-buffer-overflow when writinglog.errors += 1.Fix
Update
pm.logalongsideresolver.loginresolveMaybeNeedsTrailingSlash, and restore it in the defer block — matching the existing pattern intranspileSourceCode.Repro
Crashes intermittently under ASAN when auto-install attempts a network request that fails.