From 3a014dc38aa6a85c8a9ada0e62294e165e9d4367 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 4 Apr 2023 09:13:37 +0800 Subject: [PATCH] src: bootstrap prepare stack trace callback in shadow realm Bootstrap per-realm callbacks like `prepare_stack_trace_callback` in the ShadowRealm. This enables stack trace decoration in the ShadowRealm. PR-URL: https://github.com/nodejs/node/pull/47107 Reviewed-By: Yagiz Nizipli Reviewed-By: Matteo Collina Reviewed-By: Joyee Cheung Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- .github/CODEOWNERS | 2 +- lib/assert.js | 2 +- lib/internal/bootstrap/node.js | 28 ++-------- .../bootstrap/{loaders.js => realm.js} | 35 ++++++++++-- lib/internal/main/mksnapshot.js | 2 +- lib/internal/modules/cjs/loader.js | 2 +- lib/internal/modules/esm/hooks.js | 2 +- lib/internal/modules/esm/resolve.js | 2 +- lib/internal/modules/helpers.js | 2 +- lib/internal/process/pre_execution.js | 2 +- lib/internal/util/inspect.js | 2 +- lib/repl.js | 2 +- src/api/environment.cc | 21 ++++---- src/node_builtins.cc | 12 ++--- src/node_errors.cc | 10 ++-- src/node_realm.cc | 18 +++---- src/node_shadow_realm.cc | 19 ++++--- .../test-loaders-hidden-from-users.js | 6 +-- .../test-inspector-inspect-brk-node.js | 2 +- .../test-shadow-realm-prepare-stack-trace.js | 53 +++++++++++++++++++ 20 files changed, 141 insertions(+), 83 deletions(-) rename lib/internal/bootstrap/{loaders.js => realm.js} (91%) create mode 100644 test/parallel/test-shadow-realm-prepare-stack-trace.js diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 0a7df2ac7d8565..89fe59412773d0 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -84,7 +84,7 @@ /doc/api/module.md @nodejs/modules @nodejs/loaders /doc/api/modules.md @nodejs/modules @nodejs/loaders /doc/api/packages.md @nodejs/modules @nodejs/loaders -/lib/internal/bootstrap/loaders.js @nodejs/modules @nodejs/loaders +/lib/internal/bootstrap/realm.js @nodejs/modules @nodejs/loaders /lib/internal/modules/* @nodejs/modules @nodejs/loaders /lib/internal/process/esm_loader.js @nodejs/modules @nodejs/loaders /lib/internal/process/execution.js @nodejs/modules @nodejs/loaders diff --git a/lib/assert.js b/lib/assert.js index 04c2dd3bfcfdfb..c73e750e337fcb 100644 --- a/lib/assert.js +++ b/lib/assert.js @@ -65,7 +65,7 @@ const { openSync, closeSync, readSync } = require('fs'); const { inspect } = require('internal/util/inspect'); const { isPromise, isRegExp } = require('internal/util/types'); const { EOL } = require('internal/constants'); -const { BuiltinModule } = require('internal/bootstrap/loaders'); +const { BuiltinModule } = require('internal/bootstrap/realm'); const { isError } = require('internal/util'); const errorCache = new SafeMap(); diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 657bad0d3eb7ff..41a833885f9491 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -1,6 +1,6 @@ // Hello, and welcome to hacking node.js! // -// This file is invoked by `Realm::BootstrapNode()` in `src/node_realm.cc`, +// This file is invoked by `Realm::BootstrapRealm()` in `src/node_realm.cc`, // and is responsible for setting up Node.js core before main scripts // under `lib/internal/main/` are executed. // @@ -32,9 +32,10 @@ // `DOMException` class. // - `lib/internal/per_context/messageport.js`: JS-side components of the // `MessagePort` implementation. -// - `lib/internal/bootstrap/loaders.js`: this sets up internal binding and +// - `lib/internal/bootstrap/realm.js`: this sets up internal binding and // module loaders, including `process.binding()`, `process._linkedBinding()`, -// `internalBinding()` and `BuiltinModule`. +// `internalBinding()` and `BuiltinModule`, and per-realm internal states +// and bindings, including `prepare_stack_trace_callback`. // // The initialization done in this script is included in both the main thread // and the worker threads. After this, further initialization is done based @@ -52,8 +53,6 @@ // passed by `BuiltinLoader::CompileAndCall()`. /* global process, require, internalBinding, primordials */ -setupPrepareStackTrace(); - const { FunctionPrototypeCall, JSONParse, @@ -336,25 +335,6 @@ process.emitWarning = emitWarning; // Note: only after this point are the timers effective } -function setupPrepareStackTrace() { - const { - setEnhanceStackForFatalException, - setPrepareStackTraceCallback, - } = internalBinding('errors'); - const { - prepareStackTrace, - fatalExceptionStackEnhancers: { - beforeInspector, - afterInspector, - }, - } = require('internal/errors'); - // Tell our PrepareStackTraceCallback passed to the V8 API - // to call prepareStackTrace(). - setPrepareStackTraceCallback(prepareStackTrace); - // Set the function used to enhance the error stack for printing - setEnhanceStackForFatalException(beforeInspector, afterInspector); -} - function setupProcessObject() { const EventEmitter = require('events'); const origProcProto = ObjectGetPrototypeOf(process); diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/realm.js similarity index 91% rename from lib/internal/bootstrap/loaders.js rename to lib/internal/bootstrap/realm.js index a0a48e6c451dd7..4b03d8247cb4e3 100644 --- a/lib/internal/bootstrap/loaders.js +++ b/lib/internal/bootstrap/realm.js @@ -1,3 +1,8 @@ +// This file is executed in every realm that is created by Node.js, including +// the context of main thread, worker threads, and ShadowRealms. +// Only per-realm internal states and bindings should be bootstrapped in this +// file and no globals should be exposed to the user code. +// // This file creates the internal module & binding loaders used by built-in // modules. In contrast, user land modules are loaded using // lib/internal/modules/cjs/loader.js (CommonJS Modules) or @@ -30,7 +35,7 @@ // so they can be loaded faster without the cost of I/O. This class makes the // lib/internal/*, deps/internal/* modules and internalBinding() available by // default to core modules, and lets the core modules require itself via -// require('internal/bootstrap/loaders') even when this file is not written in +// require('internal/bootstrap/realm') even when this file is not written in // CommonJS style. // // Other objects: @@ -178,7 +183,7 @@ let internalBinding; }; } -const loaderId = 'internal/bootstrap/loaders'; +const selfId = 'internal/bootstrap/realm'; const { builtinIds, compileFunction, @@ -235,7 +240,7 @@ class BuiltinModule { static exposeInternals() { for (const { 0: id, 1: mod } of BuiltinModule.map) { // Do not expose this to user land even with --expose-internals. - if (id !== loaderId) { + if (id !== selfId) { mod.canBeRequiredByUsers = true; } } @@ -354,7 +359,7 @@ const loaderExports = { }; function requireBuiltin(id) { - if (id === loaderId) { + if (id === selfId) { return loaderExports; } @@ -374,5 +379,27 @@ function requireWithFallbackInDeps(request) { return requireBuiltin(request); } +function setupPrepareStackTrace() { + const { + setEnhanceStackForFatalException, + setPrepareStackTraceCallback, + } = internalBinding('errors'); + const { + prepareStackTrace, + fatalExceptionStackEnhancers: { + beforeInspector, + afterInspector, + }, + } = requireBuiltin('internal/errors'); + // Tell our PrepareStackTraceCallback passed to the V8 API + // to call prepareStackTrace(). + setPrepareStackTraceCallback(prepareStackTrace); + // Set the function used to enhance the error stack for printing + setEnhanceStackForFatalException(beforeInspector, afterInspector); +} + // Store the internal loaders in C++. setInternalLoaders(internalBinding, requireBuiltin); + +// Setup per-realm bindings. +setupPrepareStackTrace(); diff --git a/lib/internal/main/mksnapshot.js b/lib/internal/main/mksnapshot.js index f53d3a8f0993aa..47fcc9ac2772a4 100644 --- a/lib/internal/main/mksnapshot.js +++ b/lib/internal/main/mksnapshot.js @@ -10,7 +10,7 @@ const { } = primordials; const binding = internalBinding('mksnapshot'); -const { BuiltinModule } = require('internal/bootstrap/loaders'); +const { BuiltinModule } = require('internal/bootstrap/realm'); const { getEmbedderEntryFunction, compileSerializeMain, diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index a864ee94056bd4..af512195d21c62 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -75,7 +75,7 @@ module.exports = { initializeCJS, }; -const { BuiltinModule } = require('internal/bootstrap/loaders'); +const { BuiltinModule } = require('internal/bootstrap/realm'); const { maybeCacheSourceMap, } = require('internal/source_map/source_map_cache'); diff --git a/lib/internal/modules/esm/hooks.js b/lib/internal/modules/esm/hooks.js index 9c5c4c99b887d0..0597aaa79d9ef1 100644 --- a/lib/internal/modules/esm/hooks.js +++ b/lib/internal/modules/esm/hooks.js @@ -190,7 +190,7 @@ class Hooks { filename: '', }, ); - const { BuiltinModule } = require('internal/bootstrap/loaders'); + const { BuiltinModule } = require('internal/bootstrap/realm'); // We only allow replacing the importMetaInitializer during preload; // after preload is finished, we disable the ability to replace it. // diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index d04c04e8306c14..58373230da3587 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -24,7 +24,7 @@ const { StringPrototypeStartsWith, } = primordials; const internalFS = require('internal/fs/utils'); -const { BuiltinModule } = require('internal/bootstrap/loaders'); +const { BuiltinModule } = require('internal/bootstrap/realm'); const { realpathSync, statSync, diff --git a/lib/internal/modules/helpers.js b/lib/internal/modules/helpers.js index 5dc23f13e7cc3a..d20ef5e90f3f39 100644 --- a/lib/internal/modules/helpers.js +++ b/lib/internal/modules/helpers.js @@ -17,7 +17,7 @@ const { ERR_MANIFEST_DEPENDENCY_MISSING, ERR_UNKNOWN_BUILTIN_MODULE, } = require('internal/errors').codes; -const { BuiltinModule } = require('internal/bootstrap/loaders'); +const { BuiltinModule } = require('internal/bootstrap/realm'); const { validateString } = require('internal/validators'); const path = require('path'); diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js index e292710567b0dc..53cda5737f07c0 100644 --- a/lib/internal/process/pre_execution.js +++ b/lib/internal/process/pre_execution.js @@ -351,7 +351,7 @@ function initializeReport() { function setupDebugEnv() { require('internal/util/debuglog').initializeDebugEnv(process.env.NODE_DEBUG); if (getOptionValue('--expose-internals')) { - require('internal/bootstrap/loaders').BuiltinModule.exposeInternals(); + require('internal/bootstrap/realm').BuiltinModule.exposeInternals(); } } diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js index 57ffa96c97da40..0a7d6daf4e796e 100644 --- a/lib/internal/util/inspect.js +++ b/lib/internal/util/inspect.js @@ -150,7 +150,7 @@ const { const assert = require('internal/assert'); -const { BuiltinModule } = require('internal/bootstrap/loaders'); +const { BuiltinModule } = require('internal/bootstrap/realm'); const { validateObject, validateString, diff --git a/lib/repl.js b/lib/repl.js index 3240a66101474d..0a5a9b44ed16b8 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -96,7 +96,7 @@ const { globalThis, } = primordials; -const { BuiltinModule } = require('internal/bootstrap/loaders'); +const { BuiltinModule } = require('internal/bootstrap/realm'); const { makeRequireFunction, addBuiltinLibsToObject, diff --git a/src/api/environment.cc b/src/api/environment.cc index fd187f73423231..c9bb4bdf1645e2 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -67,17 +67,18 @@ MaybeLocal PrepareStackTraceCallback(Local context, if (env == nullptr) { return exception->ToString(context).FromMaybe(Local()); } - // TODO(legendecas): Per-realm prepareStackTrace callback. - // If we are in a Realm that is not the principal Realm (e.g. ShadowRealm), - // skip the prepareStackTrace callback to avoid passing the JS objects ( - // the exception and trace) across the realm boundary with the - // `Error.prepareStackTrace` override. - Realm* current_realm = Realm::GetCurrent(context); - if (current_realm != nullptr && - current_realm->kind() != Realm::Kind::kPrincipal) { - return exception->ToString(context).FromMaybe(Local()); + Realm* realm = Realm::GetCurrent(context); + Local prepare; + if (realm != nullptr) { + // If we are in a Realm, call the realm specific prepareStackTrace callback + // to avoid passing the JS objects (the exception and trace) across the + // realm boundary with the `Error.prepareStackTrace` override. + prepare = realm->prepare_stack_trace_callback(); + } else { + // The context is created with ContextifyContext, call the principal + // realm's prepareStackTrace callback. + prepare = env->principal_realm()->prepare_stack_trace_callback(); } - Local prepare = env->prepare_stack_trace_callback(); if (prepare.IsEmpty()) { return exception->ToString(context).FromMaybe(Local()); } diff --git a/src/node_builtins.cc b/src/node_builtins.cc index c3f6d61f20efc9..776ac8f2358aca 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -356,9 +356,9 @@ MaybeLocal BuiltinLoader::LookupAndCompile(Local context, std::vector> parameters; Isolate* isolate = context->GetIsolate(); // Detects parameters of the scripts based on module ids. - // internal/bootstrap/loaders: process, getLinkedBinding, - // getInternalBinding, primordials - if (strcmp(id, "internal/bootstrap/loaders") == 0) { + // internal/bootstrap/realm: process, getLinkedBinding, + // getInternalBinding, primordials + if (strcmp(id, "internal/bootstrap/realm") == 0) { parameters = { FIXED_ONE_BYTE_STRING(isolate, "process"), FIXED_ONE_BYTE_STRING(isolate, "getLinkedBinding"), @@ -414,9 +414,9 @@ MaybeLocal BuiltinLoader::CompileAndCall(Local context, // BuiltinLoader::LookupAndCompile(). std::vector> arguments; // Detects parameters of the scripts based on module ids. - // internal/bootstrap/loaders: process, getLinkedBinding, - // getInternalBinding, primordials - if (strcmp(id, "internal/bootstrap/loaders") == 0) { + // internal/bootstrap/realm: process, getLinkedBinding, + // getInternalBinding, primordials + if (strcmp(id, "internal/bootstrap/realm") == 0) { Local get_linked_binding; Local get_internal_binding; if (!NewFunctionTemplate(isolate, binding::GetLinkedBinding) diff --git a/src/node_errors.cc b/src/node_errors.cc index ec9ae0f13eac0a..bc6348bbb41f76 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -960,9 +960,9 @@ void PerIsolateMessageListener(Local message, Local error) { } void SetPrepareStackTraceCallback(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); + Realm* realm = Realm::GetCurrent(args); CHECK(args[0]->IsFunction()); - env->set_prepare_stack_trace_callback(args[0].As()); + realm->set_prepare_stack_trace_callback(args[0].As()); } static void SetSourceMapsEnabled(const FunctionCallbackInfo& args) { @@ -987,11 +987,11 @@ static void SetMaybeCacheGeneratedSourceMap( static void SetEnhanceStackForFatalException( const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); + Realm* realm = Realm::GetCurrent(args); CHECK(args[0]->IsFunction()); CHECK(args[1]->IsFunction()); - env->set_enhance_fatal_stack_before_inspector(args[0].As()); - env->set_enhance_fatal_stack_after_inspector(args[1].As()); + realm->set_enhance_fatal_stack_before_inspector(args[0].As()); + realm->set_enhance_fatal_stack_after_inspector(args[1].As()); } // Side effect-free stringification that will never throw exceptions. diff --git a/src/node_realm.cc b/src/node_realm.cc index 6a0876fa6dc87c..3313c683e7e424 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -179,7 +179,7 @@ MaybeLocal Realm::RunBootstrapping() { CHECK(!has_run_bootstrapping_code()); Local result; - if (!ExecuteBootstrapper("internal/bootstrap/loaders").ToLocal(&result) || + if (!ExecuteBootstrapper("internal/bootstrap/realm").ToLocal(&result) || !BootstrapRealm().ToLocal(&result)) { return MaybeLocal(); } @@ -306,11 +306,9 @@ void PrincipalRealm::MemoryInfo(MemoryTracker* tracker) const { } MaybeLocal PrincipalRealm::BootstrapRealm() { - EscapableHandleScope scope(isolate_); - - MaybeLocal result = ExecuteBootstrapper("internal/bootstrap/node"); + HandleScope scope(isolate_); - if (result.IsEmpty()) { + if (ExecuteBootstrapper("internal/bootstrap/node").IsEmpty()) { return MaybeLocal(); } @@ -327,9 +325,7 @@ MaybeLocal PrincipalRealm::BootstrapRealm() { auto thread_switch_id = env_->is_main_thread() ? "internal/bootstrap/switches/is_main_thread" : "internal/bootstrap/switches/is_not_main_thread"; - result = ExecuteBootstrapper(thread_switch_id); - - if (result.IsEmpty()) { + if (ExecuteBootstrapper(thread_switch_id).IsEmpty()) { return MaybeLocal(); } @@ -337,9 +333,7 @@ MaybeLocal PrincipalRealm::BootstrapRealm() { env_->owns_process_state() ? "internal/bootstrap/switches/does_own_process_state" : "internal/bootstrap/switches/does_not_own_process_state"; - result = ExecuteBootstrapper(process_state_switch_id); - - if (result.IsEmpty()) { + if (ExecuteBootstrapper(process_state_switch_id).IsEmpty()) { return MaybeLocal(); } @@ -351,7 +345,7 @@ MaybeLocal PrincipalRealm::BootstrapRealm() { return MaybeLocal(); } - return scope.EscapeMaybe(result); + return v8::True(isolate_); } } // namespace node diff --git a/src/node_shadow_realm.cc b/src/node_shadow_realm.cc index bf5c7c94676c55..bf39509d537757 100644 --- a/src/node_shadow_realm.cc +++ b/src/node_shadow_realm.cc @@ -1,20 +1,25 @@ #include "node_shadow_realm.h" #include "env-inl.h" +#include "node_errors.h" namespace node { namespace shadow_realm { using v8::Context; -using v8::EscapableHandleScope; using v8::HandleScope; using v8::Local; using v8::MaybeLocal; using v8::Value; +using TryCatchScope = node::errors::TryCatchScope; + // static ShadowRealm* ShadowRealm::New(Environment* env) { ShadowRealm* realm = new ShadowRealm(env); env->AssignToContext(realm->context(), realm, ContextInfo("")); + // We do not expect the realm bootstrapping to throw any + // exceptions. If it does, exit the current Node.js instance. + TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal); if (realm->RunBootstrapping().IsEmpty()) { delete realm; return nullptr; @@ -91,21 +96,19 @@ PER_REALM_STRONG_PERSISTENT_VALUES(V) #undef V v8::MaybeLocal ShadowRealm::BootstrapRealm() { - EscapableHandleScope scope(isolate_); - MaybeLocal result = v8::True(isolate_); + HandleScope scope(isolate_); // Skip "internal/bootstrap/node" as it installs node globals and per-isolate - // callbacks like PrepareStackTraceCallback. + // callbacks. if (!env_->no_browser_globals()) { - result = ExecuteBootstrapper("internal/bootstrap/web/exposed-wildcard"); - - if (result.IsEmpty()) { + if (ExecuteBootstrapper("internal/bootstrap/web/exposed-wildcard") + .IsEmpty()) { return MaybeLocal(); } } - return result; + return v8::True(isolate_); } } // namespace shadow_realm diff --git a/test/es-module/test-loaders-hidden-from-users.js b/test/es-module/test-loaders-hidden-from-users.js index 96d1c44154883e..59e910ad751dbe 100644 --- a/test/es-module/test-loaders-hidden-from-users.js +++ b/test/es-module/test-loaders-hidden-from-users.js @@ -7,16 +7,16 @@ const assert = require('assert'); assert.throws( () => { - require('internal/bootstrap/loaders'); + require('internal/bootstrap/realm'); }, { code: 'MODULE_NOT_FOUND', - message: /Cannot find module 'internal\/bootstrap\/loaders'/ + message: /Cannot find module 'internal\/bootstrap\/realm'/ } ); assert.throws( () => { - const source = 'module.exports = require("internal/bootstrap/loaders")'; + const source = 'module.exports = require("internal/bootstrap/realm")'; const { internalBinding } = require('internal/test/binding'); internalBinding('natives').owo = source; require('owo'); diff --git a/test/parallel/test-inspector-inspect-brk-node.js b/test/parallel/test-inspector-inspect-brk-node.js index 0f4795ed7b87e4..04bac6dc7cf0df 100644 --- a/test/parallel/test-inspector-inspect-brk-node.js +++ b/test/parallel/test-inspector-inspect-brk-node.js @@ -16,7 +16,7 @@ async function runTest() { await session.waitForNotification((notification) => { // The main assertion here is that we do hit the loader script first. return notification.method === 'Debugger.scriptParsed' && - notification.params.url === 'node:internal/bootstrap/loaders'; + notification.params.url === 'node:internal/bootstrap/realm'; }); await session.waitForNotification('Debugger.paused'); diff --git a/test/parallel/test-shadow-realm-prepare-stack-trace.js b/test/parallel/test-shadow-realm-prepare-stack-trace.js new file mode 100644 index 00000000000000..2460f017ee91b3 --- /dev/null +++ b/test/parallel/test-shadow-realm-prepare-stack-trace.js @@ -0,0 +1,53 @@ +// Flags: --experimental-shadow-realm +'use strict'; + +require('../common'); +const assert = require('assert'); + +let principalRealmPrepareStackTraceCalled = false; +Error.prepareStackTrace = (error, trace) => { + principalRealmPrepareStackTraceCalled = true; + return `${String(error)}\n at ${trace.join('\n at ')}`; +}; + +{ + // Validates inner Error.prepareStackTrace can not leak into the outer realm. + const shadowRealm = new ShadowRealm(); + + const stack = shadowRealm.evaluate(` +Error.prepareStackTrace = (error, trace) => { + globalThis.leaked = 'inner'; + return 'from shadow realm'; +}; + +try { + throw new Error('boom'); +} catch (e) { + e.stack; +} +`); + assert.ok(!principalRealmPrepareStackTraceCalled); + assert.strictEqual(stack, 'from shadow realm'); + assert.strictEqual('leaked' in globalThis, false); +} + +{ + // Validates stacks can be generated in the ShadowRealm. + const shadowRealm = new ShadowRealm(); + + const stack = shadowRealm.evaluate(` +function myFunc() { + throw new Error('boom'); +} + +try { + myFunc(); +} catch (e) { + e.stack; +} +`); + assert.ok(!principalRealmPrepareStackTraceCalled); + const lines = stack.split('\n'); + assert.strictEqual(lines[0], 'Error: boom'); + assert.match(lines[1], /^ {4}at myFunc \(.*\)/); +}