From ed4514294a35eeb9405e5c052ea4b654f23290d5 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Fri, 26 May 2023 21:37:36 +0200 Subject: [PATCH] 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. PR-URL: https://github.com/nodejs/node/pull/47572 Reviewed-By: James M Snell --- src/node_contextify.cc | 1 + test/parallel/test-vm-global-get-own.js | 105 ++++++++++++++++++++++++ test/parallel/test-vm-global-symbol.js | 26 ------ 3 files changed, 106 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 75cb88fce8e8d3..6713f17f65314f 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -524,6 +524,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..246fcbf866b8b6 --- /dev/null +++ b/test/parallel/test-vm-global-get-own.js @@ -0,0 +1,105 @@ +'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* or in. +// +// Related to: +// - https://github.com/nodejs/node/issues/45983 + +const global = vm.runInContext('this', vm.createContext()); + +function runAssertions(data, property, viaDefine, value1, value2, value3) { + // Define the property for the first time + setPropertyAndAssert(data, property, viaDefine, value1); + // Update the property + setPropertyAndAssert(data, property, viaDefine, value2); + // Delete the property + deletePropertyAndAssert(data, property); + // Re-define the property + setPropertyAndAssert(data, property, viaDefine, value3); + // Delete the property again + deletePropertyAndAssert(data, property); +} + +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', true, fun1, 2, 3); +runAssertions(global, Symbol.for('tyty'), true, fun1, 2, 3); + +// Assertions on: direct assignment +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 +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 +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 + +// Set the property on data and assert it worked +function setPropertyAndAssert(data, property, viaDefine, value) { + if (viaDefine) { + Object.defineProperty(data, property, { + enumerable: true, + writable: true, + value: value, + configurable: true, + }); + } else { + data[property] = value; + } + assert.strictEqual(data[property], value); + assert.ok(property in data); + if (typeof property === 'string') { + assert.ok(Object.getOwnPropertyNames(data).includes(property)); + } else { + assert.ok(Object.getOwnPropertySymbols(data).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)); +} 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));