Fix dangling log pointer in PackageManager after resolve#28500
Conversation
resolveMaybeNeedsTrailingSlash temporarily swaps resolver.log to a stack-local Log for the duration of the resolve. If getPackageManager() is called during that resolve (initializing the PM singleton for the first time), the PM permanently captures the stack-local log pointer. After the function returns, PM.log is dangling. Later calls to PM.runTasks that use manager.log then read from freed stack memory, causing a stack-buffer-overflow detected by ASAN. Fix: include the package manager in the defer that restores log pointers.
|
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)
WalkthroughFixed a memory safety issue in VirtualMachine.zig where the package manager's log pointer retained a stack-local reference after frame exit. Added corresponding regression test for mock.module with non-string module specifiers. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, targeted fix that correctly extends the existing log-pointer restoration pattern to cover the PackageManager.
Extended reasoning...
Overview
This PR modifies resolveMaybeNeedsTrailingSlash in src/bun.js/VirtualMachine.zig to add one additional pointer restoration in the existing defer block, and adds a regression test in test/js/bun/test/mock/mock-module-resolve-crash.test.ts.
Security Risks
None. The change only restores a pointer to a pre-existing valid log object (old_log), and only does so conditionally if the PackageManager was initialized. No new attack surface is introduced.
Level of Scrutiny
Low. The fix is a 4-line addition following an already-established pattern (lines 1840–1842 restore jsc_vm.log, linker.log, and resolver.log; the new code does the same for pm.log). The logic is trivially correct: if the PM was initialized during the resolve, its log pointer must be restored to old_log before the stack-local log is destroyed. The conditional null-check on package_manager is safe and avoids touching the PM when it was not initialized during this call.
Other Factors
The regression test spawns a child process that exercises the exact crash path (mock.module with a non-string specifier) and verifies it exits cleanly with exit code 0. The test structure matches existing patterns in the test suite. No CODEOWNER concerns; this is a narrow VM-level bug fix.
There was a problem hiding this comment.
The fix looks correct and follows the established pattern for log pointer restoration, but the CI report shows widespread build-zig failures across multiple platforms that should be investigated before merging.
Extended reasoning...
Overview
This PR adds pm.log = old_log to the existing defer block in resolveMaybeNeedsTrailingSlash (VirtualMachine.zig) that already restores jsc_vm.log, transpiler.linker.log, and transpiler.resolver.log. It also adds a regression test that spawns a subprocess exercising mock.module with a non-string specifier.
Security risks
No security risks introduced. This is a memory safety fix — it prevents use-after-free of a stack-local Log struct that could be captured by the PackageManager singleton if PM is first initialized during a resolve call.
Level of scrutiny
The fix is logically sound and follows an established pattern exactly. The root cause (PM initialized inside the poisoned log-swap scope capturing the stack-local pointer) is well-documented. old_log is the pre-existing VM log pointer, so restoring pm.log = old_log is correct regardless of whether PM was initialized before or during this resolve. However, the robobun CI comment reports build-zig failures across all major platforms (x64, aarch64, musl variants, ASAN, baseline) for the PR's commit. The PR description claims CI is passing, which is contradictory and warrants human verification.
Other factors
No bugs were found by the automated bug hunting system. coderabbitai also raised no actionable concerns. The change is minimal (5 lines of Zig) with a clear, well-reasoned test. The only blocker is resolving whether the CI failures are pre-existing infrastructure noise or were introduced by this change.
## Problem
Fuzzilli hit a flaky SIGSEGV (fingerprint `2519cad1804eace1`) from:
```js
const v13 = Bun.jest().vi;
try { v13.mock("function f2() {\n const v6 = new ArrayBuffer();\n ...\n}"); } catch (e) {}
Bun.gc(true);
```
`JSMock__jsModuleMock` calls `Bun__resolveSyncWithSource` on the
specifier before validating the callback, which sends the garbage string
through the resolver. The resolver's auto-install gate at
`loadNodeModules` only checks `esm_ != null`; `ESModule.Package.parse`
accepts anything that doesn't start with `.` or contain `\` / `%`, so
the whole function source is treated as a package name.
`enqueueDependencyToRoot` then calls `PackageManager.sleepUntil`, which
re-enters `EventLoop.tick()` from inside a call that is itself running
inside an event-loop tick:
```
#0 ConcurrentTask.PackedNextPtr.atomicLoadPtr
#1 UnboundedQueue(ConcurrentTask).popBatch
#3 event_loop.tickConcurrentWithCount
#7 AnyEventLoop.tick
#8 PackageManager.sleepUntil
#9 PackageManager.enqueueDependencyToRoot
#10 Resolver.resolveAndAutoInstall
#16 Bun__resolveSyncWithSource
#17 JSMock__jsModuleMock
```
The same path is reachable from `Bun.resolveSync`, `import()`, and
`require.resolve` with any user-provided string.
## Fix
Gate the auto-install branch on `strings.isNPMPackageName(esm_.?.name)`.
That validator already exists and is used by `bun link`, `bun pm view`,
and the bundler; it rejects newlines, spaces, braces, and anything else
that could never be a registry package. Specifiers failing the check
fall straight through to `.not_found` — the same result the registry
fetch would eventually produce — without initializing the package
manager or ticking the event loop.
This is a resolver-level fix, so it covers every entry point (not just
`mock.module`). It also avoids spurious network requests for garbage
specifiers; on this container a single resolve of a multi-line specifier
dropped from ~275ms to ~16ms.
## Tests
- `test/js/bun/resolve/resolve-autoinstall-invalid-name.test.ts` stands
up a local registry and verifies zero manifest requests for a set of
invalid names with `--install=force`, plus a positive control that a
valid name still hits the registry.
- `test/js/bun/test/mock/mock-module-non-string.test.ts` gains a case
for `mock.module` with newline / whitespace / bracket specifiers (with
and without a callback).
- Existing `test/cli/run/run-autoinstall.test.ts` (11 tests) and
`test/js/bun/test/mock/mock-module.test.ts` all pass.
Related: #28945, #28956, #28500, #28511.
Fingerprint: `2519cad1804eace1`
|
Superseded by #28511 which landed the same fix. |
) ## Problem Fuzzilli hit a flaky SIGSEGV (fingerprint `2519cad1804eace1`) from: ```js const v13 = Bun.jest().vi; try { v13.mock("function f2() {\n const v6 = new ArrayBuffer();\n ...\n}"); } catch (e) {} Bun.gc(true); ``` `JSMock__jsModuleMock` calls `Bun__resolveSyncWithSource` on the specifier before validating the callback, which sends the garbage string through the resolver. The resolver's auto-install gate at `loadNodeModules` only checks `esm_ != null`; `ESModule.Package.parse` accepts anything that doesn't start with `.` or contain `\` / `%`, so the whole function source is treated as a package name. `enqueueDependencyToRoot` then calls `PackageManager.sleepUntil`, which re-enters `EventLoop.tick()` from inside a call that is itself running inside an event-loop tick: ``` #0 ConcurrentTask.PackedNextPtr.atomicLoadPtr #1 UnboundedQueue(ConcurrentTask).popBatch #3 event_loop.tickConcurrentWithCount oven-sh#7 AnyEventLoop.tick oven-sh#8 PackageManager.sleepUntil oven-sh#9 PackageManager.enqueueDependencyToRoot oven-sh#10 Resolver.resolveAndAutoInstall oven-sh#16 Bun__resolveSyncWithSource oven-sh#17 JSMock__jsModuleMock ``` The same path is reachable from `Bun.resolveSync`, `import()`, and `require.resolve` with any user-provided string. ## Fix Gate the auto-install branch on `strings.isNPMPackageName(esm_.?.name)`. That validator already exists and is used by `bun link`, `bun pm view`, and the bundler; it rejects newlines, spaces, braces, and anything else that could never be a registry package. Specifiers failing the check fall straight through to `.not_found` — the same result the registry fetch would eventually produce — without initializing the package manager or ticking the event loop. This is a resolver-level fix, so it covers every entry point (not just `mock.module`). It also avoids spurious network requests for garbage specifiers; on this container a single resolve of a multi-line specifier dropped from ~275ms to ~16ms. ## Tests - `test/js/bun/resolve/resolve-autoinstall-invalid-name.test.ts` stands up a local registry and verifies zero manifest requests for a set of invalid names with `--install=force`, plus a positive control that a valid name still hits the registry. - `test/js/bun/test/mock/mock-module-non-string.test.ts` gains a case for `mock.module` with newline / whitespace / bracket specifiers (with and without a callback). - Existing `test/cli/run/run-autoinstall.test.ts` (11 tests) and `test/js/bun/test/mock/mock-module.test.ts` all pass. Related: oven-sh#28945, oven-sh#28956, oven-sh#28500, oven-sh#28511. Fingerprint: `2519cad1804eace1`
) ## Problem Fuzzilli hit a flaky SIGSEGV (fingerprint `2519cad1804eace1`) from: ```js const v13 = Bun.jest().vi; try { v13.mock("function f2() {\n const v6 = new ArrayBuffer();\n ...\n}"); } catch (e) {} Bun.gc(true); ``` `JSMock__jsModuleMock` calls `Bun__resolveSyncWithSource` on the specifier before validating the callback, which sends the garbage string through the resolver. The resolver's auto-install gate at `loadNodeModules` only checks `esm_ != null`; `ESModule.Package.parse` accepts anything that doesn't start with `.` or contain `\` / `%`, so the whole function source is treated as a package name. `enqueueDependencyToRoot` then calls `PackageManager.sleepUntil`, which re-enters `EventLoop.tick()` from inside a call that is itself running inside an event-loop tick: ``` #0 ConcurrentTask.PackedNextPtr.atomicLoadPtr oven-sh#1 UnboundedQueue(ConcurrentTask).popBatch oven-sh#3 event_loop.tickConcurrentWithCount oven-sh#7 AnyEventLoop.tick oven-sh#8 PackageManager.sleepUntil oven-sh#9 PackageManager.enqueueDependencyToRoot oven-sh#10 Resolver.resolveAndAutoInstall oven-sh#16 Bun__resolveSyncWithSource oven-sh#17 JSMock__jsModuleMock ``` The same path is reachable from `Bun.resolveSync`, `import()`, and `require.resolve` with any user-provided string. ## Fix Gate the auto-install branch on `strings.isNPMPackageName(esm_.?.name)`. That validator already exists and is used by `bun link`, `bun pm view`, and the bundler; it rejects newlines, spaces, braces, and anything else that could never be a registry package. Specifiers failing the check fall straight through to `.not_found` — the same result the registry fetch would eventually produce — without initializing the package manager or ticking the event loop. This is a resolver-level fix, so it covers every entry point (not just `mock.module`). It also avoids spurious network requests for garbage specifiers; on this container a single resolve of a multi-line specifier dropped from ~275ms to ~16ms. ## Tests - `test/js/bun/resolve/resolve-autoinstall-invalid-name.test.ts` stands up a local registry and verifies zero manifest requests for a set of invalid names with `--install=force`, plus a positive control that a valid name still hits the registry. - `test/js/bun/test/mock/mock-module-non-string.test.ts` gains a case for `mock.module` with newline / whitespace / bracket specifiers (with and without a callback). - Existing `test/cli/run/run-autoinstall.test.ts` (11 tests) and `test/js/bun/test/mock/mock-module.test.ts` all pass. Related: oven-sh#28945, oven-sh#28956, oven-sh#28500, oven-sh#28511. Fingerprint: `2519cad1804eace1`
1 participant
Root Cause
resolveMaybeNeedsTrailingSlashtemporarily swapsresolver.logto a stack-localLogfor the duration of module resolution. IfgetPackageManager()is called during that resolve — initializing the PackageManager singleton for the first time viabun.once— the PM permanently captures the stack-local log pointer in its.logfield.After
resolveMaybeNeedsTrailingSlashreturns, the stack-localLogis destroyed butPackageManager.logstill points to it. Later calls toPM.runTasksthat usemanager.log.addErrorFmt(...)then read from freed stack memory, causing a stack-buffer-overflow.The crash was triggered by
mock.module(Intl.Segmenter, ...)which stringifies the constructor to"function Segmenter() { [native code] }"and passes it through the resolver, triggering auto-install (and thus PM initialization) inside the poisoned scope.Fix
Include the package manager's
.login thedeferblock that restores log pointers after the resolve completes, matching the existing pattern forjsc_vm.log,transpiler.resolver.log, andtranspiler.linker.log.Fingerprint:
Address:stack-buffer-overflow:bun-debug+0x900741eVerification: Fix traced through resolver.zig (getPackageManager at line 532 captures this.log via initWithRuntime) confirming the dangling pointer. Only one log-swap site exists in VirtualMachine.zig. Test exercises the exact PM-init-during-resolve path via mock.module(Intl.Segmenter). CI: Lint JS pass, Buildkite pipeline pass, build in progress with no failures. No TODO/FIXME/HACK in diff. No unrelated changes.