vm: Copy missing properties from context

This addresses a current 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.

Fix #6416

Author: isaacs

src/node_contextify.cc

@@ -96,6 +96,79 @@ class ContextifyContext {
   }
 
 
+  // 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(node_isolate);
+
+    Local<Context> context = PersistentToLocal(env()->isolate(), context_);
+    Local<Object> global = context->Global()->GetPrototype()->ToObject();
+    Local<Object> sandbox = PersistentToLocal(env()->isolate(), 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();
+      bool has = sandbox->HasOwnProperty(key);
+      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(node_isolate,
+              "(function cloneProperty(source, key, target) {\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<String> fname = FIXED_ONE_BYTE_STRING(node_isolate,
+              "binding:script");
+          Local<Script> script = Script::Compile(code, fname);
+          clone_property_method = Local<Function>::Cast(script->Run());
+          assert(clone_property_method->IsFunction());
+        }
+        Local<Value> args[] = { global, key, sandbox };
+        clone_property_method->Call(global, ARRAY_SIZE(args), args);
+      }
+    }
+  }
+
+
   // This is an object that just keeps an internal pointer to this
   // ContextifyContext.  It's passed to the NamedPropertyHandler.  If we
   // pass the main JavaScript context object we're embedded in, then the
@@ -421,6 +494,7 @@ class ContextifyScript : public WeakObject {
                 display_errors,
                 args,
                 try_catch);
+    contextify_context->CopyProperties();
   }
 
   static int64_t GetTimeoutArg(const FunctionCallbackInfo<Value>& args,

test/simple/test-vm-function-declaration.js

@@ -0,0 +1,47 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+
+var vm = require('vm');
+var o = vm.createContext({ console: console });
+
+// This triggers the setter callback in node_contextify.cc
+var code = 'var a = function() {};\n';
+
+// but this does not, since function decls are defineProperties,
+// not simple sets.
+code += 'function b(){}\n';
+
+// Grab the global b function as the completion value, to ensure that
+// we are getting the global function, and not some other thing
+code += '(function(){return this})().b;\n'
+
+var res = vm.runInContext(code, o, 'test');
+
+assert.equal(typeof res, 'function', 'result should be function');
+assert.equal(res.name, 'b', 'res should be named b');
+assert.equal(typeof o.a, 'function', 'a should be function');
+assert.equal(typeof o.b, 'function', 'b should be function');
+assert.equal(res, o.b, 'result should be global b function');
+
+console.log('ok');

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

@@ -0,0 +1,46 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+
+var vm = require('vm');
+
+var code =
+    'Object.defineProperty(this, "f", {\n' +
+    '  get: function() { return x; },\n' +
+    '  set: function(k) { x = k; },\n' +
+    '  configurable: true,\n' +
+    '  enumerable: true\n' +
+    '});\n' +
+    'g = f;\n' +
+    'f;\n';
+
+var x = {};
+var o = vm.createContext({ console: console, x: x });
+
+var res = vm.runInContext(code, o, 'test');
+
+assert(res);
+assert.equal(typeof res, 'object');
+assert.equal(res, x);
+assert.equal(o.f, res);
+assert.deepEqual(Object.keys(o), ['console', 'x', 'g', 'f']);