Repository navigation
vm: recent contextify changes cause segmentation fault in node v5.9 #5768
Description
A recent change made to node_contextify.cc that was released in node 5.9.0 causes node to segfault.
The offending commit is this: bfff07b
The problem can be reproduced using the following test script:
var vm = require('vm');
function f() {
var sandbox = {};
vm.createContext(sandbox);
return function(script, ctx) {
var s = new vm.Script(script);
for(var p in ctx) { sandbox[p] = ctx[p]; };
var result = s.runInContext(sandbox);
console.log(result, sandbox);
for(var p in sandbox) { delete sandbox[p]; };
}
}
for(var i=0; i<10000; i++) {
f()('x = 3', {x : 1});
f()('x = 4', {x : 2});
}$ node -v
v5.9.0
$ uname -a
Darwin raymond-117.local 15.3.0 Darwin Kernel Version 15.3.0: Thu Dec 10 18:40:58 PST 2015; root:xnu-3248.30.4~1/RELEASE_X86_64 x86_64cc @ofrobots and @bnoordhuis
Activity
ofrobots commentedon Mar 17, 2016
I can reproduce this with 5.9.0. This doesn't seem to fail on
master. I'm investigating.ofrobots commentedon Mar 18, 2016
This should fail on
mastertoo, but it is dependent on GC timing. I am working on a fix.ofrobots commentedon Mar 18, 2016
Here's a minimal test-case:
Associating the newly created V8 context with the contextified sandbox in a gc-visible fashion would be the easiest way to solve this problem, and I am trying to figure out a way to do that.
In any case, I want to think through the solution carefully as the object graph here is a bit complex. In the meanwhile, if a fix is more urgently needed, it may be worth reverting commits from #5392 in 5.9.1 (/cc @evanlucas).
evanlucas commentedon Mar 18, 2016
@ofrobots I almost think we should revert and push out v5.9.1 pretty quick. I'm open to other opinions though. /cc @nodejs/release
ofrobots commentedon Mar 18, 2016
Agree (and sorry for the churn).
cjihrig commentedon Mar 18, 2016
@ofrobots any idea how long a proper fix will take? I was considering making a known issue test with your reproduction from #5768 (comment)
ofrobots commentedon Mar 18, 2016
Work-in-progress fix: ofrobots@95d45ce.
cjihrig commentedon Mar 18, 2016
OK, looks like you've got it under control :-)
contextify: tie lifetimes of context & sandbox