From 56bbc30a44f87e65c34f655a5658d19970e36175 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 5 Oct 2023 03:27:11 +0200 Subject: [PATCH] vm: unify host-defined option generation in vm.compileFunction Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: https://github.com/nodejs/node/pull/50137 Refs: https://github.com/nodejs/node/issues/35375 Reviewed-By: Geoffrey Booth Reviewed-By: Yagiz Nizipli Reviewed-By: Chengzhong Wu Reviewed-By: Antoine du Hamel --- lib/internal/vm.js | 25 ++++++++++++++++++- lib/vm.js | 12 +++------ src/node_contextify.cc | 20 ++++++--------- .../test-vm-no-dynamic-import-callback.js | 13 +++++++--- 4 files changed, 46 insertions(+), 24 deletions(-) diff --git a/lib/internal/vm.js b/lib/internal/vm.js index f348ef6d2d612f..a463bdec88bdd4 100644 --- a/lib/internal/vm.js +++ b/lib/internal/vm.js @@ -2,12 +2,16 @@ const { ArrayPrototypeForEach, + Symbol, } = primordials; const { compileFunction, isContext: _isContext, } = internalBinding('contextify'); +const { + default_host_defined_options, +} = internalBinding('symbols'); const { validateArray, validateBoolean, @@ -30,12 +34,27 @@ function isContext(object) { return _isContext(object); } +function getHostDefinedOptionId(importModuleDynamically, filename) { + if (importModuleDynamically !== undefined) { + // Check that it's either undefined or a function before we pass + // it into the native constructor. + validateFunction(importModuleDynamically, + 'options.importModuleDynamically'); + } + if (importModuleDynamically === undefined) { + // We need a default host defined options that are the same for all + // scripts not needing custom module callbacks so that the isolate + // compilation cache can be hit. + return default_host_defined_options; + } + return Symbol(filename); +} + function internalCompileFunction(code, params, options) { validateString(code, 'code'); if (params !== undefined) { validateStringArray(params, 'params'); } - const { filename = '', columnOffset = 0, @@ -72,6 +91,8 @@ function internalCompileFunction(code, params, options) { validateObject(extension, name, kValidateObjectAllowNullable); }); + const hostDefinedOptionId = + getHostDefinedOptionId(importModuleDynamically, filename); const result = compileFunction( code, filename, @@ -82,6 +103,7 @@ function internalCompileFunction(code, params, options) { parsingContext, contextExtensions, params, + hostDefinedOptionId, ); if (produceCachedData) { @@ -113,6 +135,7 @@ function internalCompileFunction(code, params, options) { } module.exports = { + getHostDefinedOptionId, internalCompileFunction, isContext, }; diff --git a/lib/vm.js b/lib/vm.js index f134cdc983db6d..fe2981019a6170 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -41,7 +41,6 @@ const { const { validateBoolean, validateBuffer, - validateFunction, validateInt32, validateObject, validateOneOf, @@ -54,6 +53,7 @@ const { kVmBreakFirstLineSymbol, } = require('internal/util'); const { + getHostDefinedOptionId, internalCompileFunction, isContext, } = require('internal/vm'); @@ -86,12 +86,8 @@ class Script extends ContextifyScript { } validateBoolean(produceCachedData, 'options.produceCachedData'); - if (importModuleDynamically !== undefined) { - // Check that it's either undefined or a function before we pass - // it into the native constructor. - validateFunction(importModuleDynamically, - 'options.importModuleDynamically'); - } + const hostDefinedOptionId = + getHostDefinedOptionId(importModuleDynamically, filename); // Calling `ReThrow()` on a native TryCatch does not generate a new // abort-on-uncaught-exception check. A dummy try/catch in JS land // protects against that. @@ -103,7 +99,7 @@ class Script extends ContextifyScript { cachedData, produceCachedData, parsingContext, - importModuleDynamically !== undefined); + hostDefinedOptionId); } catch (e) { throw e; /* node-do-not-add-exception-line */ } diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 1754797e06851b..f669f323b1c6f3 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -771,11 +771,11 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { bool produce_cached_data = false; Local parsing_context = context; - bool needs_custom_host_defined_options = false; + Local id_symbol; if (argc > 2) { // new ContextifyScript(code, filename, lineOffset, columnOffset, // cachedData, produceCachedData, parsingContext, - // needsCustomHostDefinedOptions) + // hostDefinedOptionId) CHECK_EQ(argc, 8); CHECK(args[2]->IsNumber()); line_offset = args[2].As()->Value(); @@ -795,9 +795,8 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { CHECK_NOT_NULL(sandbox); parsing_context = sandbox->context(); } - if (args[7]->IsTrue()) { - needs_custom_host_defined_options = true; - } + CHECK(args[7]->IsSymbol()); + id_symbol = args[7].As(); } ContextifyScript* contextify_script = @@ -821,12 +820,6 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { Local host_defined_options = PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); - // We need a default host defined options that's the same for all scripts - // not needing custom module callbacks for so that the isolate compilation - // cache can be hit. - Local id_symbol = needs_custom_host_defined_options - ? Symbol::New(isolate, filename) - : env->default_host_defined_options(); host_defined_options->Set( isolate, loader::HostDefinedOptions::kID, id_symbol); @@ -1200,6 +1193,10 @@ void ContextifyContext::CompileFunction( params_buf = args[8].As(); } + // Argument 10: host-defined option symbol + CHECK(args[9]->IsSymbol()); + Local id_symbol = args[9].As(); + // Read cache from cached data buffer ScriptCompiler::CachedData* cached_data = nullptr; if (!cached_data_buf.IsEmpty()) { @@ -1211,7 +1208,6 @@ void ContextifyContext::CompileFunction( // Set host_defined_options Local host_defined_options = PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); - Local id_symbol = Symbol::New(isolate, filename); host_defined_options->Set( isolate, loader::HostDefinedOptions::kID, id_symbol); diff --git a/test/parallel/test-vm-no-dynamic-import-callback.js b/test/parallel/test-vm-no-dynamic-import-callback.js index 3651f997598b21..35b553d587c6e4 100644 --- a/test/parallel/test-vm-no-dynamic-import-callback.js +++ b/test/parallel/test-vm-no-dynamic-import-callback.js @@ -1,7 +1,7 @@ 'use strict'; -require('../common'); -const { Script } = require('vm'); +const common = require('../common'); +const { Script, compileFunction } = require('vm'); const assert = require('assert'); assert.rejects(async () => { @@ -10,4 +10,11 @@ assert.rejects(async () => { await imported; }, { code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING' -}); +}).then(common.mustCall()); + +assert.rejects(async () => { + const imported = compileFunction('return import("fs")')(); + await imported; +}, { + code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING' +}).then(common.mustCall());