From f83abced73817951a97e984202cec6f6b1d97d5c Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 6 May 2023 16:51:51 +0200 Subject: [PATCH 1/3] src: implement natives binding without special casing This patch removes special case in the internal binding loader for natives, and implements it using the builtins internal binding. Internally we do not actually need the natives binding, so implement it as a legacy wrapper instead. --- lib/internal/bootstrap/realm.js | 15 +++++++------ lib/internal/debugger/inspect_repl.js | 8 ++----- lib/internal/legacy/processbinding.js | 7 +++++++ lib/internal/v8_prof_processor.js | 2 +- src/node_binding.cc | 9 -------- src/node_builtins.cc | 21 +++++++++++++++---- src/node_builtins.h | 4 +++- .../test-loaders-hidden-from-users.js | 5 +++-- 8 files changed, 42 insertions(+), 29 deletions(-) diff --git a/lib/internal/bootstrap/realm.js b/lib/internal/bootstrap/realm.js index 8ea6b6ed5f45f0..608e3072850d45 100644 --- a/lib/internal/bootstrap/realm.js +++ b/lib/internal/bootstrap/realm.js @@ -78,11 +78,13 @@ ObjectDefineProperty(process, 'moduleLoadList', { }); -// internalBindingAllowlist contains the name of internalBinding modules -// that are allowed for access via process.binding()... This is used -// to provide a transition path for modules that are being moved over to -// internalBinding. -const internalBindingAllowlist = new SafeSet([ +// processBindingAllowList contains the name of bindings that are allowed +// for access via process.binding(). This is used to provide a transition +// path for modules that are being moved over to internalBinding. +// Certain bindings may not actually correspond to an internalBinding any +// more, we just implement them as legacy wrappers instead. See the +// legacyWrapperList. +const processBindingAllowList = new SafeSet([ 'async_wrap', 'buffer', 'cares_wrap', @@ -124,6 +126,7 @@ const runtimeDeprecatedList = new SafeSet([ ]); const legacyWrapperList = new SafeSet([ + 'natives', 'util', ]); @@ -145,7 +148,7 @@ const experimentalModuleList = new SafeSet(); module = String(module); // Deprecated specific process.binding() modules, but not all, allow // selective fallback to internalBinding for the deprecated ones. - if (internalBindingAllowlist.has(module)) { + if (processBindingAllowList.has(module)) { if (runtimeDeprecatedList.has(module)) { runtimeDeprecatedList.delete(module); process.emitWarning( diff --git a/lib/internal/debugger/inspect_repl.js b/lib/internal/debugger/inspect_repl.js index 7fba613f09c632..b4f454152dc438 100644 --- a/lib/internal/debugger/inspect_repl.js +++ b/lib/internal/debugger/inspect_repl.js @@ -118,16 +118,12 @@ function extractFunctionName(description) { return fnNameMatch ? `: ${fnNameMatch[1]}` : ''; } -const { - builtinIds: PUBLIC_BUILTINS, -} = internalBinding('builtins'); -const NATIVES = internalBinding('natives'); +const { builtinIds } = internalBinding('builtins'); function isNativeUrl(url) { url = SideEffectFreeRegExpPrototypeSymbolReplace(/\.js$/, url, ''); return StringPrototypeStartsWith(url, 'node:internal/') || - ArrayPrototypeIncludes(PUBLIC_BUILTINS, url) || - url in NATIVES || url === 'bootstrap_node'; + ArrayPrototypeIncludes(builtinIds, url); } function getRelativePath(filenameOrURL) { diff --git a/lib/internal/legacy/processbinding.js b/lib/internal/legacy/processbinding.js index 4b15dd1c12df1c..c93e540278d142 100644 --- a/lib/internal/legacy/processbinding.js +++ b/lib/internal/legacy/processbinding.js @@ -33,4 +33,11 @@ module.exports = { ], key); }))); }, + natives() { + const { natives: result, configs } = internalBinding('builtins'); + // Legacy feature: process.binding('natives').config contains stringified + // config.gypi + result.configs = configs; + return result; + }, }; diff --git a/lib/internal/v8_prof_processor.js b/lib/internal/v8_prof_processor.js index 4ed6fe8b9b3902..1d4f33418ff733 100644 --- a/lib/internal/v8_prof_processor.js +++ b/lib/internal/v8_prof_processor.js @@ -12,7 +12,7 @@ const console = require('internal/console/global'); const vm = require('vm'); const { SourceTextModule } = require('internal/vm/module'); -const natives = internalBinding('natives'); +const { natives } = internalBinding('builtins'); async function linker(specifier, referencingModule) { // Transform "./file.mjs" to "file" diff --git a/src/node_binding.cc b/src/node_binding.cc index e365bf1dbac5b4..1d098eedbfbd66 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -635,15 +635,6 @@ void GetInternalBinding(const FunctionCallbackInfo& args) { exports = Object::New(isolate); CHECK(exports->SetPrototype(context, Null(isolate)).FromJust()); DefineConstants(isolate, exports); - } else if (!strcmp(*module_v, "natives")) { - exports = realm->env()->builtin_loader()->GetSourceObject(context); - // Legacy feature: process.binding('natives').config contains stringified - // config.gypi - CHECK(exports - ->Set(context, - realm->isolate_data()->config_string(), - realm->env()->builtin_loader()->GetConfigString(isolate)) - .FromJust()); } else { return THROW_ERR_INVALID_MODULE(isolate, "No such binding: %s", *module_v); } diff --git a/src/node_builtins.cc b/src/node_builtins.cc index 8673142d1e5dfd..da0dab568fcf47 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -63,15 +63,19 @@ bool BuiltinLoader::Add(const char* id, const UnionBytes& source) { return result.second; } -Local BuiltinLoader::GetSourceObject(Local context) { - Isolate* isolate = context->GetIsolate(); +void BuiltinLoader::GetNatives(Local property, + const PropertyCallbackInfo& info) { + Environment* env = Environment::GetCurrent(info); + Isolate* isolate = env->isolate(); + Local context = env->context(); + Local out = Object::New(isolate); - auto source = source_.read(); + auto source = env->builtin_loader()->source_.read(); for (auto const& x : *source) { Local key = OneByteString(isolate, x.first.c_str(), x.first.size()); out->Set(context, key, x.second.ToStringChecked(isolate)).FromJust(); } - return out; + info.GetReturnValue().Set(out); } Local BuiltinLoader::GetConfigString(Isolate* isolate) { @@ -689,6 +693,14 @@ void BuiltinLoader::CreatePerIsolateProperties(IsolateData* isolate_data, None, SideEffectType::kHasNoSideEffect); + target->SetAccessor(FIXED_ONE_BYTE_STRING(isolate, "natives"), + GetNatives, + nullptr, + Local(), + DEFAULT, + None, + SideEffectType::kHasNoSideEffect); + SetMethod(isolate, target, "getCacheUsage", BuiltinLoader::GetCacheUsage); SetMethod(isolate, target, "compileFunction", BuiltinLoader::CompileFunction); SetMethod(isolate, target, "hasCachedBuiltins", HasCachedBuiltins); @@ -712,6 +724,7 @@ void BuiltinLoader::RegisterExternalReferences( registry->Register(CompileFunction); registry->Register(HasCachedBuiltins); registry->Register(SetInternalLoaders); + registry->Register(GetNatives); RegisterExternalReferencesForInternalizedBuiltinCode(registry); } diff --git a/src/node_builtins.h b/src/node_builtins.h index 3cfdf552a31b9a..5e634254fc6560 100644 --- a/src/node_builtins.h +++ b/src/node_builtins.h @@ -107,7 +107,6 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { const char* id, Realm* realm); - v8::Local GetSourceObject(v8::Local context); // Returns config.gypi as a JSON string v8::Local GetConfigString(v8::Isolate* isolate); bool Exists(const char* id); @@ -169,6 +168,9 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { static void CompileFunction(const v8::FunctionCallbackInfo& args); static void HasCachedBuiltins( const v8::FunctionCallbackInfo& args); + // For legacy process.binding('natives') + static void GetNatives(v8::Local property, + const v8::PropertyCallbackInfo& info); void AddExternalizedBuiltin(const char* id, const char* filename); diff --git a/test/es-module/test-loaders-hidden-from-users.js b/test/es-module/test-loaders-hidden-from-users.js index 59e910ad751dbe..e1b5518e90cbfe 100644 --- a/test/es-module/test-loaders-hidden-from-users.js +++ b/test/es-module/test-loaders-hidden-from-users.js @@ -17,8 +17,9 @@ assert.throws( assert.throws( () => { const source = 'module.exports = require("internal/bootstrap/realm")'; - const { internalBinding } = require('internal/test/binding'); - internalBinding('natives').owo = source; + // This needs to be process.binding() to mimic what's normally available + // in the user land. + process.binding('natives').owo = source; require('owo'); }, { code: 'MODULE_NOT_FOUND', From 6f7a2534286f9355b48acac1226092cdb186fe99 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 6 May 2023 17:01:34 +0200 Subject: [PATCH 2/3] src: implement constants binding directly Instead of adding a special case for it in the internal binding loader, just implement it as usual using a per-context property initializer. --- src/node_binding.cc | 6 +----- src/node_constants.cc | 20 ++++++++++++++++---- src/node_constants.h | 5 ----- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/node_binding.cc b/src/node_binding.cc index 1d098eedbfbd66..97257d47c61738 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -34,6 +34,7 @@ V(builtins) \ V(cares_wrap) \ V(config) \ + V(constants) \ V(contextify) \ V(credentials) \ V(encoding_binding) \ @@ -619,7 +620,6 @@ void GetInternalBinding(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); Isolate* isolate = realm->isolate(); HandleScope scope(isolate); - Local context = realm->context(); CHECK(args[0]->IsString()); @@ -631,10 +631,6 @@ void GetInternalBinding(const FunctionCallbackInfo& args) { if (mod != nullptr) { exports = InitInternalBinding(realm, mod); realm->internal_bindings.insert(mod); - } else if (!strcmp(*module_v, "constants")) { - exports = Object::New(isolate); - CHECK(exports->SetPrototype(context, Null(isolate)).FromJust()); - DefineConstants(isolate, exports); } else { return THROW_ERR_INVALID_MODULE(isolate, "No such binding: %s", *module_v); } diff --git a/src/node_constants.cc b/src/node_constants.cc index 68b457fcd42aaa..05582b28dafd9f 100644 --- a/src/node_constants.cc +++ b/src/node_constants.cc @@ -63,10 +63,14 @@ namespace node { +using v8::Context; +using v8::Isolate; using v8::Local; +using v8::Null; using v8::Object; +using v8::Value; -namespace { +namespace constants { void DefineErrnoConstants(Local target) { #ifdef E2BIG @@ -1270,10 +1274,14 @@ void DefineTraceConstants(Local target) { NODE_DEFINE_CONSTANT(target, TRACE_EVENT_PHASE_LINK_IDS); } -} // anonymous namespace +void CreatePerContextProperties(Local target, + Local unused, + Local context, + void* priv) { + Isolate* isolate = context->GetIsolate(); + Environment* env = Environment::GetCurrent(context); -void DefineConstants(v8::Isolate* isolate, Local target) { - Environment* env = Environment::GetCurrent(isolate); + CHECK(target->SetPrototype(env->context(), Null(env->isolate())).FromJust()); Local os_constants = Object::New(isolate); CHECK(os_constants->SetPrototype(env->context(), @@ -1353,4 +1361,8 @@ void DefineConstants(v8::Isolate* isolate, Local target) { trace_constants).Check(); } +} // namespace constants } // namespace node + +NODE_BINDING_CONTEXT_AWARE_INTERNAL(constants, + node::constants::CreatePerContextProperties) diff --git a/src/node_constants.h b/src/node_constants.h index d7de705fb8ec7e..08f94bd623aa60 100644 --- a/src/node_constants.h +++ b/src/node_constants.h @@ -74,11 +74,6 @@ #endif // NODE_OPENSSL_DEFAULT_CIPHER_LIST #endif // HAVE_OPENSSL -namespace node { - -void DefineConstants(v8::Isolate* isolate, v8::Local target); -} // namespace node - #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #endif // SRC_NODE_CONSTANTS_H_ From 8461ac15fb00fa5dc71ea0099b055da12b5ff037 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 1 Jun 2023 17:31:18 +0200 Subject: [PATCH 3/3] fixup! src: implement natives binding without special casing --- lib/internal/legacy/processbinding.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/internal/legacy/processbinding.js b/lib/internal/legacy/processbinding.js index c93e540278d142..ecf6c7859bc927 100644 --- a/lib/internal/legacy/processbinding.js +++ b/lib/internal/legacy/processbinding.js @@ -36,7 +36,8 @@ module.exports = { natives() { const { natives: result, configs } = internalBinding('builtins'); // Legacy feature: process.binding('natives').config contains stringified - // config.gypi + // config.gypi. We do not use this object internally so it's fine to mutate + // it. result.configs = configs; return result; },