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: https://github.com/nodejs/node/issues/10223
Refs: https://github.com/nodejs/node/pull/10227
PR-URL: https://github.com/nodejs/node/pull/11109
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

1 files changed, 18 insertions, 9 deletions

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index d74b01ea0d..5ed7d5faf0 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -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);
   }