Node: vm.compileFunction is crashing the node process with unknown error

[kalinkrustev]

• Version: 10.15.3
• Platform: Windows 10
• Subsystem: vm

The following simple script crashes the node process with no apparent reason:

const vm = require('vm');
console.log(process.version);
vm.compileFunction('return', ['ab'.replace('b', 'b'.repeat(12)).split('').join('')]);
console.log('compiled ok 1');
vm.compileFunction('return', ['ab'.replace('b', 'b'.repeat(11))]);
console.log('compiled ok 2');
try {
    vm.compileFunction('return', ['ab'.replace('b', 'b'.repeat(12))]); // crash here
    console.log('compiled ok 3');
} catch (e) { // no exception was caught
    console.error(e);
}

Output:

v10.15.3
compiled ok 1
compiled ok 2

[addaleax]

This seems to have been fixed between v11.10.1 and v11.11.0, so it’s probably just a question of figuring out the bugfix and backporting it. I’ll try to bisect.

[kalinkrustev]

I am curious to see the reason behind this. If you find the fix, can you please post a reference here?

[addaleax]

Ok, so, there are two issues here:

The crash is being addressed by 7b198935d63aec4acace1500d81ac2662c732d18; in particular, https://github.com/nodejs/node/commit/7b198935d63aec4acace1500d81ac2662c732d18#diff-0cf206672499c2f86db4ffb0cc5b668bL1101 would previously have crashed, because there was no exception caught by the try_catch object (which lead to a nullptr dereference).

However, this does not actually work on newer released versions of Node.js, it just returns undefined instead of a compiled function. That seems like a V8 bug, which is fixed on Node.js master but not on Node v11.x; I’ll try to investigate a bit more.

[addaleax]

Okay, digging a bit more: Until v8/v8@61f4c2251e107aebca620054701f2faec36209a8, V8 assumed that the argument name string was not internally represented as a concatenated string. Apparently, V8 has a cutoff value here that decides whether the result of your string concatenation is internally represented as a contiguous block of memory, or as the concatenation of two or more contiguous blocks of memory. The “is this a valid identifier” check would simply fail in the latter case. This is fixed by the linked V8 commit.

It’s also not ideal that V8 doesn’t throw an exception in this case, which imo it should when returning an empty MaybeLocal<> from ScriptCompiler::CompileFunctionInContext() … @hashseed Should that be added? I’d be happy to do so.

[addaleax]

https://github.com/nodejs/node/pull/27259 should address the main issue here for v11.x, and could be backported to v10.x.

[hashseed]

Yes. I think that it should throw an exception indeed, similar to other compile APIs.