From 84950bcfa92c8f73f44080ef458ddab06f131665 Mon Sep 17 00:00:00 2001
From: Domenic Denicola <d@domenic.me>
Date: Mon, 1 Jun 2015 12:22:19 -0400
Subject: [PATCH] vm: fix symbol access

By using the new SetHandler API instead of SetNamedPropertyHandler, we can
intercept symbols now. This forces us to use Maybes and MaybeLocals more,
since this new API does not have a non-maybe variant.

Fixes: https://github.com/nodejs/io.js/issues/884
PR-URL: https://github.com/nodejs/io.js/pull/1773
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
---
 src/node_contextify.cc           | 70 +++++++++++++++++++-------------
 test/parallel/test-vm-symbols.js | 25 ++++++++++++
 2 files changed, 67 insertions(+), 28 deletions(-)
 create mode 100644 test/parallel/test-vm-symbols.js

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 6c05e78d2b88ea..c89c89bd388379 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -26,6 +26,10 @@ using v8::HandleScope;
 using v8::Integer;
 using v8::Isolate;
 using v8::Local;
+using v8::Maybe;
+using v8::MaybeLocal;
+using v8::Name;
+using v8::NamedPropertyHandlerConfiguration;
 using v8::None;
 using v8::Object;
 using v8::ObjectTemplate;
@@ -202,12 +206,14 @@ class ContextifyContext {
 
     Local<ObjectTemplate> object_template =
         function_template->InstanceTemplate();
-    object_template->SetNamedPropertyHandler(GlobalPropertyGetterCallback,
+
+    NamedPropertyHandlerConfiguration config(GlobalPropertyGetterCallback,
                                              GlobalPropertySetterCallback,
                                              GlobalPropertyQueryCallback,
                                              GlobalPropertyDeleterCallback,
                                              GlobalPropertyEnumeratorCallback,
                                              CreateDataWrapper(env));
+    object_template->SetHandler(config);
 
     Local<Context> ctx = Context::New(env->isolate(), nullptr, object_template);
     if (!ctx.IsEmpty())
@@ -342,7 +348,7 @@ class ContextifyContext {
 
 
   static void GlobalPropertyGetterCallback(
-      Local<String> property,
+      Local<Name> property,
       const PropertyCallbackInfo<Value>& args) {
     Isolate* isolate = args.GetIsolate();
     HandleScope scope(isolate);
@@ -351,22 +357,26 @@ class ContextifyContext {
         Unwrap<ContextifyContext>(args.Data().As<Object>());
 
     Local<Object> sandbox = PersistentToLocal(isolate, ctx->sandbox_);
-    Local<Value> rv = sandbox->GetRealNamedProperty(property);
-    if (rv.IsEmpty()) {
+    MaybeLocal<Value> maybe_rv =
+        sandbox->GetRealNamedProperty(ctx->context(), property);
+    if (maybe_rv.IsEmpty()) {
       Local<Object> proxy_global = PersistentToLocal(isolate,
                                                      ctx->proxy_global_);
-      rv = proxy_global->GetRealNamedProperty(property);
-    }
-    if (!rv.IsEmpty() && rv == ctx->sandbox_) {
-      rv = PersistentToLocal(isolate, ctx->proxy_global_);
+      maybe_rv = proxy_global->GetRealNamedProperty(ctx->context(), property);
     }
 
-    args.GetReturnValue().Set(rv);
+    Local<Value> rv;
+    if (maybe_rv.ToLocal(&rv)) {
+      if (rv == ctx->sandbox_)
+        rv = PersistentToLocal(isolate, ctx->proxy_global_);
+
+      args.GetReturnValue().Set(rv);
+    }
   }
 
 
   static void GlobalPropertySetterCallback(
-      Local<String> property,
+      Local<Name> property,
       Local<Value> value,
       const PropertyCallbackInfo<Value>& args) {
     Isolate* isolate = args.GetIsolate();
@@ -380,7 +390,7 @@ class ContextifyContext {
 
 
   static void GlobalPropertyQueryCallback(
-      Local<String> property,
+      Local<Name> property,
       const PropertyCallbackInfo<Integer>& args) {
     Isolate* isolate = args.GetIsolate();
     HandleScope scope(isolate);
@@ -389,35 +399,39 @@ class ContextifyContext {
         Unwrap<ContextifyContext>(args.Data().As<Object>());
 
     Local<Object> sandbox = PersistentToLocal(isolate, ctx->sandbox_);
-    Local<Object> proxy_global = PersistentToLocal(isolate,
-                                                   ctx->proxy_global_);
-
-    if (sandbox->HasRealNamedProperty(property)) {
-      PropertyAttribute propAttr =
-          sandbox->GetRealNamedPropertyAttributes(property).FromJust();
-      args.GetReturnValue().Set(propAttr);
-    } else if (proxy_global->HasRealNamedProperty(property)) {
-      PropertyAttribute propAttr =
-          proxy_global->GetRealNamedPropertyAttributes(property).FromJust();
-      args.GetReturnValue().Set(propAttr);
-    } else {
-      args.GetReturnValue().Set(None);
+    Maybe<PropertyAttribute> maybe_prop_attr =
+        sandbox->GetRealNamedPropertyAttributes(ctx->context(), property);
+
+    if (maybe_prop_attr.IsNothing()) {
+      Local<Object> proxy_global = PersistentToLocal(isolate,
+          ctx->proxy_global_);
+
+      maybe_prop_attr =
+          proxy_global->GetRealNamedPropertyAttributes(ctx->context(),
+              property);
+    }
+
+    if (maybe_prop_attr.IsJust()) {
+      PropertyAttribute prop_attr = maybe_prop_attr.FromJust();
+      args.GetReturnValue().Set(prop_attr);
     }
   }
 
 
   static void GlobalPropertyDeleterCallback(
-      Local<String> property,
+      Local<Name> property,
       const PropertyCallbackInfo<Boolean>& args) {
     Isolate* isolate = args.GetIsolate();
     HandleScope scope(isolate);
 
     ContextifyContext* ctx =
         Unwrap<ContextifyContext>(args.Data().As<Object>());
+    Local<Object> sandbox = PersistentToLocal(isolate, ctx->sandbox_);
+
+    Maybe<bool> success = sandbox->Delete(ctx->context(), property);
 
-    bool success = PersistentToLocal(isolate,
-                                     ctx->sandbox_)->Delete(property);
-    args.GetReturnValue().Set(success);
+    if (success.IsJust())
+      args.GetReturnValue().Set(success.FromJust());
   }
 
 
diff --git a/test/parallel/test-vm-symbols.js b/test/parallel/test-vm-symbols.js
new file mode 100644
index 00000000000000..d7d0ffe3af6157
--- /dev/null
+++ b/test/parallel/test-vm-symbols.js
@@ -0,0 +1,25 @@
+'use strict';
+
+var common = require('../common');
+var assert = require('assert');
+
+var vm = require('vm');
+
+var symbol = Symbol();
+
+function Document() {
+  this[symbol] = 'foo';
+}
+
+Document.prototype.getSymbolValue = function() {
+  return this[symbol];
+};
+
+var context = new Document();
+vm.createContext(context);
+
+assert.equal(context.getSymbolValue(), 'foo',
+    'should return symbol-keyed value from the outside');
+
+assert.equal(vm.runInContext('this.getSymbolValue()', context), 'foo',
+    'should return symbol-keyed value from the inside');