Permalink
Browse files

src: don't overwrite non-writable vm globals

Check that the property doesn't have the read-only flag set before
overwriting it.

This is Ben Noordhuis previous commit, but keeping
is_contextual_store. is_contextual_store describes whether
this.foo = 42 or foo = 42 was called. The second is contextual
and will fail in strict mode if foo is used without
declaration. Therefore only do an early return if it is
a contextual store. In particular, don't do an early
return for Object.defineProperty(this, ...).

Fixes: #10223
Refs: #10227
PR-URL: #11109
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
  • Loading branch information...
fhinkel authored and italoacasas committed Feb 1, 2017
1 parent 5649174 commit d8a5e1c37ffc2aae67ec76db0bc895ff6975d32e
Showing with 31 additions and 9 deletions.
  1. +18 −9 src/node_contextify.cc
  2. +13 −0 test/parallel/test-vm-context.js
View
@@ -383,19 +383,28 @@ class ContextifyContext {
if (ctx->context_.IsEmpty())
return;
+ auto attributes = PropertyAttribute::None;
bool is_declared =
- ctx->global_proxy()->HasRealNamedProperty(ctx->context(),
- property).FromJust();
+ ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(),
+ property)
+ .To(&attributes);
+ bool read_only =
+ static_cast<int>(attributes) &
+ static_cast<int>(PropertyAttribute::ReadOnly);
+
+ if (is_declared && read_only)
+ return;
+
+ // true for x = 5
+ // false for this.x = 5
+ // false for Object.defineProperty(this, 'foo', ...)
+ // false for vmResult.x = 5 where vmResult = vm.runInContext();
bool is_contextual_store = ctx->global_proxy() != args.This();
- bool set_property_will_throw =
- args.ShouldThrowOnError() &&
- !is_declared &&
- is_contextual_store;
+ if (!is_declared && args.ShouldThrowOnError() && is_contextual_store)
+ return;
- if (!set_property_will_throw) {
- ctx->sandbox()->Set(property, value);
- }
+ ctx->sandbox()->Set(property, value);
}
@@ -75,3 +75,16 @@ assert.throws(function() {
// https://github.com/nodejs/node/issues/6158
ctx = new Proxy({}, {});
assert.strictEqual(typeof vm.runInNewContext('String', ctx), 'function');
+
+// https://github.com/nodejs/node/issues/10223
+ctx = vm.createContext();
+vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
+assert.strictEqual(ctx.x, 42);
+assert.strictEqual(vm.runInContext('x', ctx), 42);
+
+vm.runInContext('x = 0', ctx); // Does not throw but x...
+assert.strictEqual(vm.runInContext('x', ctx), 42); // ...should be unaltered.
+
+assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx),
+ /Cannot assign to read only property 'x'/);
+assert.strictEqual(vm.runInContext('x', ctx), 42);

0 comments on commit d8a5e1c

Please sign in to comment.