From 8f48edd28fd4eb5aaac95246dc45f0a720cd286b Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 1 May 2019 23:22:47 +0200
Subject: [PATCH] vm: mark global proxy as side-effect-free
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes: https://github.com/nodejs/node/issues/27518

PR-URL: https://github.com/nodejs/node/pull/27523
Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 src/node_contextify.cc                        | 20 ++++++-----
 ...r-vm-global-accessors-getter-sideeffect.js | 33 +++++++++++++++++++
 ...spector-vm-global-accessors-sideeffects.js | 31 +++++++++++++++++
 3 files changed, 76 insertions(+), 8 deletions(-)
 create mode 100644 test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js
 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 6590da62c5b497..77622f8cbd6d2b 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -61,6 +61,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;
@@ -149,13 +150,15 @@ MaybeLocal<Context> ContextifyContext::CreateV8Context(
   if (!CreateDataWrapper(env).ToLocal(&data_wrapper))
     return MaybeLocal<Context>();
 
-  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,
@@ -164,7 +167,8 @@ MaybeLocal<Context> 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-getter-sideeffect.js b/test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js
new file mode 100644
index 00000000000000..5dc9c9bb2dff3a
--- /dev/null
+++ b/test/parallel/test-inspector-vm-global-accessors-getter-sideeffect.js
@@ -0,0 +1,33 @@
+'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));
+
+  assert(context);  // Keep 'context' alive and make linter happy.
+});
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'
+    }
+  });
+});