From 3b021efe11ea6828ed8c856167fc6f5e20b18fcc 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 c5976a56ec5f6c..ae043eb376a115 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();
 
@@ -350,22 +356,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();
@@ -378,7 +388,7 @@ class ContextifyContext {
 
 
   static void GlobalPropertyQueryCallback(
-      Local<String> property,
+      Local<Name> property,
       const PropertyCallbackInfo<Integer>& args) {
     Isolate* isolate = args.GetIsolate();
 
@@ -386,34 +396,38 @@ 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();
 
     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');