From b280d9027990c9618dba45c50bb829da158a3db0 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 5 Jan 2019 06:02:33 +0800 Subject: [PATCH] src: simplify NativeModule caching and remove redundant data - Remove `NativeModule._source` - the compilation is now entirely done in C++ and `process.binding('natives')` is implemented directly in the binding loader so there is no need to store additional source code strings. - Instead of using an object as `NativeModule._cached` and insert into it after compilation of each native module, simply prebuild a JS map filled with all the native modules and infer the state of compilation through `mod.loading`/`mod.loaded`. - Rename `NativeModule.nonInternalExists` to `NativeModule.canBeRequiredByUsers` and precompute that property for all the native modules during bootstrap instead of branching in every require call during runtime. This also fixes the bug where `worker_threads` can be made available with `--expose-internals`. - Rename `NativeModule.requireForDeps` to `NativeModule.requireWithFallbackInDeps`. - Add a test to make sure we do not accidentally leak any module to the global namespace. Backport-PR-URL: https://github.com/nodejs/node/pull/25964 PR-URL: https://github.com/nodejs/node/pull/25352 Reviewed-By: Anna Henningsen --- lib/internal/bootstrap/cache.js | 24 ++-- lib/internal/bootstrap/loaders.js | 112 +++++++----------- lib/internal/modules/cjs/loader.js | 14 ++- lib/internal/modules/esm/default_resolve.js | 2 +- lib/internal/modules/esm/translators.js | 2 +- src/node_native_module.cc | 20 +++- src/node_native_module.h | 9 +- test/code-cache/test-code-cache.js | 5 +- test/parallel/test-internal-module-require.js | 112 ++++++++++++++++++ 9 files changed, 199 insertions(+), 101 deletions(-) create mode 100644 test/parallel/test-internal-module-require.js diff --git a/lib/internal/bootstrap/cache.js b/lib/internal/bootstrap/cache.js index d65a7192932657..e706ccb20dc813 100644 --- a/lib/internal/bootstrap/cache.js +++ b/lib/internal/bootstrap/cache.js @@ -7,14 +7,10 @@ const { NativeModule } = require('internal/bootstrap/loaders'); const { - source, getCodeCache, compileFunction + getCodeCache, compileFunction } = internalBinding('native_module'); const { hasTracing, hasInspector } = process.binding('config'); -const depsModule = Object.keys(source).filter( - (key) => NativeModule.isDepsModule(key) || key.startsWith('internal/deps') -); - // Modules with source code compiled in js2c that // cannot be compiled with the code cache. const cannotUseCache = [ @@ -29,7 +25,7 @@ const cannotUseCache = [ // the code cache is also used when compiling these two files. 'internal/bootstrap/loaders', 'internal/bootstrap/node' -].concat(depsModule); +]; // Skip modules that cannot be required when they are not // built into the binary. @@ -69,11 +65,19 @@ if (!process.versions.openssl) { ); } +const cachableBuiltins = []; +for (const id of NativeModule.map.keys()) { + if (id.startsWith('internal/deps') || + id.startsWith('v8/') || id.startsWith('node-inspect/')) { + cannotUseCache.push(id); + } + if (!cannotUseCache.includes(id)) { + cachableBuiltins.push(id); + } +} + module.exports = { - cachableBuiltins: Object.keys(source).filter( - (key) => !cannotUseCache.includes(key) - ), - getSource(id) { return source[id]; }, + cachableBuiltins, getCodeCache, compileFunction, cannotUseCache diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js index 813ea8b82d7b4f..4554526298b0a0 100644 --- a/lib/internal/bootstrap/loaders.js +++ b/lib/internal/bootstrap/loaders.js @@ -158,6 +158,12 @@ let internalBinding; // Create this WeakMap in js-land because V8 has no C++ API for WeakMap. internalBinding('module_wrap').callbackMap = new WeakMap(); +// Think of this as module.exports in this file even though it is not +// written in CommonJS style. +const loaderExports = { internalBinding, NativeModule }; +const loaderId = 'internal/bootstrap/loaders'; +const config = internalBinding('config'); + // Set up NativeModule. function NativeModule(id) { this.filename = `${id}.js`; @@ -167,34 +173,35 @@ function NativeModule(id) { this.exportKeys = undefined; this.loaded = false; this.loading = false; + if (id === loaderId) { + // Do not expose this to user land even with --expose-internals. + this.canBeRequiredByUsers = false; + } else if (id.startsWith('internal/')) { + this.canBeRequiredByUsers = config.exposeInternals; + } else { + this.canBeRequiredByUsers = true; + } } const { - source, + moduleIds, compileFunction } = internalBinding('native_module'); -NativeModule._source = source; -NativeModule._cache = {}; - -const config = internalBinding('config'); - -// Think of this as module.exports in this file even though it is not -// written in CommonJS style. -const loaderExports = { internalBinding, NativeModule }; -const loaderId = 'internal/bootstrap/loaders'; +NativeModule.map = new Map(); +for (var i = 0; i < moduleIds.length; ++i) { + const id = moduleIds[i]; + const mod = new NativeModule(id); + NativeModule.map.set(id, mod); +} NativeModule.require = function(id) { if (id === loaderId) { return loaderExports; } - const cached = NativeModule.getCached(id); - if (cached && (cached.loaded || cached.loading)) { - return cached.exports; - } - - if (!NativeModule.exists(id)) { + const mod = NativeModule.map.get(id); + if (!mod) { // Model the error off the internal/errors.js model, but // do not use that module given that it could actually be // the one causing the error if there's a bug in Node.js. @@ -205,62 +212,31 @@ NativeModule.require = function(id) { throw err; } - moduleLoadList.push(`NativeModule ${id}`); - - const nativeModule = new NativeModule(id); - - nativeModule.cache(); - nativeModule.compile(); - - return nativeModule.exports; -}; - -NativeModule.isDepsModule = function(id) { - return id.startsWith('node-inspect/') || id.startsWith('v8/'); -}; - -NativeModule.requireForDeps = function(id) { - if (!NativeModule.exists(id) || - // TODO(TimothyGu): remove when DEP0084 reaches end of life. - NativeModule.isDepsModule(id)) { - id = `internal/deps/${id}`; + if (mod.loaded || mod.loading) { + return mod.exports; } - return NativeModule.require(id); -}; -NativeModule.getCached = function(id) { - return NativeModule._cache[id]; + moduleLoadList.push(`NativeModule ${id}`); + mod.compile(); + return mod.exports; }; NativeModule.exists = function(id) { - return NativeModule._source.hasOwnProperty(id); + return NativeModule.map.has(id); }; -if (config.exposeInternals) { - NativeModule.nonInternalExists = function(id) { - // Do not expose this to user land even with --expose-internals. - if (id === loaderId) { - return false; - } - return NativeModule.exists(id); - }; - - NativeModule.isInternal = function(id) { - // Do not expose this to user land even with --expose-internals. - return id === loaderId; - }; -} else { - NativeModule.nonInternalExists = function(id) { - return NativeModule.exists(id) && !NativeModule.isInternal(id); - }; - - NativeModule.isInternal = function(id) { - return id.startsWith('internal/'); - }; -} +NativeModule.canBeRequiredByUsers = function(id) { + const mod = NativeModule.map.get(id); + return mod && mod.canBeRequiredByUsers; +}; -NativeModule.getSource = function(id) { - return NativeModule._source[id]; +// Allow internal modules from dependencies to require +// other modules from dependencies by providing fallbacks. +NativeModule.requireWithFallbackInDeps = function(request) { + if (!NativeModule.map.has(request)) { + request = `internal/deps/${request}`; + } + return NativeModule.require(request); }; const getOwn = (target, property, receiver) => { @@ -334,13 +310,13 @@ NativeModule.prototype.compile = function() { try { const requireFn = this.id.startsWith('internal/deps/') ? - NativeModule.requireForDeps : + NativeModule.requireWithFallbackInDeps : NativeModule.require; const fn = compileFunction(id); fn(this.exports, requireFn, this, process, internalBinding); - if (config.experimentalModules && !NativeModule.isInternal(this.id)) { + if (config.experimentalModules && this.canBeRequiredByUsers) { this.proxifyExports(); } @@ -350,10 +326,6 @@ NativeModule.prototype.compile = function() { } }; -NativeModule.prototype.cache = function() { - NativeModule._cache[this.id] = this; -}; - // Coverage must be turned on early, so that we can collect // it for Node.js' own internal libraries. if (process.env.NODE_V8_COVERAGE) { diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 04d2ea8a55e45a..0da026843f6232 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -113,8 +113,12 @@ function Module(id, parent) { this.children = []; } -const builtinModules = Object.keys(NativeModule._source) - .filter(NativeModule.nonInternalExists); +const builtinModules = []; +for (const [id, mod] of NativeModule.map) { + if (mod.canBeRequiredByUsers) { + builtinModules.push(id); + } +} Object.freeze(builtinModules); Module.builtinModules = builtinModules; @@ -423,7 +427,7 @@ if (isWindows) { var indexChars = [ 105, 110, 100, 101, 120, 46 ]; var indexLen = indexChars.length; Module._resolveLookupPaths = function(request, parent, newReturn) { - if (NativeModule.nonInternalExists(request)) { + if (NativeModule.canBeRequiredByUsers(request)) { debug('looking for %j in []', request); return (newReturn ? null : [request, []]); } @@ -540,7 +544,7 @@ Module._load = function(request, parent, isMain) { return cachedModule.exports; } - if (NativeModule.nonInternalExists(filename)) { + if (NativeModule.canBeRequiredByUsers(filename)) { debug('load native module %s', request); return NativeModule.require(filename); } @@ -573,7 +577,7 @@ function tryModuleLoad(module, filename) { } Module._resolveFilename = function(request, parent, isMain, options) { - if (NativeModule.nonInternalExists(request)) { + if (NativeModule.canBeRequiredByUsers(request)) { return request; } diff --git a/lib/internal/modules/esm/default_resolve.js b/lib/internal/modules/esm/default_resolve.js index 9aa54d09a1b07c..2cf9014a672ce0 100644 --- a/lib/internal/modules/esm/default_resolve.js +++ b/lib/internal/modules/esm/default_resolve.js @@ -53,7 +53,7 @@ const extensionFormatMap = { }; function resolve(specifier, parentURL) { - if (NativeModule.nonInternalExists(specifier)) { + if (NativeModule.canBeRequiredByUsers(specifier)) { return { url: specifier, format: 'builtin' diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 6bfc4c8a98ed83..776b8d584df21f 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -81,7 +81,7 @@ translators.set('builtin', async (url) => { // slice 'node:' scheme const id = url.slice(5); NativeModule.require(id); - const module = NativeModule.getCached(id); + const module = NativeModule.map.get(id); return createDynamicModule( [...module.exportKeys, 'default'], url, (reflect) => { debug(`Loading BuiltinModule ${url}`); diff --git a/src/node_native_module.cc b/src/node_native_module.cc index bc911795fba8c7..187c7c600f01fd 100644 --- a/src/node_native_module.cc +++ b/src/node_native_module.cc @@ -83,11 +83,21 @@ void NativeModuleLoader::GetCacheUsage( args.GetReturnValue().Set(result); } -void NativeModuleLoader::SourceObjectGetter( +void NativeModuleLoader::ModuleIdsGetter( Local property, const PropertyCallbackInfo& info) { Local context = info.GetIsolate()->GetCurrentContext(); - info.GetReturnValue().Set( - per_process::native_module_loader.GetSourceObject(context)); + Isolate* isolate = info.GetIsolate(); + + const NativeModuleRecordMap& source_ = + per_process::native_module_loader.source_; + std::vector> ids; + ids.reserve(source_.size()); + + for (auto const& x : source_) { + ids.push_back(OneByteString(isolate, x.first.c_str(), x.first.size())); + } + + info.GetReturnValue().Set(Array::New(isolate, ids.data(), ids.size())); } void NativeModuleLoader::ConfigStringGetter( @@ -306,8 +316,8 @@ void NativeModuleLoader::Initialize(Local target, .FromJust()); CHECK(target ->SetAccessor(env->context(), - env->source_string(), - SourceObjectGetter, + FIXED_ONE_BYTE_STRING(env->isolate(), "moduleIds"), + ModuleIdsGetter, nullptr, MaybeLocal(), DEFAULT, diff --git a/src/node_native_module.h b/src/node_native_module.h index 9aea8bed574876..62c417a0b61474 100644 --- a/src/node_native_module.h +++ b/src/node_native_module.h @@ -58,11 +58,10 @@ class NativeModuleLoader { private: static void GetCacheUsage(const v8::FunctionCallbackInfo& args); - // Passing map of builtin module source code into JS land as - // internalBinding('native_module').source - static void SourceObjectGetter( - v8::Local property, - const v8::PropertyCallbackInfo& info); + // Passing ids of builtin module source code into JS land as + // internalBinding('native_module').moduleIds + static void ModuleIdsGetter(v8::Local property, + const v8::PropertyCallbackInfo& info); // Passing config.gypi into JS land as internalBinding('native_module').config static void ConfigStringGetter( v8::Local property, diff --git a/test/code-cache/test-code-cache.js b/test/code-cache/test-code-cache.js index 367e72168a6b67..54bd0b0e2a66be 100644 --- a/test/code-cache/test-code-cache.js +++ b/test/code-cache/test-code-cache.js @@ -5,15 +5,12 @@ // and the cache is used when built in modules are compiled. // Otherwise, verifies that no cache is used when compiling builtins. -require('../common'); +const { isMainThread } = require('../common'); const assert = require('assert'); const { cachableBuiltins, cannotUseCache } = require('internal/bootstrap/cache'); -const { - isMainThread -} = require('worker_threads'); const { internalBinding diff --git a/test/parallel/test-internal-module-require.js b/test/parallel/test-internal-module-require.js new file mode 100644 index 00000000000000..17559d228774f7 --- /dev/null +++ b/test/parallel/test-internal-module-require.js @@ -0,0 +1,112 @@ +'use strict'; + +// Flags: --expose-internals +// This verifies that +// 1. We do not leak internal modules unless the --require-internals option +// is on. +// 2. We do not accidentally leak any modules to the public global scope. +// 3. Deprecated modules are properly deprecated. + +const common = require('../common'); + +if (!common.isMainThread) { + common.skip('Cannot test the existence of --expose-internals from worker'); +} + +const assert = require('assert'); +const fork = require('child_process').fork; + +const expectedPublicModules = new Set([ + '_http_agent', + '_http_client', + '_http_common', + '_http_incoming', + '_http_outgoing', + '_http_server', + '_stream_duplex', + '_stream_passthrough', + '_stream_readable', + '_stream_transform', + '_stream_wrap', + '_stream_writable', + '_tls_common', + '_tls_wrap', + 'assert', + 'async_hooks', + 'buffer', + 'child_process', + 'cluster', + 'console', + 'constants', + 'crypto', + 'dgram', + 'dns', + 'domain', + 'events', + 'fs', + 'http', + 'http2', + 'https', + 'inspector', + 'module', + 'net', + 'os', + 'path', + 'perf_hooks', + 'process', + 'punycode', + 'querystring', + 'readline', + 'repl', + 'stream', + 'string_decoder', + 'sys', + 'timers', + 'tls', + 'trace_events', + 'tty', + 'url', + 'util', + 'v8', + 'vm', + 'worker_threads', + 'zlib' +]); + +if (process.argv[2] === 'child') { + assert(!process.execArgv.includes('--expose-internals')); + process.once('message', ({ allBuiltins }) => { + const publicModules = new Set(); + for (const id of allBuiltins) { + if (id.startsWith('internal/')) { + common.expectsError(() => { + require(id); + }, { + code: 'MODULE_NOT_FOUND', + message: `Cannot find module '${id}'` + }); + } else { + require(id); + publicModules.add(id); + } + } + assert(allBuiltins.length > publicModules.size); + // Make sure all the public modules are available through + // require('module').builtinModules + assert.deepStrictEqual( + publicModules, + new Set(require('module').builtinModules) + ); + assert.deepStrictEqual(publicModules, expectedPublicModules); + }); +} else { + assert(process.execArgv.includes('--expose-internals')); + const child = fork(__filename, ['child'], { + execArgv: [] + }); + const { builtinModules } = require('module'); + // When --expose-internals is on, require('module').builtinModules + // contains internal modules. + const message = { allBuiltins: builtinModules }; + child.send(message); +}