Skip to content

Commit

Permalink
vm: unify host-defined option generation in vm.compileFunction
Browse files Browse the repository at this point in the history
Set a default host-defined option for vm.compileFunction so that
it's consistent with vm.Script.

PR-URL: #50137
Backport-PR-URL: #51004
Refs: #35375
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
  • Loading branch information
joyeecheung authored and richardlau committed Mar 15, 2024
1 parent 6291c10 commit 9f7899e
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 24 deletions.
25 changes: 24 additions & 1 deletion lib/internal/vm.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@

const {
ArrayPrototypeForEach,
Symbol,
} = primordials;

const {
compileFunction,
isContext: _isContext,
} = internalBinding('contextify');
const {
default_host_defined_options,
} = internalBinding('symbols');
const {
validateArray,
validateBoolean,
Expand All @@ -28,12 +32,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,
Expand Down Expand Up @@ -70,6 +89,8 @@ function internalCompileFunction(code, params, options) {
validateObject(extension, name, { __proto__: null, nullable: true });
});

const hostDefinedOptionId =
getHostDefinedOptionId(importModuleDynamically, filename);
const result = compileFunction(
code,
filename,
Expand All @@ -80,6 +101,7 @@ function internalCompileFunction(code, params, options) {
parsingContext,
contextExtensions,
params,
hostDefinedOptionId,
);

if (produceCachedData) {
Expand Down Expand Up @@ -111,6 +133,7 @@ function internalCompileFunction(code, params, options) {
}

module.exports = {
getHostDefinedOptionId,
internalCompileFunction,
isContext,
};
12 changes: 4 additions & 8 deletions lib/vm.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ const {
const {
validateBoolean,
validateBuffer,
validateFunction,
validateInt32,
validateObject,
validateOneOf,
Expand All @@ -55,6 +54,7 @@ const {
kVmBreakFirstLineSymbol,
} = require('internal/util');
const {
getHostDefinedOptionId,
internalCompileFunction,
isContext,
} = require('internal/vm');
Expand Down Expand Up @@ -87,12 +87,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.
Expand All @@ -104,7 +100,7 @@ class Script extends ContextifyScript {
cachedData,
produceCachedData,
parsingContext,
importModuleDynamically !== undefined);
hostDefinedOptionId);
} catch (e) {
throw e; /* node-do-not-add-exception-line */
}
Expand Down
20 changes: 8 additions & 12 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -787,11 +787,11 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
bool produce_cached_data = false;
Local<Context> parsing_context = context;

bool needs_custom_host_defined_options = false;
Local<Symbol> 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<Int32>()->Value();
Expand All @@ -811,9 +811,8 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& 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<Symbol>();
}

ContextifyScript* contextify_script =
Expand All @@ -837,12 +836,6 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {

Local<PrimitiveArray> 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<Symbol> 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);

Expand Down Expand Up @@ -1201,6 +1194,10 @@ void ContextifyContext::CompileFunction(
params_buf = args[8].As<Array>();
}

// Argument 10: host-defined option symbol
CHECK(args[9]->IsSymbol());
Local<Symbol> id_symbol = args[9].As<Symbol>();

// Read cache from cached data buffer
ScriptCompiler::CachedData* cached_data = nullptr;
if (!cached_data_buf.IsEmpty()) {
Expand All @@ -1212,7 +1209,6 @@ void ContextifyContext::CompileFunction(
// Set host_defined_options
Local<PrimitiveArray> host_defined_options =
PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
Local<Symbol> id_symbol = Symbol::New(isolate, filename);
host_defined_options->Set(
isolate, loader::HostDefinedOptions::kID, id_symbol);

Expand Down
13 changes: 10 additions & 3 deletions test/parallel/test-vm-no-dynamic-import-callback.js
Original file line number Diff line number Diff line change
@@ -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 () => {
Expand All @@ -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());

0 comments on commit 9f7899e

Please sign in to comment.