From d164cd4d1e1df5d507ea719cf45607ab6d520137 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Sat, 15 Apr 2023 14:33:13 +0000 Subject: [PATCH 1/4] vm: properly handle defining symbol props This PR is a follow-up of #46615. When bumping V8 for node 18, various bugs appeared around vm. Some have been fixed along the flow, but there is at least one remaining and described at: https://github.com/facebook/jest/issues/13338. The current fix does nothing on node 20: the new bump of v8 done for node 20 seems to fix it. But it would fix the problem in both node 18 and node 19. I confirmed it would fix node 19 by cherry-picking the commit on v19.x-staging and launching `make -j4 jstest` on it. --- src/node_contextify.cc | 1 + test/parallel/test-vm-global-get-own.js | 80 +++++++++++++++++++++++++ test/parallel/test-vm-global-symbol.js | 26 -------- 3 files changed, 81 insertions(+), 26 deletions(-) create mode 100644 test/parallel/test-vm-global-get-own.js delete mode 100644 test/parallel/test-vm-global-symbol.js diff --git a/src/node_contextify.cc b/src/node_contextify.cc index a21acf06a32781..419756cb63c701 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -527,6 +527,7 @@ void ContextifyContext::PropertySetterCallback( !is_function) return; + if (!is_declared && property->IsSymbol()) return; if (ctx->sandbox()->Set(context, property, value).IsNothing()) return; Local desc; diff --git a/test/parallel/test-vm-global-get-own.js b/test/parallel/test-vm-global-get-own.js new file mode 100644 index 00000000000000..15a1735fd999be --- /dev/null +++ b/test/parallel/test-vm-global-get-own.js @@ -0,0 +1,80 @@ +'use strict'; +require('../common'); +const assert = require('assert'); +const vm = require('vm'); + +// These assertions check that we can set new keys to the global context, +// get them back and also list them via getOwnProperty*. +// +// Related to: +// - https://github.com/nodejs/node/issues/45983 + +const global = vm.runInContext('this', vm.createContext()); + +const totoSymbol = Symbol.for('toto'); +Object.defineProperty(global, totoSymbol, { + enumerable: true, + writable: true, + value: 4, + configurable: true, +}); +assert.strictEqual(global[totoSymbol], 4); +assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol)); +Object.defineProperty(global, totoSymbol, { + enumerable: true, + writable: true, + value: 44, + configurable: true, +}); +assert.strictEqual(global[totoSymbol], 44); +assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol)); + +const totoKey = 'toto'; +Object.defineProperty(global, totoKey, { + enumerable: true, + writable: true, + value: 5, + configurable: true, +}); +assert.strictEqual(global[totoKey], 5); +assert.ok(Object.getOwnPropertyNames(global).includes(totoKey)); +Object.defineProperty(global, totoKey, { + enumerable: true, + writable: true, + value: 55, + configurable: true, +}); +assert.strictEqual(global[totoKey], 55); +assert.ok(Object.getOwnPropertyNames(global).includes(totoKey)); + +const titiSymbol = Symbol.for('titi'); +global[titiSymbol] = 6; +assert.strictEqual(global[titiSymbol], 6); +assert.ok(Object.getOwnPropertySymbols(global).includes(titiSymbol)); +global[titiSymbol] = 66; +assert.strictEqual(global[titiSymbol], 66); +assert.ok(Object.getOwnPropertySymbols(global).includes(titiSymbol)); + +const titiKey = 'titi'; +global[titiKey] = 7; +assert.strictEqual(global[titiKey], 7); +assert.ok(Object.getOwnPropertyNames(global).includes(titiKey)); +global[titiKey] = 77; +assert.strictEqual(global[titiKey], 77); +assert.ok(Object.getOwnPropertyNames(global).includes(titiKey)); + +const tutuSymbol = Symbol.for('tutu'); +global[tutuSymbol] = () => {}; +assert.strictEqual(typeof global[tutuSymbol], 'function'); +assert.ok(Object.getOwnPropertySymbols(global).includes(tutuSymbol)); +global[tutuSymbol] = () => {}; +assert.strictEqual(typeof global[tutuSymbol], 'function'); +assert.ok(Object.getOwnPropertySymbols(global).includes(tutuSymbol)); + +const tutuKey = 'tutu'; +global[tutuKey] = () => {}; +assert.strictEqual(typeof global[tutuKey], 'function'); +assert.ok(Object.getOwnPropertyNames(global).includes(tutuKey)); +global[tutuKey] = () => {}; +assert.strictEqual(typeof global[tutuKey], 'function'); +assert.ok(Object.getOwnPropertyNames(global).includes(tutuKey)); diff --git a/test/parallel/test-vm-global-symbol.js b/test/parallel/test-vm-global-symbol.js deleted file mode 100644 index 92038d9bfcf02d..00000000000000 --- a/test/parallel/test-vm-global-symbol.js +++ /dev/null @@ -1,26 +0,0 @@ -'use strict'; -require('../common'); -const assert = require('assert'); -const vm = require('vm'); - -const global = vm.runInContext('this', vm.createContext()); - -const totoSymbol = Symbol.for('toto'); -Object.defineProperty(global, totoSymbol, { - enumerable: true, - writable: true, - value: 4, - configurable: true, -}); -assert.strictEqual(global[totoSymbol], 4); -assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol)); - -const totoKey = 'toto'; -Object.defineProperty(global, totoKey, { - enumerable: true, - writable: true, - value: 5, - configurable: true, -}); -assert.strictEqual(global[totoKey], 5); -assert.ok(Object.getOwnPropertyNames(global).includes(totoKey)); From d7fc35e5445a4d40b72983a581fdfbd63943acc0 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Wed, 19 Apr 2023 11:27:08 +0000 Subject: [PATCH 2/4] share more across variants of the test --- test/parallel/test-vm-global-get-own.js | 123 ++++++++++++------------ 1 file changed, 60 insertions(+), 63 deletions(-) diff --git a/test/parallel/test-vm-global-get-own.js b/test/parallel/test-vm-global-get-own.js index 15a1735fd999be..5459fd4a829a21 100644 --- a/test/parallel/test-vm-global-get-own.js +++ b/test/parallel/test-vm-global-get-own.js @@ -4,77 +4,74 @@ const assert = require('assert'); const vm = require('vm'); // These assertions check that we can set new keys to the global context, -// get them back and also list them via getOwnProperty*. +// get them back and also list them via getOwnProperty* or in. // // Related to: // - https://github.com/nodejs/node/issues/45983 const global = vm.runInContext('this', vm.createContext()); -const totoSymbol = Symbol.for('toto'); -Object.defineProperty(global, totoSymbol, { - enumerable: true, - writable: true, - value: 4, - configurable: true, -}); -assert.strictEqual(global[totoSymbol], 4); -assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol)); -Object.defineProperty(global, totoSymbol, { - enumerable: true, - writable: true, - value: 44, - configurable: true, -}); -assert.strictEqual(global[totoSymbol], 44); -assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol)); +function runAssertions(property, viaDefine, value1, value2, value3) { + // Define the property for the first time + setPropertyAndAssert(property, viaDefine, value1); + // Update the property + setPropertyAndAssert(property, viaDefine, value2); + // Delete the property + deletePropertyAndAssert(property); + // Re-define the property + setPropertyAndAssert(property, viaDefine, value3); + // Delete the property again + deletePropertyAndAssert(property); +} -const totoKey = 'toto'; -Object.defineProperty(global, totoKey, { - enumerable: true, - writable: true, - value: 5, - configurable: true, -}); -assert.strictEqual(global[totoKey], 5); -assert.ok(Object.getOwnPropertyNames(global).includes(totoKey)); -Object.defineProperty(global, totoKey, { - enumerable: true, - writable: true, - value: 55, - configurable: true, -}); -assert.strictEqual(global[totoKey], 55); -assert.ok(Object.getOwnPropertyNames(global).includes(totoKey)); +const fun1 = () => 1; +const fun2 = () => 2; +const fun3 = () => 3; -const titiSymbol = Symbol.for('titi'); -global[titiSymbol] = 6; -assert.strictEqual(global[titiSymbol], 6); -assert.ok(Object.getOwnPropertySymbols(global).includes(titiSymbol)); -global[titiSymbol] = 66; -assert.strictEqual(global[titiSymbol], 66); -assert.ok(Object.getOwnPropertySymbols(global).includes(titiSymbol)); +// Assertions on: define property +runAssertions('toto', true, 1, 2, 3); +runAssertions(Symbol.for('toto'), true, 1, 2, 3); +runAssertions('tutu', true, fun1, fun2, fun3); +runAssertions(Symbol.for('tutu'), true, fun1, fun2, fun3); +runAssertions('tyty', false, fun1, 2, 3); +runAssertions(Symbol.for('tyty'), false, fun1, 2, 3); -const titiKey = 'titi'; -global[titiKey] = 7; -assert.strictEqual(global[titiKey], 7); -assert.ok(Object.getOwnPropertyNames(global).includes(titiKey)); -global[titiKey] = 77; -assert.strictEqual(global[titiKey], 77); -assert.ok(Object.getOwnPropertyNames(global).includes(titiKey)); +// Assertions on: direct assignment +runAssertions('titi', false, 1, 2, 3); +runAssertions(Symbol.for('titi'), false, 1, 2, 3); +runAssertions('tata', false, fun1, fun2, fun3); +runAssertions(Symbol.for('tata'), false, fun1, fun2, fun3); +runAssertions('tztz', false, fun1, 2, 3); +runAssertions(Symbol.for('tztz'), false, fun1, 2, 3); -const tutuSymbol = Symbol.for('tutu'); -global[tutuSymbol] = () => {}; -assert.strictEqual(typeof global[tutuSymbol], 'function'); -assert.ok(Object.getOwnPropertySymbols(global).includes(tutuSymbol)); -global[tutuSymbol] = () => {}; -assert.strictEqual(typeof global[tutuSymbol], 'function'); -assert.ok(Object.getOwnPropertySymbols(global).includes(tutuSymbol)); +// Helpers -const tutuKey = 'tutu'; -global[tutuKey] = () => {}; -assert.strictEqual(typeof global[tutuKey], 'function'); -assert.ok(Object.getOwnPropertyNames(global).includes(tutuKey)); -global[tutuKey] = () => {}; -assert.strictEqual(typeof global[tutuKey], 'function'); -assert.ok(Object.getOwnPropertyNames(global).includes(tutuKey)); +// Set the property on global and assert it worked +function setPropertyAndAssert(property, viaDefine, value) { + if (viaDefine) { + Object.defineProperty(global, property, { + enumerable: true, + writable: true, + value: value, + configurable: true, + }); + } else { + global[property] = value; + } + assert.strictEqual(global[property], value); + assert.ok(property in global); + if (typeof property === 'string') { + assert.ok(Object.getOwnPropertyNames(global).includes(property)); + } else { + assert.ok(Object.getOwnPropertySymbols(global).includes(property)); + } +} + +// Delete the property from global and assert it worked +function deletePropertyAndAssert(property) { + delete global[property]; + assert.strictEqual(global[property], undefined); + assert.ok(!(property in global)); + assert.ok(!Object.getOwnPropertyNames(global).includes(property)); + assert.ok(!Object.getOwnPropertySymbols(global).includes(property)); +} From 6ab652b4333aa73d63c875435d939e98e96ef2e4 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Wed, 19 Apr 2023 11:42:21 +0000 Subject: [PATCH 3/4] run assertions from the sandbox too --- test/parallel/test-vm-global-get-own.js | 118 +++++++++++++++++------- 1 file changed, 85 insertions(+), 33 deletions(-) diff --git a/test/parallel/test-vm-global-get-own.js b/test/parallel/test-vm-global-get-own.js index 5459fd4a829a21..abc3ea4bb7d8e5 100644 --- a/test/parallel/test-vm-global-get-own.js +++ b/test/parallel/test-vm-global-get-own.js @@ -11,17 +11,17 @@ const vm = require('vm'); const global = vm.runInContext('this', vm.createContext()); -function runAssertions(property, viaDefine, value1, value2, value3) { +function runAssertions(data, property, viaDefine, value1, value2, value3) { // Define the property for the first time - setPropertyAndAssert(property, viaDefine, value1); + setPropertyAndAssert(data, property, viaDefine, value1); // Update the property - setPropertyAndAssert(property, viaDefine, value2); + setPropertyAndAssert(data, property, viaDefine, value2); // Delete the property - deletePropertyAndAssert(property); + deletePropertyAndAssert(data, property); // Re-define the property - setPropertyAndAssert(property, viaDefine, value3); + setPropertyAndAssert(data, property, viaDefine, value3); // Delete the property again - deletePropertyAndAssert(property); + deletePropertyAndAssert(data, property); } const fun1 = () => 1; @@ -29,49 +29,101 @@ const fun2 = () => 2; const fun3 = () => 3; // Assertions on: define property -runAssertions('toto', true, 1, 2, 3); -runAssertions(Symbol.for('toto'), true, 1, 2, 3); -runAssertions('tutu', true, fun1, fun2, fun3); -runAssertions(Symbol.for('tutu'), true, fun1, fun2, fun3); -runAssertions('tyty', false, fun1, 2, 3); -runAssertions(Symbol.for('tyty'), false, fun1, 2, 3); +runAssertions(global, 'toto', true, 1, 2, 3); +runAssertions(global, Symbol.for('toto'), true, 1, 2, 3); +runAssertions(global, 'tutu', true, fun1, fun2, fun3); +runAssertions(global, Symbol.for('tutu'), true, fun1, fun2, fun3); +runAssertions(global, 'tyty', false, fun1, 2, 3); +runAssertions(global, Symbol.for('tyty'), false, fun1, 2, 3); // Assertions on: direct assignment -runAssertions('titi', false, 1, 2, 3); -runAssertions(Symbol.for('titi'), false, 1, 2, 3); -runAssertions('tata', false, fun1, fun2, fun3); -runAssertions(Symbol.for('tata'), false, fun1, fun2, fun3); -runAssertions('tztz', false, fun1, 2, 3); -runAssertions(Symbol.for('tztz'), false, fun1, 2, 3); +runAssertions(global, 'titi', false, 1, 2, 3); +runAssertions(global, Symbol.for('titi'), false, 1, 2, 3); +runAssertions(global, 'tata', false, fun1, fun2, fun3); +runAssertions(global, Symbol.for('tata'), false, fun1, fun2, fun3); +runAssertions(global, 'tztz', false, fun1, 2, 3); +runAssertions(global, Symbol.for('tztz'), false, fun1, 2, 3); + +// Assertions on: define property from sandbox +vm.runInContext( + "runAssertions(this, 'toto', true, 1, 2, 3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); +vm.runInContext( + "runAssertions(this, Symbol.for('toto'), true, 1, 2, 3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); +vm.runInContext( + "runAssertions(this, 'tutu', true, fun1, fun2, fun3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); +vm.runInContext( + "runAssertions(this, Symbol.for('tutu'), true, fun1, fun2, fun3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); +vm.runInContext( + "runAssertions(this, 'tyty', false, fun1, 2, 3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); +vm.runInContext( + "runAssertions(this, Symbol.for('tyty'), false, fun1, 2, 3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); + +// Assertions on: direct assignment from sandbox +vm.runInContext( + "runAssertions(this, 'titi', false, 1, 2, 3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); +vm.runInContext( + "runAssertions(this, Symbol.for('titi'), false, 1, 2, 3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); +vm.runInContext( + "runAssertions(this, 'tata', false, fun1, fun2, fun3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); +vm.runInContext( + "runAssertions(this, Symbol.for('tata'), false, fun1, fun2, fun3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); +vm.runInContext( + "runAssertions(this, 'tztz', false, fun1, 2, 3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); +vm.runInContext( + "runAssertions(this, Symbol.for('tztz'), false, fun1, 2, 3)", + vm.createContext({ runAssertions, fun1, fun2, fun3 }) +); // Helpers -// Set the property on global and assert it worked -function setPropertyAndAssert(property, viaDefine, value) { +// Set the property on data and assert it worked +function setPropertyAndAssert(data, property, viaDefine, value) { if (viaDefine) { - Object.defineProperty(global, property, { + Object.defineProperty(data, property, { enumerable: true, writable: true, value: value, configurable: true, }); } else { - global[property] = value; + data[property] = value; } - assert.strictEqual(global[property], value); - assert.ok(property in global); + assert.strictEqual(data[property], value); + assert.ok(property in data); if (typeof property === 'string') { - assert.ok(Object.getOwnPropertyNames(global).includes(property)); + assert.ok(Object.getOwnPropertyNames(data).includes(property)); } else { - assert.ok(Object.getOwnPropertySymbols(global).includes(property)); + assert.ok(Object.getOwnPropertySymbols(data).includes(property)); } } -// Delete the property from global and assert it worked -function deletePropertyAndAssert(property) { - delete global[property]; - assert.strictEqual(global[property], undefined); - assert.ok(!(property in global)); - assert.ok(!Object.getOwnPropertyNames(global).includes(property)); - assert.ok(!Object.getOwnPropertySymbols(global).includes(property)); +// Delete the property from data and assert it worked +function deletePropertyAndAssert(data, property) { + delete data[property]; + assert.strictEqual(data[property], undefined); + assert.ok(!(property in data)); + assert.ok(!Object.getOwnPropertyNames(data).includes(property)); + assert.ok(!Object.getOwnPropertySymbols(data).includes(property)); } From d108a9c6b972482b7a660b28eba153e5b172b235 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Wed, 19 Apr 2023 18:35:07 +0000 Subject: [PATCH 4/4] run even more assertions against sandbox --- test/parallel/test-vm-global-get-own.js | 72 +++++++++---------------- 1 file changed, 24 insertions(+), 48 deletions(-) diff --git a/test/parallel/test-vm-global-get-own.js b/test/parallel/test-vm-global-get-own.js index abc3ea4bb7d8e5..246fcbf866b8b6 100644 --- a/test/parallel/test-vm-global-get-own.js +++ b/test/parallel/test-vm-global-get-own.js @@ -28,13 +28,19 @@ const fun1 = () => 1; const fun2 = () => 2; const fun3 = () => 3; +function runAssertionsOnSandbox(builder) { + const sandboxContext = vm.createContext({ runAssertions, fun1, fun2, fun3 }); + vm.runInContext(builder('this'), sandboxContext); + vm.runInContext(builder('{}'), sandboxContext); +} + // Assertions on: define property runAssertions(global, 'toto', true, 1, 2, 3); runAssertions(global, Symbol.for('toto'), true, 1, 2, 3); runAssertions(global, 'tutu', true, fun1, fun2, fun3); runAssertions(global, Symbol.for('tutu'), true, fun1, fun2, fun3); -runAssertions(global, 'tyty', false, fun1, 2, 3); -runAssertions(global, Symbol.for('tyty'), false, fun1, 2, 3); +runAssertions(global, 'tyty', true, fun1, 2, 3); +runAssertions(global, Symbol.for('tyty'), true, fun1, 2, 3); // Assertions on: direct assignment runAssertions(global, 'titi', false, 1, 2, 3); @@ -45,55 +51,25 @@ runAssertions(global, 'tztz', false, fun1, 2, 3); runAssertions(global, Symbol.for('tztz'), false, fun1, 2, 3); // Assertions on: define property from sandbox -vm.runInContext( - "runAssertions(this, 'toto', true, 1, 2, 3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) -); -vm.runInContext( - "runAssertions(this, Symbol.for('toto'), true, 1, 2, 3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) -); -vm.runInContext( - "runAssertions(this, 'tutu', true, fun1, fun2, fun3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) -); -vm.runInContext( - "runAssertions(this, Symbol.for('tutu'), true, fun1, fun2, fun3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) -); -vm.runInContext( - "runAssertions(this, 'tyty', false, fun1, 2, 3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) -); -vm.runInContext( - "runAssertions(this, Symbol.for('tyty'), false, fun1, 2, 3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) +runAssertionsOnSandbox( + (variable) => ` + runAssertions(${variable}, 'toto', true, 1, 2, 3); + runAssertions(${variable}, Symbol.for('toto'), true, 1, 2, 3); + runAssertions(${variable}, 'tutu', true, fun1, fun2, fun3); + runAssertions(${variable}, Symbol.for('tutu'), true, fun1, fun2, fun3); + runAssertions(${variable}, 'tyty', true, fun1, 2, 3); + runAssertions(${variable}, Symbol.for('tyty'), true, fun1, 2, 3);` ); // Assertions on: direct assignment from sandbox -vm.runInContext( - "runAssertions(this, 'titi', false, 1, 2, 3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) -); -vm.runInContext( - "runAssertions(this, Symbol.for('titi'), false, 1, 2, 3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) -); -vm.runInContext( - "runAssertions(this, 'tata', false, fun1, fun2, fun3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) -); -vm.runInContext( - "runAssertions(this, Symbol.for('tata'), false, fun1, fun2, fun3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) -); -vm.runInContext( - "runAssertions(this, 'tztz', false, fun1, 2, 3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) -); -vm.runInContext( - "runAssertions(this, Symbol.for('tztz'), false, fun1, 2, 3)", - vm.createContext({ runAssertions, fun1, fun2, fun3 }) +runAssertionsOnSandbox( + (variable) => ` + runAssertions(${variable}, 'titi', false, 1, 2, 3); + runAssertions(${variable}, Symbol.for('titi'), false, 1, 2, 3); + runAssertions(${variable}, 'tata', false, fun1, fun2, fun3); + runAssertions(${variable}, Symbol.for('tata'), false, fun1, fun2, fun3); + runAssertions(${variable}, 'tztz', false, fun1, 2, 3); + runAssertions(${variable}, Symbol.for('tztz'), false, fun1, 2, 3);` ); // Helpers