Add missing exception checks on module-loading paths - #40069
Conversation
- Object-loader plugins: a throwing `__esModule` / `default` getter on the exports object was handed to `reject()` with the exception still pending (a second throw / rejectedInternalPromise over it). Route through rejectOrResolve, which takes it first; use TRY_CLEAR_EXCEPTION there so a termination is not turned into a rejection. - Module.runMain: check after toWTFString before loadAndEvaluateModule. - import.meta.resolveSync / require resolution with virtual modules or an overridden Module._resolveFilename: check after each toWTFString, the requireMap get and constructEmptyObject. - $createCommonJSModule and JSCommonJSModule::create(key): check after toString / JSString::value / jsSubstring; napi_module_register checks its create() call. - module.filename / id / path setters: check after toString. - process.dlopen: stop draining pending napi registrations once one throws; check after the `module.exports` put. - bun:jsc heapStats: check after reading `options.dump`, its toWTFString, and the two JSONParse calls.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 minutes Limit details: You’ve used the included review currently available. Your 61 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 29 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
Comment |
|
Updated 12:13 PM PT - Aug 22nd, 2026
✅ @dylan-conway, your commit 6528de457db35c1cf2efee99d875b972ad6aedbd passed in 🧪 To try this PR locally: bunx bun-pr 40069That installs a local version of the PR into your bun-40069 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ks-module-loading
…w; node::AddEnvironmentCleanupHook path checks JSCommonJSModule::create too
There was a problem hiding this comment.
LGTM — both earlier findings were addressed in e078a58, and this pass found nothing new.
What was reviewed:
- Verified
m_pendingNapiModules.clear()now present at both dlopen cleanup sites and the v8/node.cppJSCommonJSModule::createcaller now checks — the two items from the prior review. - Confirmed the removed
!scope.exception()guard before themodule.exportsput in BunProcess.cpp is redundant:RETURN_IF_EXCEPTIONat lines 754/772 already cover it. rejectOrResolve({})replacingreject(scope.exception())in ModuleLoader.cpp correctly takes the pending exception viaTRY_CLEAR_EXCEPTION(leaves termination pending).- The lone CI failure (
test/internal/oxlint-plugin-bun.test.ts) is a lint test unrelated to these module-loading paths.
Extended reasoning...
Overview
Third batch of the missing-exception-check audit, focused on module loading/resolution. Adds DECLARE_THROW_SCOPE + RETURN_IF_EXCEPTION after throwing JSC calls (toWTFString, toString, getIfPropertyExists, JSString::value, jsSubstring, JSONParse, constructEmptyObject, map get) across ImportMetaObject.cpp, JSCommonJSModule.cpp, ModuleLoader.cpp, BunProcess.cpp, napi.cpp, v8/node.cpp, BunJSCModule.h, and NodeModuleModule.cpp. Also restructures process.dlopen's napi-registration drain loops to break on exception and clear leftover pending registrations. New tests cover the object-loader __esModule getter throw, Module.runMain argument stringify, and module setter conversion failures.
Security risks
None. All changes are defensive: adding early-return exception checks after operations that can throw. No new inputs, no new trust boundaries, no changes to validation logic.
Level of scrutiny
Moderate. This touches critical native module-loading code, but the changes follow the well-established JSC RETURN_IF_EXCEPTION discipline mechanically — the same pattern applied in the two prior batches of this audit. Every change is either (a) inserting an exception check after a call that can throw, or (b) routing an existing exception-pending path through the correct helper (rejectOrResolve / TRY_CLEAR_EXCEPTION). The one non-mechanical piece — break-on-exception in the dlopen napi drain loops — was flagged in my earlier review for leaking pending registrations, and the follow-up commit adds m_pendingNapiModules.clear() at both cleanup sites alongside the existing V8-vector clear.
Other factors
- I reviewed the previous revision and left two nit-level findings; both were addressed in e078a58 and both threads are resolved.
- The removed
!scope.exception()guard before the finalmodule.exportsput inProcess_functionDlopenis provably redundant:RETURN_IF_EXCEPTIONat lines 754 and 772 guarantee no exception is pending when control reaches that block, and the new check after theputis the correct placement. TRY_CLEAR_EXCEPTIONis the established repo macro (used across streams bindings) for clearing a non-termination exception while leaving termination pending — matches the PR description's rationale.- Tests were added for the user-reachable paths and the author reports the touched suites pass under
BUN_JSC_validateExceptionChecks=1. - The single CI failure is
test/internal/oxlint-plugin-bun.test.ts, a lint-check test unrelated to any file in this diff.
… the drain loops can just RETURN_IF_EXCEPTION
…ck runs (inside dlopen() and in the cached replay), not only after napi execution
There was a problem hiding this comment.
LGTM — the three prior review threads are addressed and the current Process_functionDlopen refactor traces correctly.
Checked: the scope-exit guard in both dlopen blocks now clears m_pendingNapiModules and the exports slots on every exit; RETURN_IF_EXCEPTION sits right after the V8-style register callback ran (post-dlopen() and after each node_module_register() in the unrolled replay loop) and after each executePendingNapiModule; the removed !scope.exception() guard on the final put() is dead given the preceding checks. TRY_CLEAR_EXCEPTION in rejectOrResolve matches its existing uses in the streams code so a termination stays pending. The JSCommonJSModule::create(JSString*) overload's three callers (napi.cpp, v8/node.cpp, and the WTF::String overload) are all covered.
Extended reasoning...
Overview
Third batch of missing-exception-check fixes on module-loading paths, touching 8 C++ files plus 2 test files. Most changes are one-line RETURN_IF_EXCEPTION insertions after toWTFString/toString/getIfPropertyExists/JSONParse/constructEmptyObject. The non-trivial piece is the Process_functionDlopen refactor in BunProcess.cpp: the manual post-loop state cleanup was replaced with a WTF::makeScopeExit guard, exception checks moved into the drain-loop bodies and after the V8-register callback sites, and the cached-replay std::visit unrolled to a plain if/else so the check sits next to node_module_register().
Security risks
None. These are exception-scope hygiene fixes — the failure mode without them is running further JSC operations with an exception pending, which trips BUN_JSC_validateExceptionChecks=1 in debug and is caught by the next check in release. No untrusted-input parsing, no auth/crypto/permissions.
Level of scrutiny
Medium. The dlopen refactor touches native-module loading and required three review iterations: (1) the break-on-exception left re-entrantly-registered napi modules in the global vector; (2) the sibling v8/node.cpp call site of the newly-throwing JSCommonJSModule::create() was missed; (3) moving the check into the loop body dropped coverage for the V8-only path where the loop never executes. All three are now fixed and the threads are resolved. I traced the final state line-by-line: the scope-exit guard fires on every return, and there is a RETURN_IF_EXCEPTION on every path where an nm_register_func (V8 or napi) or executePendingNapiModule could have thrown.
Other factors
New tests cover the object-loader __esModule getter throw, Module.runMain argument stringify failure, and the filename/id/path setter Symbol coercion — the user-reachable paths. The PR description confirms these plus the existing plugins, import-meta, bun-jsc, and heapStats-mimalloc tests pass under BUN_JSC_validateExceptionChecks=1. The bug-hunting system found nothing on this revision. The remaining edits (ImportMetaObject.cpp, JSCommonJSModule.cpp setters/create, BunJSCModule.h heapStats, NodeModuleModule.cpp runMain, ModuleLoader.cpp rejectOrResolve) are mechanical and match the surrounding code's conventions.
…ks-module-loading
There was a problem hiding this comment.
I've reviewed this across three rounds and the current state looks correct — all three earlier findings (stale m_pendingNapiModules on error, the missed v8/node.cpp sibling, and the V8-only registration path) are addressed by the scope-exit guard + the checks at BunProcess.cpp:611/624/658/676. Because the process.dlopen refactor reshapes error-path control flow in a critical native-loading path and took a few iterations to converge, a human sign-off would still be worthwhile.
What was reviewed:
Process_functionDlopen: verified themakeScopeExitguard fires on every early return in both self-register and cached-replay blocks, and thatm_pendingNapiModuleAndExports[0]is read into a local before the guard clears it.- Confirmed the removed
!scope.exception()guard at themodule.exportsput is dead — lines 749/767 already return on exception before reaching it. - Traced
JSCommonJSModule::create's newnullptrreturn through its callers; bothnapi.cppandv8/node.cppnow check. rejectOrResolve'sTRY_CLEAR_EXCEPTIONleaves a termination pending rather than converting it to a rejection.
Extended reasoning...
Overview
Third batch of the missing-exception-check audit, focused on module loading. Eight native files (BunProcess.cpp, ImportMetaObject.cpp, JSCommonJSModule.cpp, ModuleLoader.cpp, napi.cpp, v8/node.cpp, BunJSCModule.h, NodeModuleModule.cpp) plus two test files. Most hunks are the mechanical pattern — declare a throw scope, insert RETURN_IF_EXCEPTION after toWTFString/toString/getIfPropertyExists/JSONParse/constructEmptyObject. The non-mechanical piece is the Process_functionDlopen refactor: manual post-loop cleanup replaced with a WTF::makeScopeExit guard, per-iteration RETURN_IF_EXCEPTION in both drain loops, and the cached-replay std::visit flattened into a plain loop so the check sits next to node_module_register().
Security risks
None identified. This is exception-scope hygiene — release-build behavior is essentially unchanged (JSC checks after native calls anyway); the goal is correctness under BUN_JSC_validateExceptionChecks=1 and not running further JSC operations with an exception pending. No new user-controlled input parsing, no permission/auth surface.
Level of scrutiny
High for BunProcess.cpp — process.dlopen is native module loading, and the scope-exit refactor changes error-path ordering (state cleanup now happens via destructor rather than inline). This hunk went through three review rounds with me, each finding a real (nit-level) gap that was fixed: (1) stale m_pendingNapiModules on early-exit, (2) the v8/node.cpp twin of JSCommonJSModule::create not updated, (3) the V8-only registration path where the drain loop body never runs. All three are now addressed and I've verified the current diff against each. Low-to-medium for everything else — those are the standard RETURN_IF_EXCEPTION-after-throwing-call insertions the previous batches established.
Other factors
New tests cover the object-loader __esModule getter, Module.runMain argument stringify, and the module setter conversions; PR description confirms the affected suites pass under BUN_JSC_validateExceptionChecks=1. The one thing tipping this to defer rather than approve is that the dlopen refactor took multiple iterations to get right — a human familiar with the napi/V8 registration flow should confirm the scope-exit ordering and the std::visit → plain-loop rewrite before merge.
…ks-module-loading
There was a problem hiding this comment.
LGTM — all three prior findings are addressed and the current diff checks out.
What was reviewed:
- Re-traced both
Process_functionDlopenself-registration blocks: themakeScopeExitguard now clearsm_pendingNapiModulesalongside the V8 vector, andRETURN_IF_EXCEPTIONis placed right afterdlopen()and after each replayednode_module_register(), covering the V8-only path where the drain loop body doesn't execute. - Confirmed the
resultValueread at line 631/680 happens before the scope-exit guard clearsm_pendingNapiModuleAndExports[0], so the ABI-mismatch error-instance path still throws correctly. - Verified
v8/node.cppnow checks afterJSCommonJSModule::create(), matching the napi.cpp twin. - Checked the removed
!scope.exception()guard at BunProcess.cpp:777 is redundant given the precedingRETURN_IF_EXCEPTIONat 749/767.
Extended reasoning...
Overview
Third batch of a missing-exception-check audit, focused on module loading and resolution. Adds RETURN_IF_EXCEPTION after fallible JSC calls (toString, toWTFString, value(), jsSubstring, getIfPropertyExists, constructEmptyObject, JSONParse, put) across ImportMetaObject.cpp, JSCommonJSModule.cpp, ModuleLoader.cpp, napi.cpp, v8/node.cpp, BunJSCModule.h, and NodeModuleModule.cpp. The one non-mechanical change is the Process_functionDlopen refactor in BunProcess.cpp: the two self-registration cleanup sequences are now WTF::makeScopeExit guards, exception checks moved to sit directly after each point where a module's register callback runs, and the cached-replay std::visit became a plain loop so the check can sit next to node_module_register(). Two new tests cover the object-loader throwing-__esModule path and Module.runMain/setter string-conversion failures.
Security risks
None. These are exception-hygiene fixes — they don't introduce new inputs, change validation, or touch auth/crypto/permissions.
Level of scrutiny
High, and it received it: this is my fourth pass. Three prior rounds each surfaced a specific issue in the BunProcess.cpp refactor (stale m_pendingNapiModules on exception, the missed v8/node.cpp sibling, and the V8-only-registration path losing coverage when the check moved into the drain loop). All three were fixed in follow-up commits (e078a58, 19d0730), and I've re-verified each fix against the current file contents. The remaining seven files are mechanical one-to-three-line RETURN_IF_EXCEPTION insertions following the same pattern as prior batches.
Other factors
- The PR description states validation under
BUN_JSC_validateExceptionChecks=1for the affected test suites, which is the correct verification for this class of change per REVIEW.md. - The
ModuleLoader.cppchange fromreject(scope.exception())torejectOrResolve({})is an improvement: the pending exception is taken viaTRY_CLEAR_EXCEPTION(leaves a termination pending) rather than being passed to a secondthrowExceptionwhile still set. - The dropped
makeAtomStringinjsFunctionRunMainis intentional per the commit message —loadAndEvaluateModuleatomizes internally. - The multi-agent bug hunting system found nothing new on this revision.
3 participants
What does this PR do?
Third batch from the missing-exception-check audit: module loading / resolution.
ModuleLoader.cpp): when aBun.pluginonLoad/build.modulereturns{ loader: "object", exports }for a CommonJSrequire(), a throwing__esModuleordefaultgetter onexportswas passed toreject()with the exception still pending — a secondthrowException, orrejectedInternalPromisebuilt over it. It now goes throughrejectOrResolve, which takes the exception first; that helper usesTRY_CLEAR_EXCEPTIONso a VM termination is left pending rather than converted to a rejection.Module.runMain(x): check aftertoWTFStringbeforeloadAndEvaluateModule.import.meta.resolveSync/requireresolution when virtual modules exist orModule._resolveFilenameis overridden (ImportMetaObject.cpp): check after eachtoWTFString, therequireMapget, andconstructEmptyObject.$createCommonJSModule/JSCommonJSModule::create(key): check aftertoString,JSString::value,jsSubstring;napi_module_registerchecks itscreate().module.filename/id/pathsetters: check aftertoString.process.dlopen: stop draining pending napi registrations once one throws; check after themodule.exportsput.bun:jscheapStats(options): check after readingoptions.dump, its string conversion, and bothJSONParsecalls (the function had no scope).How did you verify your code works?
New tests: object-loader
__esModulegetter (plugins.test.ts),Module.runMainargument stringify and module setters (node-module-module.test.js). These already produced the right error in release — the fixes are about not running further JSC operations with the exception pending — and pass underBUN_JSC_validateExceptionChecks=1on a debug build along withplugins,import-meta,bun-jsc,heapStats-mimalloc.