Skip to content

Commit

Permalink
vm: add isContext; prevent double-contextifying
Browse files Browse the repository at this point in the history
Previously, calling `vm.createContext(o)` repeatedly on the same `o`
would cause new C++ `ContextifyContext`s to be created and stored on
`o`, while the previous resident went off into leaked-memory limbo.
Now, repeatedly trying to contextify a sandbox will do nothing after
the first time.

To detect this, an independently-useful `vm.isContext(sandbox)` export
was added.
  • Loading branch information
domenic authored and bnoordhuis committed Aug 28, 2013
1 parent a3bf3d1 commit 9c110d8
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 3 deletions.
6 changes: 5 additions & 1 deletion lib/vm.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ var util = require('util');
// - runInThisContext()
// - runInContext(sandbox, [timeout])
// - makeContext(sandbox)
// - isContext(sandbox)
// From this we build the entire documented API.

Script.prototype.runInNewContext = function(initSandbox, timeout, disp) {
Expand All @@ -44,10 +45,11 @@ exports.createScript = function(code, filename, disp) {
exports.createContext = function(initSandbox) {
if (util.isUndefined(initSandbox)) {
initSandbox = {};
} else if (binding.isContext(initSandbox)) {
return initSandbox;
}

binding.makeContext(initSandbox);

return initSandbox;
};

Expand All @@ -65,3 +67,5 @@ exports.runInThisContext = function(code, filename, timeout, disp) {
var script = exports.createScript(code, filename, disp);
return script.runInThisContext(timeout, disp);
};

exports.isContext = binding.isContext;
23 changes: 22 additions & 1 deletion src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ class ContextifyContext {
data_wrapper_ctor.Reset(node_isolate, function_template->GetFunction());

NODE_SET_METHOD(target, "makeContext", MakeContext);
NODE_SET_METHOD(target, "isContext", IsContext);
}


Expand All @@ -130,11 +131,31 @@ class ContextifyContext {
}
Local<Object> sandbox = args[0].As<Object>();

Local<String> hidden_name =
FIXED_ONE_BYTE_STRING(node_isolate, "_contextifyHidden");

// Don't allow contextifying a sandbox multiple times.
assert(sandbox->GetHiddenValue(hidden_name).IsEmpty());

ContextifyContext* context = new ContextifyContext(sandbox);
Local<External> hidden_context = External::New(context);
sandbox->SetHiddenValue(hidden_name, hidden_context);
}


static void IsContext(const FunctionCallbackInfo<Value>& args) {
HandleScope scope(node_isolate);

if (!args[0]->IsObject()) {
ThrowTypeError("sandbox must be an object");
return;
}
Local<Object> sandbox = args[0].As<Object>();

Local<String> hidden_name =
FIXED_ONE_BYTE_STRING(node_isolate, "_contextifyHidden");
sandbox->SetHiddenValue(hidden_name, hidden_context);

args.GetReturnValue().Set(!sandbox->GetHiddenValue(hidden_name).IsEmpty());
}


Expand Down
8 changes: 7 additions & 1 deletion test/simple/test-vm-create-context-arg.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,15 @@ var vm = require('vm');

assert.throws(function() {
var ctx = vm.createContext('string is not supported');
});
}, TypeError);

assert.doesNotThrow(function() {
var ctx = vm.createContext({ a: 1 });
ctx = vm.createContext([0, 1, 2, 3]);
});

assert.doesNotThrow(function() {
var sandbox = {};
vm.createContext(sandbox);
vm.createContext(sandbox);
});
38 changes: 38 additions & 0 deletions test/simple/test-vm-is-context.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// 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');

assert.throws(function() {
vm.isContext('string is not supported');
}, TypeError);

assert.strictEqual(vm.isContext({}), false);
assert.strictEqual(vm.isContext([]), false);

assert.strictEqual(vm.isContext(vm.createContext()), true);
assert.strictEqual(vm.isContext(vm.createContext([])), true);

var sandbox = { foo: 'bar' };
vm.createContext(sandbox);
assert.strictEqual(vm.isContext(sandbox), true);

0 comments on commit 9c110d8

Please sign in to comment.