node:module: throw instead of crashing when _resolveFilename is set to a non-callable

[robobun]

Problem

• require("module")._resolveFilename = {} (or a string, or any other non-callable object) followed by require("./x") or require.resolve("./x") crashes the process: panic(main thread): Segmentation fault at address 0x5 in release builds, ASSERTION FAILED: overrideHandler->isCallable() at ImportMetaObject.cpp:231 (functionImportMeta__resolveSyncPrivate) in debug builds.
• Node throws TypeError: Module._resolveFilename is not a function from the require() call instead; the assignment itself is allowed and the property reads back as whatever was assigned.
• Cause: the custom setter setNodeModuleResolveFilename (src/jsc/modules/NodeModuleModule.cpp:439) stores any cell into m_moduleResolveFilenameFunction, the LazyProperty that otherwise holds the builtin, and sets hasOverriddenModuleResolveFilenameFunction. The require() path (src/jsc/bindings/ImportMetaObject.cpp:228) then downcasts the stored cell to a JSObject and calls it, trusting the assertion that it is callable. The same setter silently dropped non-cell values (undefined, null, numbers), so Module._resolveFilename = undefined left the builtin in place, unlike Node.

Fix

• Adds a WriteBarrier<JSC::Unknown> slot (m_moduleResolveFilenameOverride) on the global object for the user-assigned value. The setter stores whatever was assigned there (any value, as a plain data property would) and flags the override; assigning the builtin back clears both. m_moduleResolveFilenameFunction now only ever holds the builtin and is typed LazyProperty<JSFunction> accordingly. The getter returns the override while one is set.
• The slot lives on Zig::GlobalObject because the rest of this feature's per-realm state (the builtin's slot and hasOverriddenModuleResolveFilenameFunction) already does, and it sits next to them; the Module constructor object itself is a plain InternalFunction with no fields of its own, so holding the value there would mean splitting the state across two objects. hasOverriddenModuleResolveFilenameFunction is kept as the fast-path check (it is also what bun:internal-for-testing exposes and what bun test --isolate: opt-in global reuse fast path #36871 snapshots).
• functionImportMeta__resolveSyncPrivate looks up the override's CallData once; when there is none it throws TypeError: Module._resolveFilename is not a function, otherwise it calls through as before. Callable non-functions (e.g. a Proxy around a function) keep working because the check is getCallData, not a JSFunction type check.
• Why this is correct: it matches Node. _resolveFilename there is a plain data property, so assignment always succeeds and reads back, and only Module._load calling it throws; require(), require.resolve() and createRequire() requires all reach that one call, and in Bun all three reach this one native function, so a single check covers them. ESM resolution does not consult _resolveFilename in either runtime and is unchanged (the check is inside the existing !isESM branch).
• Verified with test/js/node/module/node-module-module.test.js ("Overwriting _resolveFilename with a non-callable makes require() throw like Node"), which runs a child that assigns {}, a string, undefined, null, a number and a symbol and checks that each reads back and that require, require.resolve and a createRequire require throw Node's message, that a callable Proxy override is honored, and that restoring the original makes require work again. The child's expected output was generated by running the same script under Node v26.3.0. The test crashes the child on the released binary and passes with this change.
• Also ran test/js/bun/resolve/import-meta.test.js (covers the slow-path flag toggling), test/js/bun/resolve/builtin-esm-lazy-exports.test.ts (reads the getter when building the node:module namespace) and test/js/node/module/module-resolve-filename-paths.test.js with the debug build; all pass.
• The block in ImportMetaObject.cpp is also being reworked by node:module: pass Node-compatible arguments to an overridden Module._resolveFilename #34102 (which changes the arguments passed to the override and keeps the assertion this PR removes); the two changes are independent, whichever lands second needs a small rebase of that hunk.
• Intentionally excluded: Module.runMain (setModuleRunMain / NodeModuleModule__callOverriddenRunMain, same file) has the same setter and call-site pattern, reachable only when a preload assigns it. It is left out of this PR because the Rust entry-point loader currently reports any exception from a runMain override as a bare Error occurred loading entry point: JSError, so a TypeError thrown there would be swallowed; the crash and that reporting need to be fixed together and are tracked as a separate change.

Background

• Module._resolveFilename in Bun is a custom accessor on the node:module constructor rather than a data property. Assigning it runs setNodeModuleResolveFilename; reading it runs the matching getter. The native require() fast path skips the JS-visible property entirely and only consults the override when hasOverriddenModuleResolveFilenameFunction is set, which is why the setter has to record the override somewhere native code can see it.
• LazyProperty<T> is JSC's "create on first access" slot; it can only hold a T* cell, which is why the old setter could not store primitives. WriteBarrier<Unknown> is a GC-visited slot that holds any JSValue. Members of Zig::GlobalObject declared through FOR_EACH_GLOBALOBJECT_GC_MEMBER are visited by the garbage collector automatically, so the new slot keeps the assigned value alive. Error.prepareStackTrace already uses this same two-slot arrangement (builtin in a LazyProperty, user value in a WriteBarrier<Unknown>).
• JSC::getCallData(value) returns CallData::Type::None for anything that cannot be called (primitives, plain objects, strings) and a usable CallData for functions, bound functions and callable proxies; profiledCall takes that CallData, so computing it once serves both the check and the call.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 25 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e3bc6623-98d0-4a04-bcfa-57b3dd029bab

📥 Commits

Reviewing files that changed from the base of the PR and between b7a0431 and 8c280f3.

📒 Files selected for processing (4)

• src/jsc/bindings/ImportMetaObject.cpp
• src/jsc/bindings/ZigGlobalObject.h
• src/jsc/modules/NodeModuleModule.cpp
• test/js/node/module/node-module-module.test.js

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: fix and test pushed, waiting on CI.

Reproduced on the released 1.4.0 canary (da3851e) and on a debug build of main (04148c8):

const Module = require("module");
Module._resolveFilename = {};   // or "a string"
require("./anything");          // release: Segmentation fault at address 0x5; debug: ASSERTION FAILED: overrideHandler->isCallable()

require.resolve() and a createRequire() require crash the same way. Node 26 throws TypeError: Module._resolveFilename is not a function for all three, which is what this PR makes Bun do. The new case in test/js/node/module/node-module-module.test.js crashes the child process on the released binary and passes with this branch.

The equivalent problem with Module.runMain assigned a non-callable in a preload is tracked separately; it also needs the entry-point error reporting on the Rust side fixed, so it is not part of this PR.

[claude]

Beyond the inline nit, I also checked: the new m_moduleResolveFilenameOverride slot is GC-visited via the visitGlobalObjectMember(WriteBarrier<T>&) overload wired through FOR_EACH_GLOBALOBJECT_GC_MEMBER; and the LazyProperty<JSCell> → LazyProperty<JSFunction> retype has no other consumers (only the getter and initLater reference m_moduleResolveFilenameFunction).

Extended reasoning...

The inline nit about Module.runMain covers the one same-class site; beyond that I verified the two memory-safety-adjacent concerns a reviewer would check first on a change that adds a field to Zig::GlobalObject: (1) the new WriteBarrier<Unknown> is declared inside FOR_EACH_GLOBALOBJECT_GC_MEMBER, and ZigGlobalObject.cpp has a visitGlobalObjectMember(Visitor&, WriteBarrier<T>&) overload that appends it, so the assigned override value is kept alive; (2) grepping the tree for m_moduleResolveFilenameFunction shows only the getter and the initLater in NodeModuleModule.cpp reference it, both updated for the JSFunction type, so nothing else is broken by the tightened element type.

[robobun]

Agreed that Module.runMain is the same pattern; it is intentionally left out of this PR and the description now says so. The reason is that the runMain override is invoked from the Rust entry-point loader, which currently turns any exception from the override into a bare Error occurred loading entry point: JSError (reproducible today with a callable override that throws), so adding the callable check alone would swap the crash for a swallowed TypeError. Fixing the crash and that reporting together is tracked as a separate change.

[robobun]

Heads up: #38099 adds RETURN_IF_EXCEPTION after requireMap()->get() and parentID.toWTFString() inside the same block this PR re-indents (plus two in the virtual-module branch above it). The two changes are independent; whichever lands second just needs a trivial rebase, and the exception checks should be kept in the re-indented version.

[robobun]

^{Updated 2:05 AM PT - Aug 13th, 2026}

@robobun, your commit 8c280f3 is building: #94317

[robobun]

^{Updated 5:35 AM PT - Aug 13th, 2026}

❌ @robobun, your commit 8c280f3 has some failures in Build #94317 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38089

That installs a local version of the PR into your bun-38089 executable, so you can run:

bun-38089 --bun