vm: in strict mode, assigning functions to certain globals throws, but succeeds

[ninevra]

• Version: v14.17.0
• Platform: Linux 5.8.0-53-generic x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: vm

What steps will reproduce the bug?

const vm = require('vm');

vm.runInNewContext(`
"use strict";

try {
  Foo = function myfoo() {};
  console.log("function-valued assignment to undeclared global Foo did not throw");
} catch (error) {
  console.log("function-valued assignment to undeclared global Foo threw", error);
}

try {
  console.log("Foo is", Foo);
} catch (error) {
  console.log("Foo was not set");
}
`, {console});

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

Requires strict mode, requires that the value be a function.

Can be reproduced with an undeclared global, or with a global added to the context object post-contextualization. Cannot be reproduced with a global defined in the script, nor with a global added to the context object pre-contextualization.

Post-contextualization global example

const vm = require('vm');

const context = vm.createContext({console});
context.Foo = function foo() {};

vm.runInNewContext(`
"use strict";

try {
  console.log("Foo is", Foo);
} catch (error) {
  console.log("Foo is not set");
}

try {
  Foo = function myfoo() {};
  console.log("function-valued assignment to global Foo did not throw");
} catch (error) {
  console.log("function-valued assignment to global Foo threw", error);
}

try {
  console.log("Foo is", Foo);
} catch (error) {
  console.log("Foo is not set");
}
`, context);

What is the expected behavior?

With an undeclared global, the assignment should throw, and the global should not be set. This is what happens when running the same code in node directly, rather than in vm.

With a declared global, the assignment should not throw, and the global should be set.

What do you see instead?

The assignment throws, but is still performed.

function-valued assignment to undeclared global Foo threw ReferenceError: Foo is not defined
...
Foo is [Function: myfoo]

Additional information

node/src/node_contextify.cc

Lines 436 to 443 in a172397

	// Indicator to not return before setting (undeclared) function declarations
	// on the sandbox in strict mode, i.e. args.ShouldThrowOnError() = true.
	// True for 'function f() {}', 'this.f = function() {}',
	// 'var f = function()'.
	// In effect only for 'function f() {}' because
	// var f = function(), is_declared = true
	// this.f = function() {}, is_contextual_store = false.
	bool is_function = value->IsFunction();

seems like a potential culprit, although I don't really understand what's going on there.

[Ayase-252]

Reproduced in v16.3.0

If the inner code run in Chrome, it outputs

function-valued assignment to undeclared global Foo threw ReferenceError: Foo is not defined
    at index.html:7
index.html:16 Foo was not set

cc @nodejs/vm

[mischnic]

A related problem: even globals that are defined can't be assigned in strict mode:

const vm = require('vm');
const context = { output: undefined }; vm.createContext(context);
let s = new vm.Script(
  `"use strict";
output = 3;            // works fine
output = null;         // works fine
output = false;        // works fine
output = {};           // works fine
output = function(){}; // ReferenceError: output is not defined`,
);

try {
  s.runInContext(context)
} catch(e){
  console.error(e);
}

console.log(context.output);

On the other hand, both reproductions appear to be working correctly with Node 18 (and fail with 12, 14, 16)!

[ExE-Boss]

This is possibly related to #31808.