mock.module: validate callback before running resolver#29485
mock.module: validate callback before running resolver#29485Jarred-Sumner merged 2 commits intomainfrom
Conversation
|
Updated 9:51 AM PT - Apr 19th, 2026
❌ @autofix-ci[bot], your commit 7484608 has 5 failures in
Add 🧪 To try this PR locally: bunx bun-pr 29485That installs a local version of the PR into your bun-29485 --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: 📥 CommitsReviewing files that changed from the base of the PR and between 4bb69e7be064bcca1eae75534cbf4dbcd9dc86ec and 7484608. 📒 Files selected for processing (3)
WalkthroughReordered validation in Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
Fixes #28557 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #28945 / #28956. Those gate on the specifier (control chars / whitespace / brackets) and would let |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward reorder of the callable check before resolveSpecifier() to fix the use-after-poison crash.
Extended reasoning...
Overview
The PR moves the callbackValue.isCallable() check in JSMock__jsModuleMock (BunPlugin.cpp) from after resolveSpecifier() to before it. Two test cases are added to mock-module-non-string.test.ts covering the TypeError and the subprocess registry-race scenario.
Security Risks
None. This is a defensive validation reorder — no new attack surface, no auth/crypto/permissions code touched.
Level of Scrutiny
Low. The change is a 6-line block move with no logic modifications. The error message, call path, and observable behavior for valid calls are unchanged. The only difference is that invalid calls now fail earlier (before the resolver runs).
Other Factors
- No bugs found by the bug hunting system.
- The root cause (use-after-poison from event-loop re-entry during resolver) is well-documented in the PR description.
- Tests are well-structured and cover both the fast-path TypeError and the regression scenario.
JSMock__jsModuleMock previously ran resolveSpecifier() before checking whether the second argument was callable. When the specifier is a valid npm package name (e.g. "PbQ") and auto-install is enabled, the resolver reaches PackageManager.enqueueDependencyToRoot -> sleepUntil -> event loop re-entry while still inside the host function. Fuzzilli hit a use-after-poison on this path; #29255 closed the door for invalid package names but valid names still reach it. Move the callable check in front of resolveSpecifier() so that mock.module/vi.mock throws TypeError: mock(module, fn) requires a function without ever touching the resolver when no callback was supplied. The observable error is unchanged for callers. The new regression test stands up a local registry and runs vi.mock("PbQ") with --install=force in a subprocess. Before this change the resolver hits the registry and blocks; after, zero requests are made and the process exits cleanly.
robobun
force-pushed
the
farm/9591bc2d/mock-module-callback-before-resolve
branch
from
4bb69e7 to
74649b3
Compare
There was a problem hiding this comment.
LGTM — clean reorder of existing validation logic with solid regression tests.
Extended reasoning...
Overview
Moves the callback-callable check (6 lines) from after resolveSpecifier() to before it in JSMock__jsModuleMock in BunPlugin.cpp. Two regression tests are added to mock-module-non-string.test.ts: one unit test asserting TypeError is thrown before the resolver runs, and one subprocess test confirming zero registry requests are made when the callback is missing.
Security risks
None. This is an argument validation reorder in the mock testing API — no auth, crypto, or permission surfaces touched.
Level of scrutiny
Low. The diff is a mechanical reorder of a single block of existing code with no logic changes. The error message and observable behavior are identical; only the timing of the check moves earlier. The risk of regression is minimal.
Other factors
No outstanding reviewer comments. coderabbitai raised no actionable concerns. The prior related PRs (#28945, #28956) addressed different guard conditions (specifier characters), so this is genuinely complementary, not a duplicate. The subprocess test design (racing process exit against the first registry request) is a robust way to detect regression without flakiness.
There was a problem hiding this comment.
LGTM — surgical fix for the use-after-poison crash.
Extended reasoning...
Overview
This PR touches (reordering 6 lines in ), (two new test cases), and (whitespace-only table formatting fix).
Security risks
None. This is a defensive validation reorder — it causes / to throw before entering the resolver when the callback argument is missing or non-callable, preventing reentrant event-loop ticks and the resulting use-after-poison on the stack-temporary .
Level of scrutiny
Low. The C++ change is literally moving the existing callable-check block from after to before it. The check itself is unchanged; only its position in the function moved. The ordering doesn't affect any valid call (a valid call has a callable second argument regardless of when we check). The fix is the same reorder as the previously-closed #28946, now correctly covering the case where the specifier is a valid npm package name that bypasses the guard.
Other factors
No bugs were found by the automated hunting system. The two new tests directly cover the regression: one inline (multiple specifiers, both missing and non-callable callbacks) and one subprocess test that races process exit against a registry HTTP request to catch any future regression that would reach the auto-install path. The CI failures in the timeline reference commit ; the autofix.ci follow-up () addressed formatting, and the libuv/zig failures appear pre-existing and unrelated to this PR's changes.
|
Build #46487 failures are all pre-existing on main / unrelated to this change:
|
Jarred-Sumner
deleted the
farm/9591bc2d/mock-module-callback-before-resolve
branch
## 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>
2 participants
Crash
Fuzzilli hit a flaky use-after-poison (fingerprint
Address:use-after-poison:bun-debug+0x8f2ee1e) from:JSMock__jsModuleMockrunsresolveSpecifier()before checking whether the second argument is callable."PbQ"is a valid npm package name, so theisNPMPackageNamegate added in #29255 lets it through to the auto-install path:That re-entry (plus the
ResolveMessagethrown by the failed resolution, whosereferrerfield borrows from a stack-temporaryWTF::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.mockthrowTypeError: mock(module, fn) requires a functionwithout ever entering the resolver — no auto-install, no event-loop re-entry, noResolveMessage.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.tsgains two cases:mock.module(specifier)/mock.module(specifier, 123)throw the expectedTypeErrorfor several specifiers including valid npm package names.vi.mock("PbQ")under--install=forcewith 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.Also verified
mock-module.test.ts,mock-module-resolve-log.test.ts, andresolve-autoinstall-invalid-name.test.tsstill pass.Fingerprint:
Address:use-after-poison:bun-debug+0x8f2ee1e