Function returned by `vm.compileFunction` can't accept arguments when `cachedData` is passed

[RaisinTen]

Version

v20.2.0

Platform

Darwin Darshans-MacBook-Pro.local 22.4.0 Darwin Kernel Version 22.4.0: Mon Mar 6 21:00:17 PST 2023; root:xnu-8796.101.5~3/RELEASE_X86_64 x86_64

Subsystem

vm

What steps will reproduce the bug?

test.js

const vm = require('vm');

const code = `
const os = require('os');
console.log(os.release());
`;

const script = new vm.Script(code, {
  produceCachedData: true
});

const { cachedData } = script;

const fn = vm.compileFunction(code, ['require'], {
  cachedData
});
console.log(`fn: [${fn.toString()}]`);
fn(require);

$ node test.js
fn: [
const os = require('os');
console.log(os.release());
]
evalmachine.<anonymous>:2
const os = require('os');
           ^

ReferenceError: require is not defined
    at evalmachine.<anonymous>:2:12
    at Object.<anonymous> (/tmp/test.js:18:1)
    at Module._compile (node:internal/modules/cjs/loader:1255:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1309:10)
    at Module.load (node:internal/modules/cjs/loader:1113:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
    at node:internal/main/run_main_module:23:47

Node.js v20.2.0

Also, note that when I print the function, it doesn't have the function (...) { and } wrappers. I have no idea how that's even possible.

However, if I comment out the part where cachedData is being passed to vm.compileFunction, it doesn't run into errors.

test.js

const vm = require('vm');

const code = `
const os = require('os');
console.log(os.release());
`;

const script = new vm.Script(code, {
  produceCachedData: true
});

const { cachedData } = script;

const fn = vm.compileFunction(code, ['require'], {
  // cachedData
});
console.log(`fn: [${fn.toString()}]`);
fn(require);

$ node test.js
fn: [function (require) {

const os = require('os');
console.log(os.release());

}]
22.4.0

How often does it reproduce? Is there a required condition?

Reproduces always consistently.

What is the expected behavior?

The returned function should have the params wrapped in.

Why is that the expected behavior?

The doc says that this would return a function with the passed params wrapped in but in this case, it doesn't seem to do so.

What do you see instead?

The returned function does not have the params wrapped in.

Additional information

AFAICT, Node.js simply passes the args to V8's ScriptCompiler::CompileFunction function in

node/src/node_contextify.cc

Lines 1248 to 1256 in e530098

	MaybeLocal<Function> maybe_fn = ScriptCompiler::CompileFunction(
	parsing_context,
	&source,
	params.size(),
	params.data(),
	context_extensions.size(),
	context_extensions.data(),
	options,
	v8::ScriptCompiler::NoCacheReason::kNoCacheNoReason);

, so in case this is considered as buggy behavior, it might be a V8 bug.

[RaisinTen]

cc @nodejs/vm

[kvakil]

If you use the same functions / parameters it works as expected:

const vm = require('vm');

const code = `
const os = require('os');
console.log(os.release());
`;

const fn = vm.compileFunction(code, ['require'], {
    produceCachedData: true,
});
console.log(`fn: [${fn.toString()}]`);
fn(require);

const fn2 = vm.compileFunction(code, ['require'], {
    cachedData: fn.cachedData,
});
console.log(`fn2: [${fn2.toString()}]`);
fn2(require);

perhaps we can update the documentation to make this more clear?

[RaisinTen]

Done - #48193

[devsnek]

The current implementation seems unsafe. Can we validate the cached data somehow? Maybe we can put our own header on the bytes.

[kvakil]

Note it's not just parameters, the code is not validated either:

> vm = require('vm')
> f = vm.compileFunction('return 1', [], {produceCachedData: true})
[Function (anonymous)] {
  cachedDataProduced: true,
  cachedData: <Buffer 62 05 de c0 0f 3a dc 34 08 00 00 00 2f 12 c5 a8 90 01 00 00 15 22 82 81 01 1c 53 01 20 07 a8 60 00 00 00 00 03 00 00 00 5d 07 d9 01 01 0c 4b 61 00 00 ... 374 more bytes>
}
> vm.compileFunction('return 2', [], {cachedData: f.cachedData})()
1

Doing something intelligent here would break tools like bytenode which rely on us not verifying that the code matches the cached data (although we could work with them to minimize breakage). Any change here is probably semver-major.