src: do not copy on failing setProperty()

In vm, the setter interceptor should not copy a value onto the
sandbox, if setting it on the global object will fail. It will fail if
we are in strict mode and set a value without declaring it.

Fixes: #5344
PR-URL: #7908
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

src/node_contextify.cc

@@ -380,7 +380,19 @@ class ContextifyContext {
     if (ctx->context_.IsEmpty())
       return;
 
-    ctx->sandbox()->Set(property, value);
+    bool is_declared =
+        ctx->global_proxy()->HasRealNamedProperty(ctx->context(),
+                                                  property).FromJust();
+    bool is_contextual_store = ctx->global_proxy() != args.This();
+
+    bool set_property_will_throw =
+        args.ShouldThrowOnError() &&
+        !is_declared &&
+        is_contextual_store;
+
+    if (!set_property_will_throw) {
+      ctx->sandbox()->Set(property, value);
+    }
   }
 
 

test/parallel/test-vm-strict-mode.js

@@ -0,0 +1,27 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+const vm = require('vm');
+const ctx = vm.createContext();
+
+// Test strict mode inside a vm script, i.e., using an undefined variable
+// throws a ReferenceError. Also check that variables
+// that are not successfully set in the vm, must not be set
+// on the sandboxed context.
+
+vm.runInContext('w = 1;', ctx);
+assert.strictEqual(1, ctx.w);
+
+assert.throws(function() { vm.runInContext('"use strict"; x = 1;', ctx); },
+              /ReferenceError: x is not defined/);
+assert.strictEqual(undefined, ctx.x);
+
+vm.runInContext('"use strict"; var y = 1;', ctx);
+assert.strictEqual(1, ctx.y);
+
+vm.runInContext('"use strict"; this.z = 1;', ctx);
+assert.strictEqual(1, ctx.z);
+
+// w has been defined
+vm.runInContext('"use strict"; w = 2;', ctx);
+assert.strictEqual(2, ctx.w);