From 0cb6a72ade4248ee442ccef45b7ab61dc0171a42 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 6 Jul 2026 09:40:55 +0000 Subject: [PATCH] node:module: build builtinModules from the same table as isBuiltin, and freeze it module.builtinModules and module.isBuiltin() read from two separate hardcoded tables that had drifted apart: the builtinModules table was missing node:test and bun:main, both of which isBuiltin() reports as builtins and the module loader resolves. Tools that decide "is this a builtin?" with builtinModules.includes(x) therefore disagreed with Bun's own loader. The array was also mutable, so a push() from one module was visible to every later reader in the process. Node freezes it. Build the array from the table isBuiltinModule consults, and freeze it. --- src/jsc/bindings/isBuiltinModule.cpp | 146 ++++++++++-------- src/jsc/bindings/isBuiltinModule.h | 6 + src/jsc/modules/NodeModuleModule.cpp | 107 +++---------- .../js/node/module/node-module-module.test.js | 15 +- 4 files changed, 117 insertions(+), 157 deletions(-) diff --git a/src/jsc/bindings/isBuiltinModule.cpp b/src/jsc/bindings/isBuiltinModule.cpp index 1bfd6ad23f32..bc59e1aa5584 100644 --- a/src/jsc/bindings/isBuiltinModule.cpp +++ b/src/jsc/bindings/isBuiltinModule.cpp @@ -1,92 +1,104 @@ #include "root.h" +#include "isBuiltinModule.h" -static constexpr ASCIILiteral builtinModuleNamesSortedLength[] = { - "fs"_s, - "os"_s, - "v8"_s, - "vm"_s, - "ws"_s, - "bun"_s, - "dns"_s, - "net"_s, - "sys"_s, - "tls"_s, - "tty"_s, - "url"_s, - "http"_s, - "path"_s, - "repl"_s, - "util"_s, - "wasi"_s, - "zlib"_s, - "dgram"_s, - "http2"_s, - "https"_s, +// Bun's own modules and its thirdparty overrides are intentionally included, so +// that passing `module.builtinModules` as the 'external' option to a bundler +// properly excludes things like 'ws', which only work with Bun's native +// implementation and not the JS one on npm. +static constexpr ASCIILiteral builtinModuleNamesTable[] = { + "_http_agent"_s, + "_http_client"_s, + "_http_common"_s, + "_http_incoming"_s, + "_http_outgoing"_s, + "_http_server"_s, + "_stream_duplex"_s, + "_stream_passthrough"_s, + "_stream_readable"_s, + "_stream_transform"_s, + "_stream_wrap"_s, + "_stream_writable"_s, + "_tls_common"_s, + "_tls_wrap"_s, "assert"_s, + "assert/strict"_s, + "async_hooks"_s, "buffer"_s, - "crypto"_s, - "domain"_s, - "events"_s, - "module"_s, - "stream"_s, - "timers"_s, - "undici"_s, "bun:ffi"_s, "bun:jsc"_s, + "bun:main"_s, + "bun:sqlite"_s, + "bun:test"_s, + "bun:wrap"_s, + "bun"_s, + "child_process"_s, "cluster"_s, "console"_s, - "process"_s, - "bun:wrap"_s, - "punycode"_s, - "bun:test"_s, - "bun:main"_s, - "readline"_s, - "_tls_wrap"_s, "constants"_s, + "crypto"_s, + "dgram"_s, + "diagnostics_channel"_s, + "dns"_s, + "dns/promises"_s, + "domain"_s, + "events"_s, + "fs"_s, + "fs/promises"_s, + "http"_s, + "http2"_s, + "https"_s, "inspector"_s, - "node:test"_s, - "bun:sqlite"_s, + "inspector/promises"_s, + "module"_s, + "net"_s, + "os"_s, + "path"_s, "path/posix"_s, "path/win32"_s, "perf_hooks"_s, - "stream/web"_s, - "util/types"_s, - "_http_agent"_s, - "_tls_common"_s, - "async_hooks"_s, - "fs/promises"_s, + "process"_s, + "punycode"_s, "querystring"_s, - "_http_client"_s, - "_http_common"_s, - "_http_server"_s, - "_stream_wrap"_s, - "dns/promises"_s, - "trace_events"_s, - "assert/strict"_s, - "child_process"_s, - "_http_incoming"_s, - "_http_outgoing"_s, - "_stream_duplex"_s, - "string_decoder"_s, - "worker_threads"_s, + "readline"_s, + "readline/promises"_s, + "repl"_s, + "stream"_s, + "stream/consumers"_s, "stream/promises"_s, + "stream/web"_s, + "string_decoder"_s, + "sys"_s, + "timers"_s, "timers/promises"_s, - "_stream_readable"_s, - "_stream_writable"_s, - "stream/consumers"_s, - "_stream_transform"_s, - "readline/promises"_s, - "inspector/promises"_s, - "_stream_passthrough"_s, - "diagnostics_channel"_s, + "tls"_s, + "trace_events"_s, + "tty"_s, + "undici"_s, + "url"_s, + "util"_s, + "util/types"_s, + "v8"_s, + "vm"_s, + "wasi"_s, + "worker_threads"_s, + "ws"_s, + "zlib"_s, + // Builtins that only resolve with the "node:" prefix. Node lists these with + // the prefix too, since the bare name is not a builtin. + "node:test"_s, }; namespace Bun { +std::span builtinModuleNames() +{ + return builtinModuleNamesTable; +} + bool isBuiltinModule(const String& namePossiblyWithNodePrefix) { // First check the original name as-is - for (auto& builtinModule : builtinModuleNamesSortedLength) { + for (auto& builtinModule : builtinModuleNamesTable) { if (namePossiblyWithNodePrefix == builtinModule) return true; } @@ -97,7 +109,7 @@ bool isBuiltinModule(const String& namePossiblyWithNodePrefix) name = name.substringSharingImpl(5); // Check again with the prefix removed - for (auto& builtinModule : builtinModuleNamesSortedLength) { + for (auto& builtinModule : builtinModuleNamesTable) { if (name == builtinModule) return true; } diff --git a/src/jsc/bindings/isBuiltinModule.h b/src/jsc/bindings/isBuiltinModule.h index 2038c8188a81..40774b52ca95 100644 --- a/src/jsc/bindings/isBuiltinModule.h +++ b/src/jsc/bindings/isBuiltinModule.h @@ -1,6 +1,12 @@ #pragma once +#include + namespace Bun { +/// The publicly listed builtin specifiers. `module.builtinModules` is built from this +/// list and `isBuiltinModule` consults it, so the two cannot disagree. Specifiers the +/// loader resolves but does not expose, like `bun:app`, are deliberately absent. +std::span builtinModuleNames(); bool isBuiltinModule(const String& namePossiblyWithNodePrefix); String isUnprefixedNodeBuiltin(const String& name); } // namespace Bun diff --git a/src/jsc/modules/NodeModuleModule.cpp b/src/jsc/modules/NodeModuleModule.cpp index 1749ab1b64f4..57904e791fc0 100644 --- a/src/jsc/modules/NodeModuleModule.cpp +++ b/src/jsc/modules/NodeModuleModule.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include "JavaScriptCore/Completion.h" #include "JavaScriptCore/JSNativeStdFunction.h" #include "JSCommonJSExtensions.h" @@ -44,90 +45,6 @@ JSC_DECLARE_HOST_FUNCTION(jsFunctionWrap); JSC_DECLARE_CUSTOM_GETTER(getterRequireFunction); JSC_DECLARE_CUSTOM_SETTER(setterRequireFunction); -// This is a list of builtin module names that do not have the node prefix. It -// also includes Bun's builtin modules, as well as Bun's thirdparty overrides. -// The reason for overstuffing this list is so that uses that use these as the -// 'external' option to a bundler will properly exclude things like 'ws' which -// only work with Bun's native 'ws' implementation and not the JS one on NPM. -static constexpr ASCIILiteral builtinModuleNames[] = { - "_http_agent"_s, - "_http_client"_s, - "_http_common"_s, - "_http_incoming"_s, - "_http_outgoing"_s, - "_http_server"_s, - "_stream_duplex"_s, - "_stream_passthrough"_s, - "_stream_readable"_s, - "_stream_transform"_s, - "_stream_wrap"_s, - "_stream_writable"_s, - "_tls_common"_s, - "_tls_wrap"_s, - "assert"_s, - "assert/strict"_s, - "async_hooks"_s, - "buffer"_s, - "bun:ffi"_s, - "bun:jsc"_s, - "bun:sqlite"_s, - "bun:test"_s, - "bun:wrap"_s, - "bun"_s, - "child_process"_s, - "cluster"_s, - "console"_s, - "constants"_s, - "crypto"_s, - "dgram"_s, - "diagnostics_channel"_s, - "dns"_s, - "dns/promises"_s, - "domain"_s, - "events"_s, - "fs"_s, - "fs/promises"_s, - "http"_s, - "http2"_s, - "https"_s, - "inspector"_s, - "inspector/promises"_s, - "module"_s, - "net"_s, - "os"_s, - "path"_s, - "path/posix"_s, - "path/win32"_s, - "perf_hooks"_s, - "process"_s, - "punycode"_s, - "querystring"_s, - "readline"_s, - "readline/promises"_s, - "repl"_s, - "stream"_s, - "stream/consumers"_s, - "stream/promises"_s, - "stream/web"_s, - "string_decoder"_s, - "sys"_s, - "timers"_s, - "timers/promises"_s, - "tls"_s, - "trace_events"_s, - "tty"_s, - "undici"_s, - "url"_s, - "util"_s, - "util/types"_s, - "v8"_s, - "vm"_s, - "wasi"_s, - "worker_threads"_s, - "ws"_s, - "zlib"_s, -}; - template consteval std::size_t countof(T (&)[N]) { return N; @@ -655,15 +572,27 @@ static JSValue getSourceMapFunction(VM& vm, JSObject* moduleObject) static JSValue getBuiltinModulesObject(VM& vm, JSObject* moduleObject) { - MarkedArgumentBuffer args; - args.ensureCapacity(countof(builtinModuleNames)); + auto names = Bun::builtinModuleNames(); - for (unsigned i = 0; i < countof(builtinModuleNames); ++i) { - args.append(JSC::jsOwnedString(vm, String(builtinModuleNames[i]))); + MarkedArgumentBuffer args; + args.ensureCapacity(names.size()); + for (auto& name : names) { + args.append(JSC::jsOwnedString(vm, String(name))); } auto* globalObject = defaultGlobalObject(moduleObject->globalObject()); - return JSC::constructArray(globalObject, static_cast(nullptr), JSC::ArgList(args)); + auto scope = DECLARE_THROW_SCOPE(vm); + JSArray* array = JSC::constructArray(globalObject, static_cast(nullptr), JSC::ArgList(args)); + // Neither building nor freezing a dense array of strings runs user code. The + // assertions are what let the exception-check validator see both of those + // ThrowScopes as checked before the next one opens at the same depth. + scope.assertNoException(); + + // Node freezes this array. Every reader shares one instance, so leaving it + // mutable lets one package's mutation leak into all the others. + JSC::objectConstructorFreeze(globalObject, array); + scope.assertNoException(); + return array; } static JSValue getConstantsObject(VM& vm, JSObject* moduleObject) diff --git a/test/js/node/module/node-module-module.test.js b/test/js/node/module/node-module-module.test.js index af16e04632d0..3c7631d989f4 100644 --- a/test/js/node/module/node-module-module.test.js +++ b/test/js/node/module/node-module-module.test.js @@ -6,7 +6,20 @@ import path from "path"; describe.concurrent("node-module-module", () => { test("builtinModules exists", () => { expect(Array.isArray(builtinModules)).toBe(true); - expect(builtinModules).toHaveLength(76); + expect(builtinModules).toHaveLength(78); + }); + + test("builtinModules agrees with isBuiltin()", () => { + expect(builtinModules.filter(name => !isBuiltin(name))).toEqual([]); + // "node:test" only resolves with the prefix, so it is listed with the prefix. + expect(builtinModules).toContain("node:test"); + expect(builtinModules).not.toContain("test"); + }); + + test("builtinModules is frozen", () => { + expect(Object.isFrozen(builtinModules)).toBe(true); + expect(() => builtinModules.push("not-a-builtin")).toThrow(TypeError); + expect(builtinModules).not.toContain("not-a-builtin"); }); test("isBuiltin() works", () => {