From 8c280f35b3f787b1697e0139f70382b5a1489a4d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 07:16:39 +0000 Subject: [PATCH] node:module: throw instead of crashing when _resolveFilename is set to a non-callable The _resolveFilename setter stored any cell in the slot that the builtin function lives in and flagged it as an override, and the require() path then called it without checking that it is callable. Assigning an object or a string and calling require() segfaulted (debug: ASSERTION FAILED: overrideHandler->isCallable()). Values that are not cells were silently dropped. Keep the builtin in its own slot and store whatever was assigned in a separate WriteBarrier, so the property behaves like Node's plain data property: any value can be assigned and reads back, and require(), require.resolve() and createRequire() requires throw "TypeError: Module._resolveFilename is not a function" when the stored value cannot be called, which is Node's behavior. --- src/jsc/bindings/ImportMetaObject.cpp | 74 ++++++++++--------- src/jsc/bindings/ZigGlobalObject.h | 6 +- src/jsc/modules/NodeModuleModule.cpp | 33 ++++++--- .../js/node/module/node-module-module.test.js | 70 ++++++++++++++++++ 4 files changed, 134 insertions(+), 49 deletions(-) diff --git a/src/jsc/bindings/ImportMetaObject.cpp b/src/jsc/bindings/ImportMetaObject.cpp index d01ce5d6802c..136370982f29 100644 --- a/src/jsc/bindings/ImportMetaObject.cpp +++ b/src/jsc/bindings/ImportMetaObject.cpp @@ -226,47 +226,49 @@ extern "C" JSC::EncodedJSValue functionImportMeta__resolveSyncPrivate(JSC::JSGlo if (!isESM) { if (globalObject) [[likely]] { if (globalObject->hasOverriddenModuleResolveFilenameFunction) [[unlikely]] { - auto overrideHandler = uncheckedDowncast(globalObject->m_moduleResolveFilenameFunction.getInitializedOnMainThread(globalObject)); - if (overrideHandler) [[likely]] { - ASSERT(overrideHandler->isCallable()); - JSValue parentModuleObject = globalObject->requireMap()->get(globalObject, from); - - JSValue parentID = jsUndefined(); - if (auto* parent = dynamicDowncast(parentModuleObject)) { - parentID = parent->filename(); - } else { - parentID = from; - } + JSValue overrideHandler = globalObject->m_moduleResolveFilenameOverride.get(); + JSC::CallData overrideCallData = JSC::getCallData(overrideHandler); + if (overrideCallData.type == JSC::CallData::Type::None) [[unlikely]] { + return JSC::throwVMTypeError(lexicalGlobalObject, scope, "Module._resolveFilename is not a function"_s); + } - MarkedArgumentBuffer args; - args.append(moduleName); - args.append(parentModuleObject); - auto parentIdStr = parentID.toWTFString(globalObject); - auto bunStr = Bun::toString(parentIdStr); - args.append(jsBoolean(Bun__isBunMain(lexicalGlobalObject, &bunStr))); - - // Pass options object with paths if provided - if (!userPathList.isUndefinedOrNull()) { - JSObject* options = JSC::constructEmptyObject(globalObject); - options->putDirect(vm, JSC::Identifier::fromString(vm, "paths"_s), userPathList); - args.append(options); - } + JSValue parentModuleObject = globalObject->requireMap()->get(globalObject, from); + + JSValue parentID = jsUndefined(); + if (auto* parent = dynamicDowncast(parentModuleObject)) { + parentID = parent->filename(); + } else { + parentID = from; + } - JSValue result = JSC::profiledCall(lexicalGlobalObject, ProfilingReason::API, overrideHandler, JSC::getCallData(overrideHandler), parentModuleObject, args); + MarkedArgumentBuffer args; + args.append(moduleName); + args.append(parentModuleObject); + auto parentIdStr = parentID.toWTFString(globalObject); + auto bunStr = Bun::toString(parentIdStr); + args.append(jsBoolean(Bun__isBunMain(lexicalGlobalObject, &bunStr))); + + // Pass options object with paths if provided + if (!userPathList.isUndefinedOrNull()) { + JSObject* options = JSC::constructEmptyObject(globalObject); + options->putDirect(vm, JSC::Identifier::fromString(vm, "paths"_s), userPathList); + args.append(options); + } + + JSValue result = JSC::profiledCall(lexicalGlobalObject, ProfilingReason::API, overrideHandler, overrideCallData, parentModuleObject, args); + RETURN_IF_EXCEPTION(scope, {}); + if (!isRequireDotResolve) { + JSString* string = result.toString(globalObject); RETURN_IF_EXCEPTION(scope, {}); - if (!isRequireDotResolve) { - JSString* string = result.toString(globalObject); - RETURN_IF_EXCEPTION(scope, {}); - auto str = string->value(globalObject); - RETURN_IF_EXCEPTION(scope, {}); - WTF::String prefixed = Bun::isUnprefixedNodeBuiltin(str); - if (!prefixed.isNull()) { - return JSValue::encode(jsString(vm, prefixed)); - } - return JSC::JSValue::encode(string); + auto str = string->value(globalObject); + RETURN_IF_EXCEPTION(scope, {}); + WTF::String prefixed = Bun::isUnprefixedNodeBuiltin(str); + if (!prefixed.isNull()) { + return JSValue::encode(jsString(vm, prefixed)); } - return JSC::JSValue::encode(result); + return JSC::JSValue::encode(string); } + return JSC::JSValue::encode(result); } } diff --git a/src/jsc/bindings/ZigGlobalObject.h b/src/jsc/bindings/ZigGlobalObject.h index 35a8a4f69ec6..35765cf1088e 100644 --- a/src/jsc/bindings/ZigGlobalObject.h +++ b/src/jsc/bindings/ZigGlobalObject.h @@ -516,7 +516,11 @@ class GlobalObject : public Bun::GlobalScope { \ /* TODO: these should use LazyProperty */ \ \ - V(public, LazyPropertyOfGlobalObject, m_moduleResolveFilenameFunction) \ + V(public, LazyPropertyOfGlobalObject, m_moduleResolveFilenameFunction) \ + /* Whatever user code assigned to require("module")._resolveFilename. Like Node's plain data */ \ + /* property it holds any value; require() throws if it is not callable. Only meaningful while */ \ + /* hasOverriddenModuleResolveFilenameFunction is set. */ \ + V(public, WriteBarrier, m_moduleResolveFilenameOverride) \ V(public, LazyPropertyOfGlobalObject, m_moduleRunMainFunction) \ V(public, LazyPropertyOfGlobalObject, m_modulePrototypeUnderscoreCompileFunction) \ V(public, LazyPropertyOfGlobalObject, m_commonJSRequireESMFromHijackedExtensionFunction) \ diff --git a/src/jsc/modules/NodeModuleModule.cpp b/src/jsc/modules/NodeModuleModule.cpp index b8732d7d8fee..7cf527d8f779 100644 --- a/src/jsc/modules/NodeModuleModule.cpp +++ b/src/jsc/modules/NodeModuleModule.cpp @@ -431,6 +431,9 @@ JSC_DEFINE_CUSTOM_GETTER(nodeModuleResolveFilename, PropertyName propertyName)) { auto* globalObject = defaultGlobalObject(lexicalGlobalObject); + if (globalObject->hasOverriddenModuleResolveFilenameFunction) [[unlikely]] { + return JSValue::encode(globalObject->m_moduleResolveFilenameOverride.get()); + } return JSValue::encode( globalObject->m_moduleResolveFilenameFunction.getInitializedOnMainThread( globalObject)); @@ -443,20 +446,26 @@ JSC_DEFINE_CUSTOM_SETTER(setNodeModuleResolveFilename, { auto* globalObject = defaultGlobalObject(lexicalGlobalObject); auto value = JSValue::decode(encodedValue); - if (value.isCell()) { - bool isOriginal = false; - if (value.isCallable()) { - JSC::CallData callData = JSC::getCallData(value); + bool isOriginal = false; + if (value.isCallable()) { + JSC::CallData callData = JSC::getCallData(value); - if (callData.type == JSC::CallData::Type::Native) { - if (callData.native.function.untaggedPtr() == &jsFunctionResolveFileName) { - isOriginal = true; - } + if (callData.type == JSC::CallData::Type::Native) { + if (callData.native.function.untaggedPtr() == &jsFunctionResolveFileName) { + isOriginal = true; } } - globalObject->hasOverriddenModuleResolveFilenameFunction = !isOriginal; - globalObject->m_moduleResolveFilenameFunction.set( - lexicalGlobalObject->vm(), globalObject, value.asCell()); + } + + if (isOriginal) { + globalObject->hasOverriddenModuleResolveFilenameFunction = false; + globalObject->m_moduleResolveFilenameOverride.clear(); + } else { + // Any value is accepted, as with Node's plain data property; whether it + // is callable is checked when require() goes to call it. + globalObject->m_moduleResolveFilenameOverride.set( + lexicalGlobalObject->vm(), globalObject, value); + globalObject->hasOverriddenModuleResolveFilenameFunction = true; } return true; @@ -1152,7 +1161,7 @@ void addNodeModuleConstructorProperties(JSC::VM& vm, }); globalObject->m_moduleResolveFilenameFunction.initLater( - [](const Zig::GlobalObject::Initializer& init) { + [](const Zig::GlobalObject::Initializer& init) { JSFunction* resolveFilenameFunction = JSFunction::create( init.vm, init.owner, 2, "_resolveFilename"_s, jsFunctionResolveFileName, JSC::ImplementationVisibility::Public, diff --git a/test/js/node/module/node-module-module.test.js b/test/js/node/module/node-module-module.test.js index 9014990b19f1..356fcebf00bd 100644 --- a/test/js/node/module/node-module-module.test.js +++ b/test/js/node/module/node-module-module.test.js @@ -478,6 +478,76 @@ console.log("survived", require("./late.js"));`, expect(await proc.exited).toBe(0); }); + test("Overwriting _resolveFilename with a non-callable makes require() throw like Node", async () => { + // Node keeps _resolveFilename as a plain data property: any value can be + // assigned and reads back, and require() throws when it goes to call it. + using dir = tempDir("resolve-filename-non-callable", { + "dep.cjs": `module.exports = "dep";`, + "main.cjs": ` + const Module = require("module"); + const original = Module._resolveFilename; + const attempt = fn => { + try { + return "returned " + String(fn()); + } catch (e) { + return e.constructor.name + ": " + e.message; + } + }; + const results = {}; + for (const [label, value] of [ + ["object", {}], + ["string", "not a function"], + ["undefined", undefined], + ["null", null], + ["number", 42], + ["symbol", Symbol("s")], + ]) { + Module._resolveFilename = value; + results[label] = { + readsBack: Object.is(Module._resolveFilename, value), + require: attempt(() => require("./dep.cjs")), + requireResolve: attempt(() => require.resolve("./dep.cjs")), + createRequire: attempt(() => Module.createRequire(__filename)("./dep.cjs")), + }; + } + // Callable objects other than plain functions are still honored. + Module._resolveFilename = new Proxy(original, {}); + results.callableProxy = attempt(() => require("./dep.cjs")); + Module._resolveFilename = original; + results.restored = { + readsBack: Module._resolveFilename === original, + require: attempt(() => require("./dep.cjs")), + }; + console.log(JSON.stringify(results)); + `, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + const notAFunction = { + readsBack: true, + require: "TypeError: Module._resolveFilename is not a function", + requireResolve: "TypeError: Module._resolveFilename is not a function", + createRequire: "TypeError: Module._resolveFilename is not a function", + }; + expect(JSON.parse(stdout)).toEqual({ + object: notAFunction, + string: notAFunction, + undefined: notAFunction, + null: notAFunction, + number: notAFunction, + symbol: notAFunction, + callableProxy: "returned dep", + restored: { readsBack: true, require: "returned dep" }, + }); + expect(exitCode).toBe(0); + }); + test("Overwriting Module.prototype.require", async () => { await using proc = Bun.spawn({ cmd: [bunExe(), "run", path.join(import.meta.dir, "modulePrototypeOverwrite.cjs")],