From 06d49c1f10c7d868e13a886da75f8241c6c3ab81 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 6 May 2023 16:51:51 +0200 Subject: [PATCH] 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. PR-URL: https://github.com/nodejs/node/pull/48186 Reviewed-By: James M Snell Reviewed-By: Chengzhong Wu --- lib/internal/bootstrap/realm.js | 15 +++++++------ lib/internal/debugger/inspect_repl.js | 8 ++----- lib/internal/legacy/processbinding.js | 8 +++++++ 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, 43 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..ecf6c7859bc927 100644 --- a/lib/internal/legacy/processbinding.js +++ b/lib/internal/legacy/processbinding.js @@ -33,4 +33,12 @@ module.exports = { ], key); }))); }, + natives() { + const { natives: result, configs } = internalBinding('builtins'); + // Legacy feature: process.binding('natives').config contains stringified + // config.gypi. We do not use this object internally so it's fine to mutate + // it. + 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',