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: #48186 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
nodejs · Jul 3, 2023 · 06d49c1 · 06d49c1
1 parent a2e107d
commit 06d49c1
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 29 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -635,15 +635,6 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& 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
@@ -63,15 +63,19 @@ bool BuiltinLoader::Add(const char* id, const UnionBytes& source) {
   return result.second;
 }
 
-Local<Object> BuiltinLoader::GetSourceObject(Local<Context> context) {
-  Isolate* isolate = context->GetIsolate();
+void BuiltinLoader::GetNatives(Local<Name> property,
+                               const PropertyCallbackInfo<Value>& info) {
+  Environment* env = Environment::GetCurrent(info);
+  Isolate* isolate = env->isolate();
+  Local<Context> context = env->context();
+
   Local<Object> out = Object::New(isolate);
-  auto source = source_.read();
+  auto source = env->builtin_loader()->source_.read();
   for (auto const& x : *source) {
     Local<String> 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<String> 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<Value>(),
+                      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
@@ -107,7 +107,6 @@ class NODE_EXTERN_PRIVATE BuiltinLoader {
                                            const char* id,
                                            Realm* realm);
 
-  v8::Local<v8::Object> GetSourceObject(v8::Local<v8::Context> context);
   // Returns config.gypi as a JSON string
   v8::Local<v8::String> GetConfigString(v8::Isolate* isolate);
   bool Exists(const char* id);
@@ -169,6 +168,9 @@ class NODE_EXTERN_PRIVATE BuiltinLoader {
   static void CompileFunction(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void HasCachedBuiltins(
       const v8::FunctionCallbackInfo<v8::Value>& args);
+  // For legacy process.binding('natives')
+  static void GetNatives(v8::Local<v8::Name> property,
+                         const v8::PropertyCallbackInfo<v8::Value>& 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
@@ -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',