Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modules: do not share the internal require function with public loaders #26549

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 27 additions & 30 deletions lib/internal/bootstrap/loaders.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
// can be created using NODE_MODULE_CONTEXT_AWARE_CPP() with the flag
// NM_F_LINKED.
// - internalBinding(): the private internal C++ binding loader, inaccessible
// from user land because they are only available from NativeModule.require().
// from user land unless through `require('internal/test/binding')`.
// These C++ bindings are created using NODE_MODULE_CONTEXT_AWARE_INTERNAL()
// and have their nm_flags set to NM_F_INTERNAL.
//
Expand All @@ -42,7 +42,7 @@
// This file is compiled as if it's wrapped in a function with arguments
// passed by node::RunBootstrapping()
/* global process, getLinkedBinding, getInternalBinding */
/* global experimentalModules, exposeInternals, primordials */
/* global exposeInternals, primordials */

const {
Reflect,
Expand Down Expand Up @@ -185,27 +185,9 @@ function nativeModuleRequire(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.
// eslint-disable-next-line no-restricted-syntax
const err = new Error(`No such built-in module: ${id}`);
err.code = 'ERR_UNKNOWN_BUILTIN_MODULE';
err.name = 'Error [ERR_UNKNOWN_BUILTIN_MODULE]';
throw err;
}

if (mod.loaded || mod.loading) {
return mod.exports;
}

moduleLoadList.push(`NativeModule ${id}`);
mod.compile();
return mod.exports;
return mod.compile();
}

