Proof of Concept: intercept defineProperty()

A lot of contextify issues seem to be related to the fact that we
cannot intercept defineProperty(), see
nodejs#6283.

Here is a proof of concept implementation that gets rid of the
CopyProperties() hack in contextify. For simplicty, only getters from the
descriptors are copied. Also, function declarations are not intercepted,
but that should be easy to do.

It'll be a while until I get this cleanly into V8, but I think once the
V8 API allows for intercepting defineProperty() and function declarations,
a lot of contextify issues can be solved.

deps/v8/include/v8.h

@@ -4294,6 +4294,13 @@ typedef void (*GenericNamedPropertyEnumeratorCallback)(
     const PropertyCallbackInfo<Array>& info);
 
 
+/**
+ * Returns a descriptor used in defineProperty().
+ */
+typedef void (*GenericNamedPropertyDefinerCallback)(
+    Local<Name> key, Local<Value> desc, const PropertyCallbackInfo<Value>& info);
+
+
 /**
  * Returns the value of the property if the getter intercepts the
  * request.  Otherwise, returns an empty handle.
@@ -4601,13 +4608,15 @@ struct NamedPropertyHandlerConfiguration {
       GenericNamedPropertyQueryCallback query = 0,
       GenericNamedPropertyDeleterCallback deleter = 0,
       GenericNamedPropertyEnumeratorCallback enumerator = 0,
+      GenericNamedPropertyDefinerCallback definer = 0,
       Local<Value> data = Local<Value>(),
       PropertyHandlerFlags flags = PropertyHandlerFlags::kNone)
       : getter(getter),
         setter(setter),
         query(query),
         deleter(deleter),
         enumerator(enumerator),
+        definer(definer),
         data(data),
         flags(flags) {}
 
@@ -4616,6 +4625,7 @@ struct NamedPropertyHandlerConfiguration {
   GenericNamedPropertyQueryCallback query;
   GenericNamedPropertyDeleterCallback deleter;
   GenericNamedPropertyEnumeratorCallback enumerator;
+  GenericNamedPropertyDefinerCallback definer;
   Local<Value> data;
   PropertyHandlerFlags flags;
 };

deps/v8/src/api.cc

@@ -1482,11 +1482,12 @@ void ObjectTemplate::SetAccessor(v8::Local<Name> name,
 
 
 template <typename Getter, typename Setter, typename Query, typename Deleter,
-          typename Enumerator>
+          typename Enumerator, typename Definer>
 static void ObjectTemplateSetNamedPropertyHandler(ObjectTemplate* templ,
                                                   Getter getter, Setter setter,
                                                   Query query, Deleter remover,
                                                   Enumerator enumerator,
+                                                  Definer definer,
                                                   Local<Value> data,
                                                   PropertyHandlerFlags flags) {
   i::Isolate* isolate = Utils::OpenHandle(templ)->GetIsolate();
@@ -1503,6 +1504,7 @@ static void ObjectTemplateSetNamedPropertyHandler(ObjectTemplate* templ,
   if (query != 0) SET_FIELD_WRAPPED(obj, set_query, query);
   if (remover != 0) SET_FIELD_WRAPPED(obj, set_deleter, remover);
   if (enumerator != 0) SET_FIELD_WRAPPED(obj, set_enumerator, enumerator);
+  if (definer != 0) SET_FIELD_WRAPPED(obj, set_definer, definer);
   obj->set_can_intercept_symbols(
       !(static_cast<int>(flags) &
         static_cast<int>(PropertyHandlerFlags::kOnlyInterceptStrings)));
@@ -1524,7 +1526,7 @@ void ObjectTemplate::SetNamedPropertyHandler(
     NamedPropertyQueryCallback query, NamedPropertyDeleterCallback remover,
     NamedPropertyEnumeratorCallback enumerator, Local<Value> data) {
   ObjectTemplateSetNamedPropertyHandler(
-      this, getter, setter, query, remover, enumerator, data,
+      this, getter, setter, query, remover, enumerator, nullptr, data,
       PropertyHandlerFlags::kOnlyInterceptStrings);
 }
 
@@ -1533,7 +1535,7 @@ void ObjectTemplate::SetHandler(
     const NamedPropertyHandlerConfiguration& config) {
   ObjectTemplateSetNamedPropertyHandler(
       this, config.getter, config.setter, config.query, config.deleter,
-      config.enumerator, config.data, config.flags);
+      config.enumerator, config.definer, config.data, config.flags);
 }
 
 

deps/v8/src/objects-inl.h

@@ -5495,6 +5495,7 @@ ACCESSORS(InterceptorInfo, setter, Object, kSetterOffset)
 ACCESSORS(InterceptorInfo, query, Object, kQueryOffset)
 ACCESSORS(InterceptorInfo, deleter, Object, kDeleterOffset)
 ACCESSORS(InterceptorInfo, enumerator, Object, kEnumeratorOffset)
+ACCESSORS(InterceptorInfo, definer, Object, kDefinerOffset)
 ACCESSORS(InterceptorInfo, data, Object, kDataOffset)
 SMI_ACCESSORS(InterceptorInfo, flags, kFlagsOffset)
 BOOL_ACCESSORS(InterceptorInfo, flags, can_intercept_symbols,

deps/v8/src/objects.cc

@@ -6458,6 +6458,52 @@ Maybe<bool> JSReceiver::OrdinaryDefineOwnProperty(Isolate* isolate,
     it.Next();
   }
 
+  for (; it.IsFound(); it.Next()) {
+    Handle<JSObject> holder = it.GetHolder<JSObject>();
+    Handle<Object> result;
+    Handle<Object> receiver = it.GetReceiver();
+
+    if (it.state() == LookupIterator::INTERCEPTOR) {
+      DCHECK_EQ(LookupIterator::INTERCEPTOR, it.state());
+      Handle<InterceptorInfo> interceptor = it.GetInterceptor();
+      if (interceptor->setter()->IsUndefined()) {
+        return OrdinaryDefineOwnProperty(&it, desc, should_throw);
+      }
+
+      PropertyCallbackArguments args(isolate, interceptor->data(), *receiver,
+                                     *holder, Object::DONT_THROW);
+      if (!it.IsElement()) {
+        Handle < Name > name = it.name();
+        DCHECK(!name->IsPrivate());
+
+
+        if (name->IsSymbol() && !interceptor->can_intercept_symbols()) {
+          return OrdinaryDefineOwnProperty(&it, desc, should_throw);
+        }
+
+        v8::GenericNamedPropertyDefinerCallback definePropertyCallback =
+            v8::ToCData<v8::GenericNamedPropertyDefinerCallback>(
+                interceptor->definer());
+
+        Handle < Object > descriptor = handle(*desc->ToObject(isolate),
+                                              isolate);
+
+        result = args.Call(definePropertyCallback, name, descriptor);
+      }
+    }
+  }
+
+  it.Restart();
+  // Deal with access checks first.
+  if (it.state() == LookupIterator::ACCESS_CHECK) {
+    if (!it.HasAccess()) {
+      isolate->ReportFailedAccessCheck(it.GetHolder<JSObject>());
+      RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<bool>());
+      return Just(true);
+    }
+    it.Next();
+  }
+
   return OrdinaryDefineOwnProperty(&it, desc, should_throw);
 }
 

deps/v8/src/objects.h

@@ -10556,6 +10556,7 @@ class InterceptorInfo: public Struct {
   DECL_ACCESSORS(query, Object)
   DECL_ACCESSORS(deleter, Object)
   DECL_ACCESSORS(enumerator, Object)
+  DECL_ACCESSORS(definer, Object)
   DECL_ACCESSORS(data, Object)
   DECL_BOOLEAN_ACCESSORS(can_intercept_symbols)
   DECL_BOOLEAN_ACCESSORS(all_can_read)
@@ -10575,7 +10576,8 @@ class InterceptorInfo: public Struct {
   static const int kQueryOffset = kSetterOffset + kPointerSize;
   static const int kDeleterOffset = kQueryOffset + kPointerSize;
   static const int kEnumeratorOffset = kDeleterOffset + kPointerSize;
-  static const int kDataOffset = kEnumeratorOffset + kPointerSize;
+  static const int kDefinerOffset = kEnumeratorOffset + kPointerSize;
+  static const int kDataOffset = kDefinerOffset + kPointerSize;
   static const int kFlagsOffset = kDataOffset + kPointerSize;
   static const int kSize = kFlagsOffset + kPointerSize;
 

src/node_contextify.cc

@@ -1,12 +1,8 @@
 #include "node.h"
-#include "node_internals.h"
 #include "node_watchdog.h"
-#include "base-object.h"
 #include "base-object-inl.h"
 #include "env.h"
-#include "env-inl.h"
 #include "util.h"
-#include "util-inl.h"
 #include "v8-debug.h"
 
 namespace node {
@@ -92,79 +88,6 @@ class ContextifyContext {
     return Local<Object>::Cast(context()->GetEmbedderData(kSandboxObjectIndex));
   }
 
-  // XXX(isaacs): This function only exists because of a shortcoming of
-  // the V8 SetNamedPropertyHandler function.
-  //
-  // It does not provide a way to intercept Object.defineProperty(..)
-  // calls.  As a result, these properties are not copied onto the
-  // contextified sandbox when a new global property is added via either
-  // a function declaration or a Object.defineProperty(global, ...) call.
-  //
-  // Note that any function declarations or Object.defineProperty()
-  // globals that are created asynchronously (in a setTimeout, callback,
-  // etc.) will happen AFTER the call to copy properties, and thus not be
-  // caught.
-  //
-  // The way to properly fix this is to add some sort of a
-  // Object::SetNamedDefinePropertyHandler() function that takes a callback,
-  // which receives the property name and property descriptor as arguments.
-  //
-  // Luckily, such situations are rare, and asynchronously-added globals
-  // weren't supported by Node's VM module until 0.12 anyway.  But, this
-  // should be fixed properly in V8, and this copy function should be
-  // removed once there is a better way.
-  void CopyProperties() {
-    HandleScope scope(env()->isolate());
-
-    Local<Context> context = PersistentToLocal(env()->isolate(), context_);
-    Local<Object> global =
-        context->Global()->GetPrototype()->ToObject(env()->isolate());
-    Local<Object> sandbox_obj = sandbox();
-
-    Local<Function> clone_property_method;
-
-    Local<Array> names = global->GetOwnPropertyNames();
-    int length = names->Length();
-    for (int i = 0; i < length; i++) {
-      Local<String> key = names->Get(i)->ToString(env()->isolate());
-      bool has = sandbox_obj->HasOwnProperty(context, key).FromJust();
-      if (!has) {
-        // Could also do this like so:
-        //
-        // PropertyAttribute att = global->GetPropertyAttributes(key_v);
-        // Local<Value> val = global->Get(key_v);
-        // sandbox->ForceSet(key_v, val, att);
-        //
-        // However, this doesn't handle ES6-style properties configured with
-        // Object.defineProperty, and that's exactly what we're up against at
-        // this point.  ForceSet(key,val,att) only supports value properties
-        // with the ES3-style attribute flags (DontDelete/DontEnum/ReadOnly),
-        // which doesn't faithfully capture the full range of configurations
-        // that can be done using Object.defineProperty.
-        if (clone_property_method.IsEmpty()) {
-          Local<String> code = FIXED_ONE_BYTE_STRING(env()->isolate(),
-              "(function cloneProperty(source, key, target) {\n"
-              "  if (key === 'Proxy') return;\n"
-              "  try {\n"
-              "    var desc = Object.getOwnPropertyDescriptor(source, key);\n"
-              "    if (desc.value === source) desc.value = target;\n"
-              "    Object.defineProperty(target, key, desc);\n"
-              "  } catch (e) {\n"
-              "   // Catch sealed properties errors\n"
-              "  }\n"
-              "})");
-
-          Local<Script> script =
-              Script::Compile(context, code).ToLocalChecked();
-          clone_property_method = Local<Function>::Cast(script->Run());
-          CHECK(clone_property_method->IsFunction());
-        }
-        Local<Value> args[] = { global, key, sandbox_obj };
-        clone_property_method->Call(global, arraysize(args), args);
-      }
-    }
-  }
-
 
   // This is an object that just keeps an internal pointer to this
   // ContextifyContext.  It's passed to the NamedPropertyHandler.  If we
@@ -200,7 +123,12 @@ class ContextifyContext {
                                              GlobalPropertyQueryCallback,
                                              GlobalPropertyDeleterCallback,
                                              GlobalPropertyEnumeratorCallback,
+                                             GlobalPropertyDefinerCallback,
                                              CreateDataWrapper(env));
+
+    // Whenever a property is accessed on objects created from this template,
+    // the provided callback is invoked, i.e., the property is changed
+    // on the sandbox object, too.
     object_template->SetHandler(config);
 
     Local<Context> ctx = Context::New(env->isolate(), nullptr, object_template);
@@ -327,11 +255,11 @@ class ContextifyContext {
   static ContextifyContext* ContextFromContextifiedSandbox(
       Environment* env,
       const Local<Object>& sandbox) {
-    auto maybe_value =
+    auto is_contextified =
         sandbox->GetPrivate(env->context(),
                             env->contextify_context_private_symbol());
     Local<Value> context_external_v;
-    if (maybe_value.ToLocal(&context_external_v) &&
+    if (is_contextified.ToLocal(&context_external_v) &&
         context_external_v->IsExternal()) {
       Local<External> context_external = context_external_v.As<External>();
       return static_cast<ContextifyContext*>(context_external->Value());
@@ -346,18 +274,14 @@ class ContextifyContext {
     ContextifyContext* ctx;
     ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
 
-    // Stil initializing
+    // Still initializing
     if (ctx->context_.IsEmpty())
       return;
 
     Local<Context> context = ctx->context();
     Local<Object> sandbox = ctx->sandbox();
     MaybeLocal<Value> maybe_rv =
         sandbox->GetRealNamedProperty(context, property);
-    if (maybe_rv.IsEmpty()) {
-      maybe_rv =
-          ctx->global_proxy()->GetRealNamedProperty(context, property);
-    }
 
     Local<Value> rv;
     if (maybe_rv.ToLocal(&rv)) {
@@ -376,7 +300,7 @@ class ContextifyContext {
     ContextifyContext* ctx;
     ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
 
-    // Stil initializing
+    // Still initializing
     if (ctx->context_.IsEmpty())
       return;
 
@@ -398,11 +322,6 @@ class ContextifyContext {
     Maybe<PropertyAttribute> maybe_prop_attr =
         ctx->sandbox()->GetRealNamedPropertyAttributes(context, property);
 
-    if (maybe_prop_attr.IsNothing()) {
-      maybe_prop_attr =
-          ctx->global_proxy()->GetRealNamedPropertyAttributes(context,
-                                                              property);
-    }
 
     if (maybe_prop_attr.IsJust()) {
       PropertyAttribute prop_attr = maybe_prop_attr.FromJust();
@@ -439,6 +358,28 @@ class ContextifyContext {
 
     args.GetReturnValue().Set(ctx->sandbox()->GetPropertyNames());
   }
+
+  static void GlobalPropertyDefinerCallback(
+      Local<Name> key,
+      Local<Value> desc,
+      const PropertyCallbackInfo<Value>& args) {
+    ContextifyContext* ctx;
+    ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
+
+    // Still initializing
+    if (ctx->context_.IsEmpty())
+      return;
+
+    Local<Context> context = ctx->context();
+    Local<Object> sandbox = ctx->sandbox();
+
+    MaybeLocal<Value> maybe_getter = Local<Object>::Cast(desc)
+      ->Get(context, String::NewFromUtf8(context->GetIsolate(), "get"));
+    Local<Function> getter = Local<Function>::Cast(
+        maybe_getter.ToLocalChecked());
+
+    sandbox->SetAccessorProperty(key, getter);
+  }
 };
 
 class ContextifyScript : public BaseObject {
@@ -606,14 +547,13 @@ class ContextifyScript : public BaseObject {
       TryCatch try_catch(env->isolate());
       // Do the eval within the context
       Context::Scope context_scope(contextify_context->context());
-      if (EvalMachine(contextify_context->env(),
+      EvalMachine(contextify_context->env(),
                       timeout,
                       display_errors,
                       break_on_sigint,
                       args,
-                      &try_catch)) {
-        contextify_context->CopyProperties();
-      }
+                      &try_catch);
+
 
       if (try_catch.HasCaught()) {
         try_catch.ReThrow();

test/parallel/test-vm-delete-property.js

@@ -0,0 +1,13 @@
+/* eslint-disable no-debugger */
+'use strict';
+require('../common');
+var assert = require('assert');
+var vm = require('vm');
+
+// https://github.com/nodejs/node/issues/6287
+
+const sbox = { };
+vm.createContext(sbox);
+vm.runInContext('this.x = "w00t";delete this.x;', sbox);
+
+assert.equal(sbox.x, undefined);

test/parallel/test-vm-global-define-property.js

@@ -23,4 +23,4 @@ assert(res);
 assert.equal(typeof res, 'object');
 assert.equal(res, x);
 assert.equal(o.f, res);
-assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'g', 'f']);
+assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'f', 'g']);