Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

vm: fix a variety of bugs, using V8 4.3 APIs #1773

Closed
wants to merge 3 commits into from
Closed

vm: fix a variety of bugs, using V8 4.3 APIs #1773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99104df
vm: fix property descriptors of sandbox properties
domenic May 22, 2015
6443054
vm: remove unnecessary access checks
domenic May 23, 2015
5f69cda
vm: fix symbol access
domenic Jun 1, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 43 additions & 41 deletions src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,15 @@ 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;
using v8::Persistent;
using v8::PropertyAttribute;
using v8::PropertyCallbackInfo;
using v8::Script;
using v8::ScriptCompiler;
Expand Down Expand Up @@ -201,14 +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->SetAccessCheckCallbacks(GlobalPropertyNamedAccessCheck,
GlobalPropertyIndexedAccessCheck);
object_template->SetHandler(config);

Local<Context> ctx = Context::New(env->isolate(), nullptr, object_template);
if (!ctx.IsEmpty())
Expand Down Expand Up @@ -342,24 +347,8 @@ class ContextifyContext {
}


static bool GlobalPropertyNamedAccessCheck(Local<Object> host,
Local<Value> key,
AccessType type,
Local<Value> data) {
return true;
}


static bool GlobalPropertyIndexedAccessCheck(Local<Object> host,
uint32_t key,
AccessType type,
Local<Value> data) {
return true;
}


static void GlobalPropertyGetterCallback(
Local<String> property,
Local<Name> property,
const PropertyCallbackInfo<Value>& args) {
Isolate* isolate = args.GetIsolate();
HandleScope scope(isolate);
Expand All @@ -368,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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can write this as:

if (!maybe_rv.ToLocal(&rv))
  return;

Saves a level of indent. Purely aesthetic though, I'll leave it up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of this but I kind of thought there was a prohibition against early return given some other code in this file. I'll leave it since otherwise I'd feel compelled to go and fix the other cases.

if (rv == ctx->sandbox_)
rv = PersistentToLocal(isolate, ctx->proxy_global_);

args.GetReturnValue().Set(rv);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside, is nothing / undefined / the hole the right value to return on error, here and elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure, but not positive, based on looking through similar V8 source code (which is buried in macros). I am a bit surprised that GetReturnValue().Set() does not take a Maybe...

}
}


static void GlobalPropertySetterCallback(
Local<String> property,
Local<Name> property,
Local<Value> value,
const PropertyCallbackInfo<Value>& args) {
Isolate* isolate = args.GetIsolate();
Expand All @@ -397,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);
Expand All @@ -406,30 +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_);

bool in_sandbox = sandbox->GetRealNamedProperty(property).IsEmpty();
bool in_proxy_global =
proxy_global->GetRealNamedProperty(property).IsEmpty();
if (!in_sandbox || !in_proxy_global) {
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this also be:

args.GetReturnValue().Set(success.FromMaybe(false));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not saying that's the preferred way. It's currently more explicit and easier for others scanning the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No; I don't want to return anything if success is a Nothing.

(It probably doesn't make a difference... would have to dig in to how V8 actually uses GlobalPropertyDeleterCallback. But in general I try to not return anything on failure in this maybe-world.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know. I was basing my assumption from the old code that always returned a bool.

}


Expand Down
25 changes: 25 additions & 0 deletions test/parallel/test-vm-preserves-property.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict';

var common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'use strict', maybe? Does it pass linter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing lint errors now, feeling dumb. Surprisingly none of the other tests have use strict but the linter still wants it.

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');
25 changes: 25 additions & 0 deletions test/parallel/test-vm-symbols.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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');