NativeModule.require = nativeModuleRequire;
NativeModule.exists = function(id) {
return NativeModule.map.has(id);
};
Expand All @@ -217,11 +199,25 @@ NativeModule.canBeRequiredByUsers = function(id) {

// Allow internal modules from dependencies to require
// other modules from dependencies by providing fallbacks.
NativeModule.requireWithFallbackInDeps = function(request) {
function requireWithFallbackInDeps(request) {
if (!NativeModule.map.has(request)) {
request = `internal/deps/${request}`;
}
return NativeModule.require(request);
return nativeModuleRequire(request);
}

// This is exposed for public loaders
NativeModule.prototype.compileForPublicLoader = function(needToProxify) {
if (!this.canBeRequiredByUsers) {
// No code because this is an assertion against bugs
// eslint-disable-next-line no-restricted-syntax
throw new Error(`Should not compile ${this.id} for public use`);
}
this.compile();
if (needToProxify && !this.exportKeys) {
this.proxifyExports();
}
return this.exports;
};

const getOwn = (target, property, receiver) => {
Expand Down Expand Up @@ -289,26 +285,27 @@ NativeModule.prototype.proxifyExports = function() {
};

NativeModule.prototype.compile = function() {
const id = this.id;
if (this.loaded || this.loading) {
return this.exports;
}

const id = this.id;
this.loading = true;

try {
const requireFn = this.id.startsWith('internal/deps/') ?
NativeModule.requireWithFallbackInDeps :
NativeModule.require;
requireWithFallbackInDeps : nativeModuleRequire;

const fn = compileFunction(id);
fn(this.exports, requireFn, this, process, internalBinding, primordials);

if (experimentalModules && this.canBeRequiredByUsers) {
Copy link
Member Author

@joyeecheung joyeecheung Mar 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that since we no longer hits canBeRequiredByUsers for the internal require, we should be able to remove the immediate dependency of exposeInternals in loaders.js in a future PR, and get the value of that option from C++ later when necessary - only after bootstrap is complete, of course.

this.proxifyExports();
}

this.loaded = true;
} finally {
this.loading = false;
}

moduleLoadList.push(`NativeModule ${id}`);
return this.exports;
};

// This will be passed to internal/bootstrap/node.js.
Expand Down
42 changes: 21 additions & 21 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,12 @@

// This file is compiled as if it's wrapped in a function with arguments
// passed by node::RunBootstrapping()
/* global process, loaderExports, isMainThread, ownsProcessState */
/* global process, require, internalBinding, isMainThread, ownsProcessState */
/* global primordials */

const { internalBinding, NativeModule } = loaderExports;
const { Object, Symbol } = primordials;
const config = internalBinding('config');
const { deprecate } = NativeModule.require('internal/util');
const { deprecate } = require('internal/util');

setupProcessObject();

Expand All @@ -50,17 +49,17 @@ process.domain = null;
process._exiting = false;

// Bootstrappers for all threads, including worker threads and main thread
const perThreadSetup = NativeModule.require('internal/process/per_thread');
const perThreadSetup = require('internal/process/per_thread');
// Bootstrappers for the main thread only
let mainThreadSetup;
// Bootstrappers for the worker threads only
let workerThreadSetup;
if (ownsProcessState) {
mainThreadSetup = NativeModule.require(
mainThreadSetup = require(
'internal/process/main_thread_only'
);
} else {
workerThreadSetup = NativeModule.require(
workerThreadSetup = require(
'internal/process/worker_thread_only'
);
}
Expand Down Expand Up @@ -116,14 +115,18 @@ if (isMainThread) {

const {
emitWarning
} = NativeModule.require('internal/process/warning');
} = require('internal/process/warning');

process.emitWarning = emitWarning;

const {
setupTaskQueue,
queueMicrotask
} = require('internal/process/task_queues');
const {
nextTick,
runNextTicks
} = NativeModule.require('internal/process/task_queues').setupTaskQueue();
} = setupTaskQueue();

process.nextTick = nextTick;
// Used to emulate a tick manually in the JS land.
Expand Down Expand Up @@ -161,7 +164,7 @@ if (credentials.implementsPosixCredentials) {

if (isMainThread) {
const { getStdout, getStdin, getStderr } =
NativeModule.require('internal/process/stdio').getMainThreadStdio();
require('internal/process/stdio').getMainThreadStdio();
setupProcessStdio(getStdout, getStdin, getStderr);
} else {
const { getStdout, getStdin, getStderr } =
Expand All @@ -173,7 +176,7 @@ if (config.hasInspector) {
const {
enable,
disable
} = NativeModule.require('internal/inspector_async_hook');
} = require('internal/inspector_async_hook');
internalBinding('inspector').registerAsyncHook(enable, disable);
}

Expand All @@ -183,30 +186,27 @@ if (!config.noBrowserGlobals) {
// https://console.spec.whatwg.org/#console-namespace
exposeNamespace(global, 'console', createGlobalConsole(global.console));

const { URL, URLSearchParams } = NativeModule.require('internal/url');
const { URL, URLSearchParams } = require('internal/url');
// https://url.spec.whatwg.org/#url
exposeInterface(global, 'URL', URL);
// https://url.spec.whatwg.org/#urlsearchparams
exposeInterface(global, 'URLSearchParams', URLSearchParams);

const {
TextEncoder, TextDecoder
} = NativeModule.require('internal/encoding');
} = require('internal/encoding');
// https://encoding.spec.whatwg.org/#textencoder
exposeInterface(global, 'TextEncoder', TextEncoder);
// https://encoding.spec.whatwg.org/#textdecoder
exposeInterface(global, 'TextDecoder', TextDecoder);

// https://html.spec.whatwg.org/multipage/webappapis.html#windoworworkerglobalscope
const timers = NativeModule.require('timers');
const timers = require('timers');
defineOperation(global, 'clearInterval', timers.clearInterval);
defineOperation(global, 'clearTimeout', timers.clearTimeout);
defineOperation(global, 'setInterval', timers.setInterval);
defineOperation(global, 'setTimeout', timers.setTimeout);

const {
queueMicrotask
} = NativeModule.require('internal/process/task_queues');
defineOperation(global, 'queueMicrotask', queueMicrotask);

// Non-standard extensions:
Expand Down Expand Up @@ -265,7 +265,7 @@ Object.defineProperty(process, 'features', {
fatalException,
setUncaughtExceptionCaptureCallback,
hasUncaughtExceptionCaptureCallback
} = NativeModule.require('internal/process/execution');
} = require('internal/process/execution');

process._fatalException = fatalException;
process.setUncaughtExceptionCaptureCallback =
Expand All @@ -275,7 +275,7 @@ Object.defineProperty(process, 'features', {
}

function setupProcessObject() {
const EventEmitter = NativeModule.require('events');
const EventEmitter = require('events');
const origProcProto = Object.getPrototypeOf(process);
Object.setPrototypeOf(origProcProto, EventEmitter.prototype);
EventEmitter.call(process);
Expand Down Expand Up @@ -359,7 +359,7 @@ function setupGlobalProxy() {
}

function setupBuffer() {
const { Buffer } = NativeModule.require('buffer');
const { Buffer } = require('buffer');
const bufferBinding = internalBinding('buffer');

// Only after this point can C++ use Buffer::New()
Expand All @@ -377,9 +377,9 @@ function setupBuffer() {

function createGlobalConsole(consoleFromVM) {
const consoleFromNode =
NativeModule.require('internal/console/global');
require('internal/console/global');
if (config.hasInspector) {
const inspector = NativeModule.require('internal/util/inspector');
const inspector = require('internal/util/inspector');
// This will be exposed by `require('inspector').console` later.
inspector.consoleFromVM = consoleFromVM;
// TODO(joyeecheung): postpone this until the first time inspector
Expand Down
1 change: 1 addition & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -967,6 +967,7 @@ E('ERR_UNHANDLED_ERROR',
if (err === undefined) return msg;
return `${msg} (${err})`;
}, Error);
E('ERR_UNKNOWN_BUILTIN_MODULE', 'No such built-in module: %s', Error);
E('ERR_UNKNOWN_CREDENTIAL', '%s identifier does not exist: %s', Error);
E('ERR_UNKNOWN_ENCODING', 'Unknown encoding: %s', TypeError);

Expand Down
9 changes: 5 additions & 4 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -568,8 +568,8 @@ Module._resolveLookupPaths = function(request, parent, newReturn) {

// Check the cache for the requested file.
// 1. If a module already exists in the cache: return its exports object.
// 2. If the module is native: call `NativeModule.require()` with the
// filename and return the result.
// 2. If the module is native: call
// `NativeModule.prototype.compileForPublicLoader()` and return the exports.
// 3. Otherwise, create a new module for the file and save it to the cache.
// Then have it load the file contents before returning its exports
// object.
Expand All @@ -586,9 +586,10 @@ Module._load = function(request, parent, isMain) {
return cachedModule.exports;
}

if (NativeModule.canBeRequiredByUsers(filename)) {
const mod = NativeModule.map.get(filename);
if (mod && mod.canBeRequiredByUsers) {
debug('load native module %s', request);
return NativeModule.require(filename);
return mod.compileForPublicLoader(experimentalModules);
}

// Don't call updateChildren(), Module constructor already does.
Expand Down
9 changes: 7 additions & 2 deletions lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ const {
const { URL } = require('url');
const { debuglog, promisify } = require('util');
const esmLoader = require('internal/process/esm_loader');

const {
ERR_UNKNOWN_BUILTIN_MODULE
} = require('internal/errors').codes;
const readFileAsync = promisify(fs.readFile);
const readFileSync = fs.readFileSync;
const StringReplace = FunctionPrototype.call.bind(StringPrototype.replace);
Expand Down Expand Up @@ -85,8 +87,11 @@ translators.set('builtin', async (url) => {
debug(`Translating BuiltinModule ${url}`);
// slice 'node:' scheme
const id = url.slice(5);
NativeModule.require(id);
const module = NativeModule.map.get(id);
if (!module) {
throw new ERR_UNKNOWN_BUILTIN_MODULE(id);
}
module.compileForPublicLoader(true);
return createDynamicModule(
[...module.exportKeys, 'default'], url, (reflect) => {
debug(`Loading BuiltinModule ${url}`);
Expand Down
12 changes: 6 additions & 6 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,6 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
env->process_string(),
FIXED_ONE_BYTE_STRING(isolate, "getLinkedBinding"),
FIXED_ONE_BYTE_STRING(isolate, "getInternalBinding"),
// --experimental-modules
FIXED_ONE_BYTE_STRING(isolate, "experimentalModules"),
// --expose-internals
FIXED_ONE_BYTE_STRING(isolate, "exposeInternals"),
env->primordials_string()};
Expand All @@ -309,7 +307,6 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
env->NewFunctionTemplate(binding::GetInternalBinding)
->GetFunction(context)
.ToLocalChecked(),
Boolean::New(isolate, env->options()->experimental_modules),
Boolean::New(isolate, env->options()->expose_internals),
env->primordials()};

Expand All @@ -331,16 +328,19 @@ MaybeLocal<Value> RunBootstrapping(Environment* env) {
loader_exports_obj->Get(context, env->require_string()).ToLocalChecked();
env->set_native_module_require(require.As<Function>());

// process, loaderExports, isMainThread, ownsProcessState, primordials
// process, require, internalBinding, isMainThread,
// ownsProcessState, primordials
std::vector<Local<String>> node_params = {
env->process_string(),
FIXED_ONE_BYTE_STRING(isolate, "loaderExports"),
env->require_string(),
env->internal_binding_string(),
FIXED_ONE_BYTE_STRING(isolate, "isMainThread"),
FIXED_ONE_BYTE_STRING(isolate, "ownsProcessState"),
env->primordials_string()};
std::vector<Local<Value>> node_args = {
process,
loader_exports_obj,
require,
internal_binding_loader,
Boolean::New(isolate, env->is_main_thread()),
Boolean::New(isolate, env->owns_process_state()),
env->primordials()};
Expand Down