From 99104dff716263ad1b97bf46541e552e49d66e67 Mon Sep 17 00:00:00 2001 From: Domenic Denicola Date: Fri, 22 May 2015 19:41:28 -0400 Subject: [PATCH 1/3] vm: fix property descriptors of sandbox properties The GlobalPropertyQueryCallback was changed in 2010 to return an integer instead of a boolean: https://groups.google.com/forum/#!topic/v8-users/OOjHJrix-cU This integer communicates the property descriptors of the property, instead of just its presence or absence. However, the original contextify code was probably written before this change, and it was not updated when porting to Node.js. Credit to @smikes for the test and the original PR of #885. Fixes #885; fixes #864. --- src/node_contextify.cc | 14 ++++++++---- test/parallel/test-vm-preserves-property.js | 25 +++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-vm-preserves-property.js diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 81d0388a35f390..c4eeebae135496 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -30,6 +30,7 @@ using v8::None; using v8::Object; using v8::ObjectTemplate; using v8::Persistent; +using v8::PropertyAttribute; using v8::PropertyCallbackInfo; using v8::Script; using v8::ScriptCompiler; @@ -409,10 +410,15 @@ class ContextifyContext { Local proxy_global = PersistentToLocal(isolate, ctx->proxy_global_); - bool in_sandbox = sandbox->GetRealNamedProperty(property).IsEmpty(); - bool in_proxy_global = - proxy_global->GetRealNamedProperty(property).IsEmpty(); - if (!in_sandbox || !in_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); } } diff --git a/test/parallel/test-vm-preserves-property.js b/test/parallel/test-vm-preserves-property.js new file mode 100644 index 00000000000000..85ce2d6e8165c8 --- /dev/null +++ b/test/parallel/test-vm-preserves-property.js @@ -0,0 +1,25 @@ +'use strict'; + +var common = require('../common'); +var assert = require('assert'); + +var vm = require('vm'); + +var x = {}; +Object.defineProperty(x, 'prop', { + configurable: false, + enumerable: false, + writable: false, + value: 'val' +}); +var o = vm.createContext(x); + +var code = 'Object.getOwnPropertyDescriptor(this, "prop")'; +var res = vm.runInContext(code, o, 'test'); + +assert(res); +assert.equal(typeof res, 'object'); +assert.equal(res.value, 'val'); +assert.equal(res.configurable, false, 'should not be configurable'); +assert.equal(res.enumerable, false, 'should not be enumerable'); +assert.equal(res.writable, false, 'should not be writable'); From 64430544fb751d6eb077810310d878be58b56539 Mon Sep 17 00:00:00 2001 From: Domenic Denicola Date: Fri, 22 May 2015 20:02:20 -0400 Subject: [PATCH 2/3] vm: remove unnecessary access checks No reason to install access checks if they're always going to return true. --- src/node_contextify.cc | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index c4eeebae135496..6c05e78d2b88ea 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -208,8 +208,6 @@ class ContextifyContext { GlobalPropertyDeleterCallback, GlobalPropertyEnumeratorCallback, CreateDataWrapper(env)); - object_template->SetAccessCheckCallbacks(GlobalPropertyNamedAccessCheck, - GlobalPropertyIndexedAccessCheck); Local ctx = Context::New(env->isolate(), nullptr, object_template); if (!ctx.IsEmpty()) @@ -343,22 +341,6 @@ class ContextifyContext { } - static bool GlobalPropertyNamedAccessCheck(Local host, - Local key, - AccessType type, - Local data) { - return true; - } - - - static bool GlobalPropertyIndexedAccessCheck(Local host, - uint32_t key, - AccessType type, - Local data) { - return true; - } - - static void GlobalPropertyGetterCallback( Local property, const PropertyCallbackInfo& args) { From 5f69cda3065f5cea0438cfbe17ab3ab5721a5171 Mon Sep 17 00:00:00 2001 From: Domenic Denicola Date: Mon, 1 Jun 2015 12:22:19 -0400 Subject: [PATCH 3/3] 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 #884. --- 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 object_template = function_template->InstanceTemplate(); - object_template->SetNamedPropertyHandler(GlobalPropertyGetterCallback, + + NamedPropertyHandlerConfiguration config(GlobalPropertyGetterCallback, GlobalPropertySetterCallback, GlobalPropertyQueryCallback, GlobalPropertyDeleterCallback, GlobalPropertyEnumeratorCallback, CreateDataWrapper(env)); + object_template->SetHandler(config); Local ctx = Context::New(env->isolate(), nullptr, object_template); if (!ctx.IsEmpty()) @@ -342,7 +348,7 @@ class ContextifyContext { static void GlobalPropertyGetterCallback( - Local property, + Local property, const PropertyCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); HandleScope scope(isolate); @@ -351,22 +357,26 @@ class ContextifyContext { Unwrap(args.Data().As()); Local sandbox = PersistentToLocal(isolate, ctx->sandbox_); - Local rv = sandbox->GetRealNamedProperty(property); - if (rv.IsEmpty()) { + MaybeLocal maybe_rv = + sandbox->GetRealNamedProperty(ctx->context(), property); + if (maybe_rv.IsEmpty()) { Local 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 rv; + if (maybe_rv.ToLocal(&rv)) { + if (rv == ctx->sandbox_) + rv = PersistentToLocal(isolate, ctx->proxy_global_); + + args.GetReturnValue().Set(rv); + } } static void GlobalPropertySetterCallback( - Local property, + Local property, Local value, const PropertyCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); @@ -380,7 +390,7 @@ class ContextifyContext { static void GlobalPropertyQueryCallback( - Local property, + Local property, const PropertyCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); HandleScope scope(isolate); @@ -389,35 +399,39 @@ class ContextifyContext { Unwrap(args.Data().As()); Local sandbox = PersistentToLocal(isolate, ctx->sandbox_); - Local 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 maybe_prop_attr = + sandbox->GetRealNamedPropertyAttributes(ctx->context(), property); + + if (maybe_prop_attr.IsNothing()) { + Local 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 property, + Local property, const PropertyCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); HandleScope scope(isolate); ContextifyContext* ctx = Unwrap(args.Data().As()); + Local sandbox = PersistentToLocal(isolate, ctx->sandbox_); + + Maybe 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');