Bun.plugin: fix segfault when an object loader result's exports getter throws - #37026
Conversation
…s getter throws handleOnLoadObjectResult set result.type to OnLoadResultTypeObject before reading the exports property. When that read threw, the exception branch stored the error but left the type as Object, so the caller treated the exception cell as the exports object, got nullptr from getObject(), and dereferenced it. Flip the type to OnLoadResultTypeError in that branch so the import rejects (or require throws) with the getter's error.
WalkthroughChangesObject Loader Errors
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/plugin/plugins.test.ts`:
- Around line 564-573: Update expectCleanFailure to assert that the captured
stderr is empty after awaiting the child process streams, then retain the
existing stdout assertion and exitCode check in their current order.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7281d303-4575-4ce9-aca1-5d1efb79231f
📒 Files selected for processing (2)
src/jsc/bindings/ModuleLoader.cpptest/js/bun/plugin/plugins.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line fix that tags the exception branch as OnLoadResultTypeError, matching the other error path in the same function.
What was reviewed:
- Confirmed
OnLoadResult.valueis a union and the caller'sOnLoadResultTypeObjectcase dereferencesvalue.object.getObject(), so the mislabeled type explains the null deref exactly. - Checked the sibling error branch (non-object
exports) already setsOnLoadResultTypeError; this change just brings the getter-throw branch to parity. - Tests: subprocess-isolated crash repros for
build.module+import(),build.module+require(), andbuild.onLoad+import(); stdout/stderr/exitCode asserted in order,it.concurrent, prior review comments (stderr assertion, duplicate harness import) all resolved on HEAD.
Extended reasoning...
Overview
The PR fixes a deterministic segfault in Bun.plugin when an object-loader result's exports property throws on read (via a user getter or Proxy trap). The native change is a single line in src/jsc/bindings/ModuleLoader.cpp: inside handleOnLoadObjectResult, the exception branch after getIfPropertyExists(..., exportsPublicName()) now sets result.type = OnLoadResultTypeError before storing the exception and returning. Previously the type was left at OnLoadResultTypeObject (set at the top of the function), so handleVirtualModuleResult took the Object case, called .getObject() on the stored exception cell, got nullptr, and dereferenced it. The other error branch in the same function (when exports exists but isn't an object) already sets OnLoadResultTypeError, so this is a straightforward consistency fix.
Three subprocess tests are added to test/js/bun/plugin/plugins.test.ts covering build.module + import(), build.module + require(), and build.onLoad + import(). Each spawns bun -e with bunEnv, catches the getter's error in-process, and the test asserts exact stdout, empty stderr, and exit code 0 (in that order).
Security risks
None. This turns a null-pointer crash into a properly propagated JS error. No new input surface, no privilege-relevant code.
Level of scrutiny
Low-to-medium. The native change is one line in an error path, mechanically mirrors the adjacent error branch, and the root-cause analysis in the PR description traces cleanly to the code. The test additions follow house conventions (it.concurrent, subprocess isolation for a crash repro, pipes drained concurrently, exit code asserted last).
Other factors
All prior review feedback is resolved: the CodeRabbit request for a stderr assertion was applied in 113ff8b, and my earlier note about a duplicate harness import was already handled by the autofix commit 295f468 (the mid-file import now pulls only tempDir). No outstanding threads. The bug-hunting system found no issues this run.
) ### Problem - #39804 (commit 025570f) changed `generateObjectModuleSourceCode` (`src/jsc/modules/ObjectModule.cpp:25`). A throwing getter on the exports object of a `loader: "object"` result now fails the import. Before, the loader exported `undefined` for it. - #39804 added tests for the `mock.module()` entry point only. Its description lists this change under "no repro", but a plugin reaches it: bun `1.4.0-canary.1+6e906e468` (before #39804) prints `boom=undefined` for the cases below. - This replaces #33793. Its code change landed through #39804. Its plugin test did not. ### Fix - Test only. Adds `describe("object loader with a throwing getter on an export")` to `test/js/bun/plugin/plugins.test.ts`, next to the #37026 block for a throwing getter on the result's `exports` property. - Three cases: `import()` and `require()` of a `build.module()` result, and `import()` of a `build.onLoad()` result. Each checks that the caught error is the object the getter threw. The getter sits between two plain exports. - Verified: all three fail with `USE_SYSTEM_BUN=1` (`1.4.0-canary.1+6e906e468`) and pass with a debug build of main at 40ef811. The full file passes there (45 tests). ### Background - A `loader: "object"` result becomes a synthetic module. `ModuleLoader.cpp:442` passes its `exports` object to `generateObjectModuleSourceCode`, which reads each own enumerable property once into the namespace. `require()` takes the same path after the `__esModule` check at `ModuleLoader.cpp:422`. - `mock.module()` of a module that is not loaded yet uses the same generator. That is the entry point #39804 tests. - #37026 covers the layer above: a getter on the `exports` property of the result object (`ModuleLoader.cpp:155`). <details><summary>Notes</summary> - Output on the old binary: `imported boom=undefined` for the two `import()` cases and `required boom=undefined` for the `require()` case. - #33793's test ran `import()` and `require()` in one subprocess. This version mirrors the shape of the #37026 block: one subprocess per entry point, exact `stdout`, empty `stderr`, exit code 0. - #33793's `mock.module()` test is not carried over. #39804 added two tests for that entry point in `test/js/bun/test/mock/mock-module.test.ts` (the import failure and the untouched namespace). Both of #33793's test cases pass on a debug build of main with no `src/` change. That is the basis for closing #33793. - Commands: `bun bd test test/js/bun/plugin/plugins.test.ts` (45 pass) and `USE_SYSTEM_BUN=1 bun test test/js/bun/plugin/plugins.test.ts -t "throwing getter on an export"` (3 fail). </details>
2 participants
Repro
Deterministic on 1.4.0 and main. The same crash happens through
build.onLoadreturning aloader: "object"result, and throughrequire()of the virtual module (there the fault address is thegetIfPropertyExistscall on the null object). UBSan on a debug build reportssrc/jsc/modules/ObjectModule.cpp:20:17: runtime error: member call on null pointer of type 'JSC::JSCell'.Cause
handleOnLoadObjectResultinsrc/jsc/bindings/ModuleLoader.cppsetsresult.type = OnLoadResultTypeObjectbefore reading theexportsproperty off the plugin's result object. When that property read throws (a user-defined getter, or a Proxy trap), the exception branch stored the exception inresult.value.errorbut returned with the type stillOnLoadResultTypeObject.handleVirtualModuleResultthen took the Object case,value.object.getObject()on the stored exception cell returned nullptr, and the null object was dereferenced: immediately viagetIfPropertyExistson therequire()path, or insidegenerateObjectModuleSourceCode's source generator on theimport()path.Fix
Set
result.type = OnLoadResultTypeErrorin the exception branch, matching every other error path in the function. The import now rejects (andrequire()throws) with the getter's error.Verification
New tests cover all three faces:
build.module+import(),build.module+require(), andbuild.onLoad+import(). Each spawns a subprocess and asserts the process exits 0 after printing the getter's error message.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file