Fix stack-buffer-overflow from stale PackageManager log pointer in resolveMaybeNeedsTrailingSlash#28511
Merged
Merged
Conversation
…solveMaybeNeedsTrailingSlash resolveMaybeNeedsTrailingSlash creates a stack-local logger.Log and sets it on the resolver and linker, but does not update the PackageManager's log pointer. When module resolution triggers auto-install, the package manager uses its stale log field which may point to a destroyed stack frame from a previous call. Apply the same save/restore pattern already used in ModuleLoader.zig (lines 188-198) to also update pm.log.
Collaborator
Author
Contributor
|
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:
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 (1)
WalkthroughModule resolution log redirection now includes the package manager's log during resolveMaybeNeedsTrailingSlash. A new regression test calls 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}
Contributor
There was a problem hiding this comment.
LGTM — straightforward fix that mirrors the existing pattern in ModuleLoader.zig.
Extended reasoning...
Overview
This PR modifies resolveMaybeNeedsTrailingSlash in src/bun.js/VirtualMachine.zig (6 lines) and adds a regression test in test/js/bun/test/mock/mock-module-resolve-log.test.ts (29 lines).
Security risks
None. The change only ensures a pointer is kept in sync, preventing a use-after-stack-frame bug. It does not affect authentication, permissions, or data exposure.
Level of scrutiny
Low. The fix is a direct copy of the save/restore pattern already used in ModuleLoader.zig lines 188-198 for the exact same fields. The diff is self-contained, mechanical, and immediately verifiable against the existing reference implementation.
Other factors
No existing reviewer comments are outstanding. The regression test correctly exercises the crash scenario (mock.module with a non-existent specifier triggering auto-install) and verifies the process exits cleanly. No bugs were flagged by the bug-hunting system.
Contributor
There was a problem hiding this comment.
LGTM — the assertion was added as requested, and the Zig fix correctly mirrors the save/restore pattern used elsewhere.
Extended reasoning...
Overview
Two files changed: src/bun.js/VirtualMachine.zig (4 lines added to save/restore pm.log) and a new regression test test/js/bun/test/mock/mock-module-resolve-log.test.ts.
Security risks
None. This is a memory-safety bug fix that prevents a stack-buffer-overflow; it does not touch auth, crypto, or permissions.
Level of scrutiny
Low-to-medium. The Zig change is surgical: it extends an existing save/restore pattern (old_log / defer) to cover package_manager.log, which was the one pointer not redirected. The pattern is already used in ModuleLoader.zig lines 188–198, so this is a direct port. The conditional if (jsc_vm.transpiler.resolver.package_manager) |pm| correctly handles the case where no package manager is present.
My previous concern about the test lacking assertions was addressed in commit b587312 — expect(mod.a).toBe(1) now verifies observable behavior and will fail on an unfixed binary regardless of ASAN.
Other factors
No bugs were found by the automated bug hunting system. The CI failures visible in the timeline are from an earlier commit (cca5a98) on unrelated tests (24364.test.ts, bun-types.test.ts), not from the files modified in this PR.
This was referenced Apr 7, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Apr 17, 2026
## 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`
structwafel
pushed a commit
to structwafel/bun
that referenced
this pull request
Apr 25, 2026
…solveMaybeNeedsTrailingSlash (oven-sh#28511) ## Problem \`resolveMaybeNeedsTrailingSlash\` in \`VirtualMachine.zig\` creates a stack-local \`logger.Log\` and temporarily sets it on the resolver and linker: \`\`\`zig var log = logger.Log.init(bun.default_allocator); defer log.deinit(); jsc_vm.transpiler.resolver.log = &log; jsc_vm.transpiler.linker.log = &log; \`\`\` However, it does **not** update \`PackageManager.log\`. When module resolution triggers auto-install (e.g. via \`mock.module()\` with a non-existent specifier), the package manager calls \`manager.log.addErrorFmt()\` on its stale \`log\` pointer — which may point to a destroyed stack frame from a previous call to \`resolveMaybeNeedsTrailingSlash\`. This causes a stack-buffer-overflow (ASAN fingerprint: \`Address:stack-buffer-overflow:bun-debug+0xc9f417a\`). ## Fix Apply the same save/restore pattern already used in \`ModuleLoader.zig\` (lines 188-198) to also update and restore \`pm.log\` in the defer block: \`\`\`zig if (jsc_vm.transpiler.resolver.package_manager) |pm| { pm.log = &log; } defer { if (jsc_vm.transpiler.resolver.package_manager) |pm| { pm.log = old_log; } } \`\`\` --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
structwafel
pushed a commit
to structwafel/bun
that referenced
this pull request
Apr 25, 2026
) ## 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`
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
…solveMaybeNeedsTrailingSlash (oven-sh#28511) ## Problem \`resolveMaybeNeedsTrailingSlash\` in \`VirtualMachine.zig\` creates a stack-local \`logger.Log\` and temporarily sets it on the resolver and linker: \`\`\`zig var log = logger.Log.init(bun.default_allocator); defer log.deinit(); jsc_vm.transpiler.resolver.log = &log; jsc_vm.transpiler.linker.log = &log; \`\`\` However, it does **not** update \`PackageManager.log\`. When module resolution triggers auto-install (e.g. via \`mock.module()\` with a non-existent specifier), the package manager calls \`manager.log.addErrorFmt()\` on its stale \`log\` pointer — which may point to a destroyed stack frame from a previous call to \`resolveMaybeNeedsTrailingSlash\`. This causes a stack-buffer-overflow (ASAN fingerprint: \`Address:stack-buffer-overflow:bun-debug+0xc9f417a\`). ## Fix Apply the same save/restore pattern already used in \`ModuleLoader.zig\` (lines 188-198) to also update and restore \`pm.log\` in the defer block: \`\`\`zig if (jsc_vm.transpiler.resolver.package_manager) |pm| { pm.log = &log; } defer { if (jsc_vm.transpiler.resolver.package_manager) |pm| { pm.log = old_log; } } \`\`\` --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
) ## 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`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
`resolveMaybeNeedsTrailingSlash` in `VirtualMachine.zig` creates a stack-local `logger.Log` and temporarily sets it on the resolver and linker:
```zig
var log = logger.Log.init(bun.default_allocator);
defer log.deinit();
jsc_vm.transpiler.resolver.log = &log;
jsc_vm.transpiler.linker.log = &log;
```
However, it does not update `PackageManager.log`. When module resolution triggers auto-install (e.g. via `mock.module()` with a non-existent specifier), the package manager calls `manager.log.addErrorFmt()` on its stale `log` pointer — which may point to a destroyed stack frame from a previous call to `resolveMaybeNeedsTrailingSlash`.
This causes a stack-buffer-overflow (ASAN fingerprint: `Address:stack-buffer-overflow:bun-debug+0xc9f417a`).
Fix
Apply the same save/restore pattern already used in `ModuleLoader.zig` (lines 188-198) to also update and restore `pm.log` in the defer block:
```zig
if (jsc_vm.transpiler.resolver.package_manager) |pm| {
pm.log = &log;
}
defer {
if (jsc_vm.transpiler.resolver.package_manager) |pm| {
pm.log = old_log;
}
}
```