From 178dbb850333679ed4f2b1992c55098d9225e784 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 1 May 2019 23:22:47 +0200 Subject: [PATCH 1/3] vm: mark global proxy as side-effect-free Fixes: https://github.com/nodejs/node/issues/27518 --- src/node_contextify.cc | 20 +++++++----- ...spector-vm-global-accessors-sideeffects.js | 31 +++++++++++++++++++ 2 files changed, 43 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-inspector-vm-global-accessors-sideeffects.js diff --git a/src/node_contextify.cc b/src/node_contextify.cc index f8d43e062eef24..bc3c2ad5397f54 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -60,6 +60,7 @@ using v8::PrimitiveArray; using v8::PropertyAttribute; using v8::PropertyCallbackInfo; using v8::PropertyDescriptor; +using v8::PropertyHandlerFlags; using v8::Script; using v8::ScriptCompiler; using v8::ScriptOrigin; @@ -148,13 +149,15 @@ MaybeLocal ContextifyContext::CreateV8Context( if (!CreateDataWrapper(env).ToLocal(&data_wrapper)) return MaybeLocal(); - NamedPropertyHandlerConfiguration config(PropertyGetterCallback, - PropertySetterCallback, - PropertyDescriptorCallback, - PropertyDeleterCallback, - PropertyEnumeratorCallback, - PropertyDefinerCallback, - data_wrapper); + NamedPropertyHandlerConfiguration config( + PropertyGetterCallback, + PropertySetterCallback, + PropertyDescriptorCallback, + PropertyDeleterCallback, + PropertyEnumeratorCallback, + PropertyDefinerCallback, + data_wrapper, + PropertyHandlerFlags::kHasNoSideEffect); IndexedPropertyHandlerConfiguration indexed_config( IndexedPropertyGetterCallback, @@ -163,7 +166,8 @@ MaybeLocal ContextifyContext::CreateV8Context( IndexedPropertyDeleterCallback, PropertyEnumeratorCallback, IndexedPropertyDefinerCallback, - data_wrapper); + data_wrapper, + PropertyHandlerFlags::kHasNoSideEffect); object_template->SetHandler(config); object_template->SetHandler(indexed_config); diff --git a/test/parallel/test-inspector-vm-global-accessors-sideeffects.js b/test/parallel/test-inspector-vm-global-accessors-sideeffects.js new file mode 100644 index 00000000000000..31551a08cb569b --- /dev/null +++ b/test/parallel/test-inspector-vm-global-accessors-sideeffects.js @@ -0,0 +1,31 @@ +'use strict'; +const common = require('../common'); +common.skipIfInspectorDisabled(); + +// Regression test for https://github.com/nodejs/node/issues/27518. + +const assert = require('assert'); +const inspector = require('inspector'); +const vm = require('vm'); + +const session = new inspector.Session(); +session.connect(); + +const context = vm.createContext({ + a: 100 +}); + +session.post('Runtime.evaluate', { + expression: 'a', + throwOnSideEffect: true, + contextId: 2 // context's id +}, (error, res) => { + assert.ifError(error), + assert.deepStrictEqual(res, { + result: { + type: 'number', + value: context.a, + description: '100' + } + }); +}); From f5a1d1234f4b3516b4f41a88b791084f15de8197 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 1 May 2019 23:52:04 +0200 Subject: [PATCH 2/3] fixup! vm: mark global proxy as side-effect-free --- ...r-vm-global-accessors-getter-sideeffect.js | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js diff --git a/test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js b/test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js new file mode 100644 index 00000000000000..aafa7ef8064f19 --- /dev/null +++ b/test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js @@ -0,0 +1,31 @@ +'use strict'; +const common = require('../common'); +common.skipIfInspectorDisabled(); + +// Test that if there is a side effect in a getter invoked through the vm +// global proxy, Runtime.evaluate recognizes that. + +const assert = require('assert'); +const inspector = require('inspector'); +const vm = require('vm'); + +const session = new inspector.Session(); +session.connect(); + +const context = vm.createContext({ + get a() { + global.foo = '1'; + return 100; + } +}); + +session.post('Runtime.evaluate', { + expression: 'a', + throwOnSideEffect: true, + contextId: 2 // context's id +}, (error, res) => { + assert.ifError(error); + const { exception } = res.exceptionDetails; + assert.strictEqual(exception.className, 'EvalError'); + assert(/Possible side-effect/.test(exception.description)); +}); From ad47538a0841cebebb8e7563da7fd24599228909 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 3 May 2019 15:49:57 +0200 Subject: [PATCH 3/3] fixup! fixup! vm: mark global proxy as side-effect-free --- .../test-inspector-vm-global-accessors-getter-sideeffect.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js b/test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js index aafa7ef8064f19..5dc9c9bb2dff3a 100644 --- a/test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js +++ b/test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js @@ -28,4 +28,6 @@ session.post('Runtime.evaluate', { const { exception } = res.exceptionDetails; assert.strictEqual(exception.className, 'EvalError'); assert(/Possible side-effect/.test(exception.description)); + + assert(context); // Keep 'context' alive and make linter happy. });