module: use symbol in WeakMap to manage host defined options

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in nodejs#46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
joyeecheung · Sep 26, 2023 · 7aae66a · 7aae66a
1 parent 92ba261
commit 7aae66a
Show file tree

Hide file tree

Showing 16 changed files with 185 additions and 188 deletions.
diff --git a/lib/internal/modules/esm/create_dynamic_module.js b/lib/internal/modules/esm/create_dynamic_module.js
@@ -45,8 +45,9 @@ import.meta.done();
 
   if (imports.length)
     reflect.imports = { __proto__: null };
-  const { setCallbackForWrap } = require('internal/modules/esm/utils');
-  setCallbackForWrap(m, {
+  const { registerModule } = require('internal/modules/esm/utils');
+  registerModule(m, {
+    __proto__: null,
     initializeImportMeta: (meta, wrap) => {
       meta.exports = reflect.exports;
       if (reflect.imports)

diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -180,9 +180,10 @@ class ModuleLoader {
   ) {
     const evalInstance = (url) => {
       const { ModuleWrap } = internalBinding('module_wrap');
-      const { setCallbackForWrap } = require('internal/modules/esm/utils');
+      const { registerModule } = require('internal/modules/esm/utils');
       const module = new ModuleWrap(url, undefined, source, 0, 0);
-      setCallbackForWrap(module, {
+      registerModule(module, {
+        __proto__: null,
         initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }),
         importModuleDynamically: (specifier, { url }, importAssertions) => {
           return this.import(specifier, url, importAssertions);

diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
@@ -116,8 +116,9 @@ translators.set('module', async function moduleStrategy(url, source, isMain) {
   maybeCacheSourceMap(url, source);
   debug(`Translating StandardModule ${url}`);
   const module = new ModuleWrap(url, undefined, source, 0, 0);
-  const { setCallbackForWrap } = require('internal/modules/esm/utils');
-  setCallbackForWrap(module, {
+  const { registerModule } = require('internal/modules/esm/utils');
+  registerModule(module, {
+    __proto__: null,
     initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }),
     importModuleDynamically,
   });

diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js
@@ -7,6 +7,11 @@ const {
   ObjectFreeze,
 } = primordials;
 
+const {
+  privateSymbols: {
+    host_defined_option_symbol,
+  },
+} = internalBinding('util');
 const {
   ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING,
   ERR_INVALID_ARG_VALUE,
@@ -21,16 +26,8 @@ const {
   setImportModuleDynamicallyCallback,
   setInitializeImportMetaObjectCallback,
 } = internalBinding('module_wrap');
-const {
-  getModuleFromWrap,
-} = require('internal/vm/module');
 const assert = require('internal/assert');
 
-const callbackMap = new SafeWeakMap();
-function setCallbackForWrap(wrap, data) {
-  callbackMap.set(wrap, data);
-}
-
 let defaultConditions;
 function getDefaultConditions() {
   assert(defaultConditions !== undefined);
@@ -73,21 +70,75 @@ function getConditionsSet(conditions) {
   return getDefaultConditionsSet();
 }
 
-function initializeImportMetaObject(wrap, meta) {
-  if (callbackMap.has(wrap)) {
-    const { initializeImportMeta } = callbackMap.get(wrap);
+/**
+ * @callback ImportModuleDynamicallyCallback
+ * @param {string} specifier
+ * @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer
+ * @param {object} assertions
+ * @returns { Promise<void> }
+ */
+
+/**
+ * @callback InitializeImportMetaCallback
+ * @param {object} meta
+ * @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer
+ */
+
+/**
+ * @typedef {{
+ *   callbackReferrer: ModuleWrap|ContextifyScript|Function|vm.Module
+ *   initializeImportMeta? : InitializeImportMetaCallback,
+ *   importModuleDynamically? : ImportModuleDynamicallyCallback
+ * }} ModuleRegistry
+ */
+
+/**
+ * @type {WeakMap<symbol, ModuleRegistry>}
+ */
+const moduleRegistries = new SafeWeakMap();
+
+/**
+ * V8 would make sure that as long as import() can still be initiated from
+ * the referrer, the symbol referenced by |host_defined_option_symbol| should
+ * be alive, which in term would keep the settings object alive through the
+ * WeakMap, and in turn that keeps the referrer object alive, which would be
+ * passed into the callbacks.
+ * The reference goes like this:
+ * [v8::internal::Script] (via host defined options) ----1--> [idSymbol]
+ * [callbackReferrer] (via host_defined_option_symbol) ------2------^  |
+ *                                 ^----------3---- (via WeakMap)------
+ * 1+3 makes sure that as long as import() can still be initiated, the
+ * referrer wrap is still around and can be passed into the callbacks.
+ * 2 is only there so that we can get the id symbol to configure the
+ * weak map.
+ * @param {ModuleWrap|ContextifyScript|Function} referrer The referrer to
+ *   get the id symbol from. This is different from callbackReferrer which
+ *   could be set by the caller.
+ * @param {ModuleRegistry} registry
+ */
+function registerModule(referrer, registry) {
+  const idSymbol = referrer[host_defined_option_symbol];
+  // To prevent it from being GC'ed.
+  registry.callbackReferrer ??= referrer;
+  moduleRegistries.set(idSymbol, registry);
+}
+
+// The native callback
+function initializeImportMetaObject(symbol, meta) {
+  if (moduleRegistries.has(symbol)) {
+    const { initializeImportMeta, callbackReferrer } = moduleRegistries.get(symbol);
     if (initializeImportMeta !== undefined) {
-      meta = initializeImportMeta(meta, getModuleFromWrap(wrap) || wrap);
+      meta = initializeImportMeta(meta, callbackReferrer);
     }
   }
 }
 
-async function importModuleDynamicallyCallback(wrap, specifier, assertions) {
-  if (callbackMap.has(wrap)) {
-    const { importModuleDynamically } = callbackMap.get(wrap);
+// The native callback
+async function importModuleDynamicallyCallback(symbol, specifier, assertions) {
+  if (moduleRegistries.has(symbol)) {
+    const { importModuleDynamically, callbackReferrer } = moduleRegistries.get(symbol);
     if (importModuleDynamically !== undefined) {
-      return importModuleDynamically(
-        specifier, getModuleFromWrap(wrap) || wrap, assertions);
+      return importModuleDynamically(specifier, callbackReferrer, assertions);
     }
   }
   throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING();
@@ -151,7 +202,7 @@ async function initializeHooks() {
 }
 
 module.exports = {
-  setCallbackForWrap,
+  registerModule,
   initializeESM,
   initializeHooks,
   getDefaultConditions,

diff --git a/lib/internal/vm.js b/lib/internal/vm.js
@@ -100,9 +100,10 @@ function internalCompileFunction(code, params, options) {
     const { importModuleDynamicallyWrap } = require('internal/vm/module');
     const wrapped = importModuleDynamicallyWrap(importModuleDynamically);
     const func = result.function;
-    const { setCallbackForWrap } = require('internal/modules/esm/utils');
-    setCallbackForWrap(result.cacheKey, {
-      importModuleDynamically: (s, _k, i) => wrapped(s, func, i),
+    const { registerModule } = require('internal/modules/esm/utils');
+    registerModule(func, {
+      __proto__: null,
+      importModuleDynamically: wrapped,
     });
   }
 

diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js
@@ -11,7 +11,6 @@ const {
   ObjectSetPrototypeOf,
   ReflectApply,
   SafePromiseAllReturnVoid,
-  SafeWeakMap,
   Symbol,
   SymbolToStringTag,
   TypeError,
@@ -69,7 +68,6 @@ const STATUS_MAP = {
 
 let globalModuleId = 0;
 const defaultModuleName = 'vm:module';
-const wrapToModuleMap = new SafeWeakMap();
 
 const kWrap = Symbol('kWrap');
 const kContext = Symbol('kContext');
@@ -120,25 +118,30 @@ class Module {
       });
     }
 
+    let registry = { __proto__: null };
     if (sourceText !== undefined) {
       this[kWrap] = new ModuleWrap(identifier, context, sourceText,
                                    options.lineOffset, options.columnOffset,
                                    options.cachedData);
-      const { setCallbackForWrap } = require('internal/modules/esm/utils');
-      setCallbackForWrap(this[kWrap], {
+      registry = {
+        __proto__: null,
         initializeImportMeta: options.initializeImportMeta,
         importModuleDynamically: options.importModuleDynamically ?
           importModuleDynamicallyWrap(options.importModuleDynamically) :
           undefined,
-      });
+      };
     } else {
       assert(syntheticEvaluationSteps);
       this[kWrap] = new ModuleWrap(identifier, context,
                                    syntheticExportNames,
                                    syntheticEvaluationSteps);
     }
 
-    wrapToModuleMap.set(this[kWrap], this);
+    // This will take precedence over the referrer as the object being
+    // passed into the callbacks.
+    registry.callbackReferrer = this;
+    const { registerModule } = require('internal/modules/esm/utils');
+    registerModule(this[kWrap], registry);
 
     this[kContext] = context;
   }
@@ -445,5 +448,4 @@ module.exports = {
   SourceTextModule,
   SyntheticModule,
   importModuleDynamicallyWrap,
-  getModuleFromWrap: (wrap) => wrapToModuleMap.get(wrap),
 };
diff --git a/lib/vm.js b/lib/vm.js
@@ -105,8 +105,9 @@ class Script extends ContextifyScript {
       validateFunction(importModuleDynamically,
                        'options.importModuleDynamically');
       const { importModuleDynamicallyWrap } = require('internal/vm/module');
-      const { setCallbackForWrap } = require('internal/modules/esm/utils');
-      setCallbackForWrap(this, {
+      const { registerModule } = require('internal/modules/esm/utils');
+      registerModule(this, {
+        __proto__: null,
         importModuleDynamically:
           importModuleDynamicallyWrap(importModuleDynamically),
       });

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -393,16 +393,6 @@ inline AliasedInt32Array& Environment::stream_base_state() {
   return stream_base_state_;
 }
 
-inline uint32_t Environment::get_next_module_id() {
-  return module_id_counter_++;
-}
-inline uint32_t Environment::get_next_script_id() {
-  return script_id_counter_++;
-}
-inline uint32_t Environment::get_next_function_id() {
-  return function_id_counter_++;
-}
-
 ShouldNotAbortOnUncaughtScope::ShouldNotAbortOnUncaughtScope(
     Environment* env)
     : env_(env) {

diff --git a/src/env.h b/src/env.h
@@ -746,14 +746,6 @@ class Environment : public MemoryRetainer {
   builtins::BuiltinLoader* builtin_loader();
 
   std::unordered_multimap<int, loader::ModuleWrap*> hash_to_module_map;
-  std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
-  std::unordered_map<uint32_t, contextify::ContextifyScript*>
-      id_to_script_map;
-  std::unordered_map<uint32_t, contextify::CompiledFnEntry*> id_to_function_map;
-
-  inline uint32_t get_next_module_id();
-  inline uint32_t get_next_script_id();
-  inline uint32_t get_next_function_id();
 
   EnabledDebugList* enabled_debug_list() { return &enabled_debug_list_; }
 

diff --git a/src/env_properties.h b/src/env_properties.h
@@ -21,6 +21,7 @@
   V(arrow_message_private_symbol, "node:arrowMessage")                         \
   V(contextify_context_private_symbol, "node:contextify:context")              \
   V(decorated_private_symbol, "node:decorated")                                \
+  V(host_defined_option_symbol, "node:host_defined_option_symbol")             \
   V(napi_type_tag, "node:napi:type_tag")                                       \
   V(napi_wrapper, "node:napi:wrapper")                                         \
   V(untransferable_object_private_symbol, "node:untransferableObject")         \
@@ -338,7 +339,6 @@
   V(blocklist_constructor_template, v8::FunctionTemplate)                      \
   V(contextify_global_template, v8::ObjectTemplate)                            \
   V(contextify_wrapper_template, v8::ObjectTemplate)                           \
-  V(compiled_fn_entry_template, v8::ObjectTemplate)                            \
   V(crypto_key_object_handle_constructor, v8::FunctionTemplate)                \
   V(env_proxy_template, v8::ObjectTemplate)                                    \
   V(env_proxy_ctor_template, v8::FunctionTemplate)                             \