Skip to content

Commit

Permalink
vm: fix property descriptors of sandbox properties
Browse files Browse the repository at this point in the history
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: #864
Fixes: #885
PR-URL: #1773
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
  • Loading branch information
domenic authored and rvagg committed Jul 24, 2015
1 parent bfac906 commit d019cb3
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 4 deletions.
14 changes: 10 additions & 4 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -406,10 +407,15 @@ class ContextifyContext {
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) {
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);
}
}
Expand Down
25 changes: 25 additions & 0 deletions test/parallel/test-vm-preserves-property.js
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 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');

0 comments on commit d019cb3

Please sign in to comment.