From 05992367a0d5be3f08d077753b9bb724c18d3008 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Wed, 1 Feb 2023 08:28:04 +0000 Subject: [PATCH 1/3] vm: properly support symbols on globals A regression has been introduced in node 18.2.0, it makes the following snippet fails while it used to work in the past: ``` 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(Object.getOwnPropertySymbols(global).includes(totoSymbol)); ``` Regression introduced by: https://github.com/nodejs/node/pull/42963. So I basically attempted to start understanding what it changed to make it fix the initial issue while not breaking the symbol related one. Fixes: https://github.com/nodejs/node/issues/45983 --- src/node_contextify.cc | 4 +++- test/parallel/test-vm-global-symbol.js | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-vm-global-symbol.js diff --git a/src/node_contextify.cc b/src/node_contextify.cc index ed552ddd559f51..fdab91714d7da0 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -530,7 +530,9 @@ void ContextifyContext::PropertySetterCallback( return; USE(ctx->sandbox()->Set(context, property, value)); - args.GetReturnValue().Set(value); + if (is_function) { + args.GetReturnValue().Set(value); + } } // static diff --git a/test/parallel/test-vm-global-symbol.js b/test/parallel/test-vm-global-symbol.js new file mode 100644 index 00000000000000..72ca935cd82438 --- /dev/null +++ b/test/parallel/test-vm-global-symbol.js @@ -0,0 +1,15 @@ +'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.ok(global[totoSymbol] === 4); +assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol)); From 020bc92a500df7e8e48b632aed011a06cac0f169 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Wed, 1 Feb 2023 19:45:33 +0000 Subject: [PATCH 2/3] Adding missing is_contextual_store||check --- src/node_contextify.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index fdab91714d7da0..f37e95497c48c9 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -530,7 +530,7 @@ void ContextifyContext::PropertySetterCallback( return; USE(ctx->sandbox()->Set(context, property, value)); - if (is_function) { + if (is_contextual_store || is_function) { args.GetReturnValue().Set(value); } } From b24cd68bfcce12923beddae0822727326886db33 Mon Sep 17 00:00:00 2001 From: Nicolas DUBIEN Date: Wed, 1 Feb 2023 12:23:37 +0100 Subject: [PATCH 3/3] Update test/parallel/test-vm-global-symbol.js Co-authored-by: Antoine du Hamel --- test/parallel/test-vm-global-symbol.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-vm-global-symbol.js b/test/parallel/test-vm-global-symbol.js index 72ca935cd82438..a0dfbac7b8b10b 100644 --- a/test/parallel/test-vm-global-symbol.js +++ b/test/parallel/test-vm-global-symbol.js @@ -11,5 +11,5 @@ Object.defineProperty(global, totoSymbol, { value: 4, configurable: true, }); -assert.ok(global[totoSymbol] === 4); +assert.strictEqual(global[totoSymbol], 4); assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol));