mock.module: validate callback before running resolver#28946
Conversation
Move the callable check for the second argument in front of resolveSpecifier() so that mock.module/vi.mock throws TypeError without ever entering the module resolver when no callback was supplied. Running the resolver on an arbitrary user-provided string can re-enter the JS event loop via the package-manager auto-install path, which has hit several crashes from Fuzzilli. Failing fast on a missing callback sidesteps that path entirely for the common 1-arg misuse.
|
Updated 3:40 AM PT - Apr 7th, 2026
❌ @robobun, your commit 64d8436 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 28946That installs a local version of the PR into your bun-28946 --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)
WalkthroughReordered validation in Bun__jsModuleMock to check the second argument (callback function) earlier before specifier resolution. Added test case validating that mock.module fails fast when the callback argument is missing or invalid. Changes
Possibly related PRs
🚥 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 — simple reorder of the callback validation to run before the resolver, with a targeted regression test.
Extended reasoning...
Overview
The PR modifies in to move the callable-check for the second argument (the mock factory function) from after to before it. A corresponding regression test is added to .
Security risks
None. This is a validation-ordering fix that makes Bun fail earlier with a clean TypeError instead of passing an unvalidated string into the module resolver. It reduces attack surface by preventing junk strings from reaching the package-manager auto-install code path (which has a crash history per #28511, #28518, #28945).
Level of scrutiny
Low. The change is a pure code reorder: the callable-check block already existed and is unchanged; it is simply hoisted above . No new logic is introduced. The test directly exercises the crash reproducer from the fuzz report.
Other factors
The pre-existing infinite-spin bug in for pending promises (noted in the inline comment) is unrelated to this PR — it predates these changes and is not made worse by them. The fix itself is correct and minimal.
| resolveSpecifier(); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
|
|
||
| JSC::JSValue callbackValue = callframe->argument(1); | ||
| if (!callbackValue.isCell() || !callbackValue.isCallable()) { | ||
| scope.throwException(lexicalGlobalObject, JSC::createTypeError(lexicalGlobalObject, "mock(module, fn) requires a function"_s)); | ||
| return {}; | ||
| } | ||
|
|
||
| JSC::JSObject* callback = callbackValue.getObject(); | ||
|
|
||
| JSModuleMock* mock = JSModuleMock::create(vm, globalObject->mockModule.mockModuleStructure.getInitializedOnMainThread(globalObject), callback); |
There was a problem hiding this comment.
🟣 Pre-existing bug in the getJSValue lambda (same function this PR modifies): the while loop that unwraps Promise chains has no escape path for a Pending promise — default: { break; } exits only the switch, not the while, so when executeOnce returns a pending Promise the loop busy-spins forever, permanently hanging the JS event loop thread. This is triggered by calling mock.module('already-imported-module', async () => ({ default: 42 })) — the async factory returns a pending Promise synchronously, and the loop never exits.
Extended reasoning...
The bug: In the getJSValue lambda (lines 603–620 of BunPlugin.cpp), a while loop unwraps Promise chains returned by executeOnce. The loop condition is:
while (JSC::JSPromise* promise = jsDynamicCast<JSC::JSPromise*>(result))The three cases are:
- Rejected → throws and returns (correctly exits the loop)
- Fulfilled → updates
resultwith the resolved value (loop exits on next iteration once result is no longer a Promise) - Pending (
default) →break;which in C++ exits only the enclosingswitchstatement, not thewhileloop
Why the loop spins forever: Because result is never updated in the Pending branch, jsDynamicCast<JSC::JSPromise*>(result) evaluates to the same non-null pointer on the very next iteration. The loop has no sleep, no yield, and no way to drain microtasks between iterations — so a Pending promise can never transition to Fulfilled during iteration. The thread spins at 100% CPU until the process is killed.
Concrete trigger sequence:
import 'my-module'— module is now in the ESM registrymock.module('my-module', async () => ({ default: 42 }))- Inside
JSMock__jsModuleMock,esm->getreturns the existing entry, sogetJSValue()is called executeOncecalls the async factory; anasyncfunction always returns a Promise in Pending state synchronously (the microtask that resolves it hasn't run yet)- The
whileloop receives a PendingJSPromise, entersdefault: { break; }, exits the switch, immediately re-evaluates the while condition with the same unchangedresult— Pending again - Infinite loop — the JS event loop thread is permanently hung
Why existing code doesn't prevent it: The TODO comment // TODO: blocking wait for promise explicitly acknowledges that blocking on the promise is unimplemented. But the implementation is actively incorrect: instead of returning an error or the pending promise to the caller (as runVirtualModule correctly does at line ~930), it re-spins endlessly.
Contrast with runVirtualModule: That function handles Pending by return promise; — returning the pending promise to the caller for proper async resolution. getJSValue has no equivalent escape.
Fix: In the default/Pending case, either goto / break out of the while loop (e.g., using a labeled break or a flag variable), throw a descriptive error, or return the pending promise to the caller and handle it asynchronously as runVirtualModule does. The minimal safe fix is to add goto done; or restructure to break out of the outer loop when the status is Pending.
|
Giving up on this one. The fix itself (moving the callback check in front of But the gate's reported failures look unrelated to this change:
I can't reproduce the original Fuzzilli crash ( The broader class of crashes in this code path (specifiers that shouldn't reach the resolver at all) is already being addressed by #28945. Closing in favor of that. |
## Crash
Fuzzilli hit a flaky use-after-poison (fingerprint
`Address:use-after-poison:bun-debug+0x8f2ee1e`) from:
```js
const v3 = Bun.jest().vi;
try { v3.mock("PbQ"); } catch (e) {}
Bun.gc(true);
```
`JSMock__jsModuleMock` runs `resolveSpecifier()` before checking whether
the second argument is callable. `"PbQ"` is a valid npm package name, so
the `isNPMPackageName` gate added in #29255 lets it through to the
auto-install path:
```
PackageManager.enqueueDependencyToRoot
-> PackageManager.sleepUntil
-> EventLoop.tick() // re-entry while still inside JSMock__jsModuleMock
```
That re-entry (plus the `ResolveMessage` thrown by the failed
resolution, whose `referrer` field borrows from a stack-temporary
`WTF::String`) leaves the process in a state where a later GC can read
freed mimalloc memory. The crash reproduces only under fuzzilli's REPRL
with specific prior state, which is why it's flaky.
## Fix
Move the callable check for the second argument in front of
`resolveSpecifier()`. When the caller omits the callback (or passes
something non-callable), `mock.module` / `vi.mock` throw `TypeError:
mock(module, fn) requires a function` without ever entering the resolver
— no auto-install, no event-loop re-entry, no `ResolveMessage`.
The observable error is unchanged; only its timing moves earlier.
This is the same reorder as the previously-closed #28946. That PR was
closed in favour of #29255, which gated auto-install on
`isNPMPackageName`; but valid package names like `"PbQ"` pass that gate,
so the 1-arg misuse still reaches the resolver on main today.
## Test
`test/js/bun/test/mock/mock-module-non-string.test.ts` gains two cases:
- A direct assertion that `mock.module(specifier)` /
`mock.module(specifier, 123)` throw the expected `TypeError` for several
specifiers including valid npm package names.
- A subprocess test that runs `vi.mock("PbQ")` under `--install=force`
with a local registry. On main the resolver blocks on the registry (the
test races against the first request and fails fast with a clear
message); with this change the process throws immediately, makes
**zero** registry requests, and exits 0.
```
test/js/bun/test/mock/mock-module-non-string.test.ts:
(pass) mock.module throws TypeError for non-string first argument
(pass) mock.module still works with valid string argument
(pass) mock.module does not crash on specifiers that are not valid npm package names
(pass) mock.module throws TypeError without resolving when callback is missing
(pass) mock.module does not run the resolver when callback is missing
```
Also verified `mock-module.test.ts`, `mock-module-resolve-log.test.ts`,
and `resolve-autoinstall-invalid-name.test.ts` still pass.
Fingerprint: `Address:use-after-poison:bun-debug+0x8f2ee1e`
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
## Crash
Fuzzilli hit a flaky use-after-poison (fingerprint
`Address:use-after-poison:bun-debug+0x8f2ee1e`) from:
```js
const v3 = Bun.jest().vi;
try { v3.mock("PbQ"); } catch (e) {}
Bun.gc(true);
```
`JSMock__jsModuleMock` runs `resolveSpecifier()` before checking whether
the second argument is callable. `"PbQ"` is a valid npm package name, so
the `isNPMPackageName` gate added in oven-sh#29255 lets it through to the
auto-install path:
```
PackageManager.enqueueDependencyToRoot
-> PackageManager.sleepUntil
-> EventLoop.tick() // re-entry while still inside JSMock__jsModuleMock
```
That re-entry (plus the `ResolveMessage` thrown by the failed
resolution, whose `referrer` field borrows from a stack-temporary
`WTF::String`) leaves the process in a state where a later GC can read
freed mimalloc memory. The crash reproduces only under fuzzilli's REPRL
with specific prior state, which is why it's flaky.
## Fix
Move the callable check for the second argument in front of
`resolveSpecifier()`. When the caller omits the callback (or passes
something non-callable), `mock.module` / `vi.mock` throw `TypeError:
mock(module, fn) requires a function` without ever entering the resolver
— no auto-install, no event-loop re-entry, no `ResolveMessage`.
The observable error is unchanged; only its timing moves earlier.
This is the same reorder as the previously-closed oven-sh#28946. That PR was
closed in favour of oven-sh#29255, which gated auto-install on
`isNPMPackageName`; but valid package names like `"PbQ"` pass that gate,
so the 1-arg misuse still reaches the resolver on main today.
## Test
`test/js/bun/test/mock/mock-module-non-string.test.ts` gains two cases:
- A direct assertion that `mock.module(specifier)` /
`mock.module(specifier, 123)` throw the expected `TypeError` for several
specifiers including valid npm package names.
- A subprocess test that runs `vi.mock("PbQ")` under `--install=force`
with a local registry. On main the resolver blocks on the registry (the
test races against the first request and fails fast with a clear
message); with this change the process throws immediately, makes
**zero** registry requests, and exits 0.
```
test/js/bun/test/mock/mock-module-non-string.test.ts:
(pass) mock.module throws TypeError for non-string first argument
(pass) mock.module still works with valid string argument
(pass) mock.module does not crash on specifiers that are not valid npm package names
(pass) mock.module throws TypeError without resolving when callback is missing
(pass) mock.module does not run the resolver when callback is missing
```
Also verified `mock-module.test.ts`, `mock-module-resolve-log.test.ts`,
and `resolve-autoinstall-invalid-name.test.ts` still pass.
Fingerprint: `Address:use-after-poison:bun-debug+0x8f2ee1e`
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
## Crash
Fuzzilli hit a flaky use-after-poison (fingerprint
`Address:use-after-poison:bun-debug+0x8f2ee1e`) from:
```js
const v3 = Bun.jest().vi;
try { v3.mock("PbQ"); } catch (e) {}
Bun.gc(true);
```
`JSMock__jsModuleMock` runs `resolveSpecifier()` before checking whether
the second argument is callable. `"PbQ"` is a valid npm package name, so
the `isNPMPackageName` gate added in oven-sh#29255 lets it through to the
auto-install path:
```
PackageManager.enqueueDependencyToRoot
-> PackageManager.sleepUntil
-> EventLoop.tick() // re-entry while still inside JSMock__jsModuleMock
```
That re-entry (plus the `ResolveMessage` thrown by the failed
resolution, whose `referrer` field borrows from a stack-temporary
`WTF::String`) leaves the process in a state where a later GC can read
freed mimalloc memory. The crash reproduces only under fuzzilli's REPRL
with specific prior state, which is why it's flaky.
## Fix
Move the callable check for the second argument in front of
`resolveSpecifier()`. When the caller omits the callback (or passes
something non-callable), `mock.module` / `vi.mock` throw `TypeError:
mock(module, fn) requires a function` without ever entering the resolver
— no auto-install, no event-loop re-entry, no `ResolveMessage`.
The observable error is unchanged; only its timing moves earlier.
This is the same reorder as the previously-closed oven-sh#28946. That PR was
closed in favour of oven-sh#29255, which gated auto-install on
`isNPMPackageName`; but valid package names like `"PbQ"` pass that gate,
so the 1-arg misuse still reaches the resolver on main today.
## Test
`test/js/bun/test/mock/mock-module-non-string.test.ts` gains two cases:
- A direct assertion that `mock.module(specifier)` /
`mock.module(specifier, 123)` throw the expected `TypeError` for several
specifiers including valid npm package names.
- A subprocess test that runs `vi.mock("PbQ")` under `--install=force`
with a local registry. On main the resolver blocks on the registry (the
test races against the first request and fails fast with a clear
message); with this change the process throws immediately, makes
**zero** registry requests, and exits 0.
```
test/js/bun/test/mock/mock-module-non-string.test.ts:
(pass) mock.module throws TypeError for non-string first argument
(pass) mock.module still works with valid string argument
(pass) mock.module does not crash on specifiers that are not valid npm package names
(pass) mock.module throws TypeError without resolving when callback is missing
(pass) mock.module does not run the resolver when callback is missing
```
Also verified `mock-module.test.ts`, `mock-module-resolve-log.test.ts`,
and `resolve-autoinstall-invalid-name.test.ts` still pass.
Fingerprint: `Address:use-after-poison:bun-debug+0x8f2ee1e`
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
1 participant
Crash
Fuzzilli hit a flaky SIGSEGV (fingerprint
ac324f57f10d95ce) from this body:v4ends up as the string"function f3() {}". It is then passed as the first (and only) argument tovi.mock. The function is going to throwTypeError: mock(module, fn) requires a functionregardless — but before it gets there,JSMock__jsModuleMockrunsresolveSpecifier(), which feeds that junk string into the module resolver. The resolver in turn can reach the package manager auto-install path, which has a history of crashing on inputs it doesn't expect (see #28511, #28518, #28945).Fix
Move the callable check for the second argument in front of
resolveSpecifier(). When a caller forgets the callback (or passes something non-callable), we throwTypeError: mock(module, fn) requires a functionwithout ever touching the resolver. That avoids the entire auto-install re-entry for the 1-arg misuse and matches howjest.fn/vi.mockconsumers actually use the API.The error message and behavior for valid 2-arg calls are unchanged.
Test
Adds a regression test to
test/js/bun/test/mock/mock-module-non-string.test.tsthat exercises a few specifiers that could previously reach the resolver (a function source, an arrow function, a bogus package-name-with-braces), verifying each throwsTypeErrorcleanly.Fingerprint:
ac324f57f10d95ce