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.

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

vm: recent contextify changes cause segmentation fault in node v5.9 #5768

Closed
raymondfeng opened this issue Mar 17, 2016 · 8 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. vm Issues and PRs related to the vm subsystem.

Comments

@raymondfeng
Copy link
Contributor

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_64

cc @ofrobots and @bnoordhuis

@evanlucas evanlucas added the confirmed-bug Issues with confirmed bugs. label Mar 17, 2016
@ofrobots
Copy link
Contributor

I can reproduce this with 5.9.0. This doesn't seem to fail on master. I'm investigating.

@ofrobots
Copy link
Contributor

This should fail on master too, but it is dependent on GC timing. I am working on a fix.

@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Mar 18, 2016
@ofrobots
Copy link
Contributor

Here's a minimal test-case:

// Flags: --expose-gc
var sandbox = {x: 1};
vm.createContext(sandbox);
gc();
vm.runInContext("x = 2", sandbox);

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
Copy link
Contributor

@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
Copy link
Contributor

Agree (and sorry for the churn).

@cjihrig
Copy link
Contributor

cjihrig commented 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
Copy link
Contributor

Work-in-progress fix: ofrobots@95d45ce.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 18, 2016

OK, looks like you've got it under control :-)

ofrobots added a commit to ofrobots/node that referenced this issue Mar 19, 2016
When the previous set of changes (bfff07b) it was possible to have the
context get garbage collected while sandbox was still live. We need to
tie the lifetime of the context to the lifetime of the sandbox.

Fixes: nodejs#5768
PR-URL: nodejs#5786
@ofrobots ofrobots mentioned this issue Mar 19, 2016
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants