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: fix crash on fatal error in debug context #1229

Merged
merged 1 commit into from
Mar 22, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 2 additions & 4 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,8 @@ class Environment {
inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; }
inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; }

static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX;

private:
static const int kIsolateSlot = NODE_ISOLATE_SLOT;

Expand All @@ -482,10 +484,6 @@ class Environment {
inline ~Environment();
inline IsolateData* isolate_data() const;

enum ContextEmbedderDataIndex {
kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX
};

v8::Isolate* const isolate_;
IsolateData* const isolate_data_;
uv_check_t immediate_check_handle_;
Expand Down
24 changes: 23 additions & 1 deletion src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -233,10 +233,32 @@ class ContextifyContext {


static void RunInDebugContext(const FunctionCallbackInfo<Value>& args) {
// Ensure that the debug context has an Environment assigned in case
// a fatal error is raised. The fatal exception handler in node.cc
// is not equipped to deal with contexts that don't have one and
// can't easily be taught that due to a deficiency in the V8 API:
// there is no way for the embedder to tell if the data index is
// in use.
struct ScopedEnvironment {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just assign it? Is there any point in rolling it back?

Copy link
Member Author

Choose a reason for hiding this comment

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

Two reasons:

  1. The environment doesn't really belong on the debug context. This is a hack to stop it from crashing until we have better ways to deal with this.
  2. It introduces a potential use-after-free because the pointer stays around when the non-debug context is disposed.

ScopedEnvironment(Local<Context> context, Environment* env)
: context_(context) {
const int index = Environment::kContextEmbedderDataIndex;
context->SetAlignedPointerInEmbedderData(index, env);
}
~ScopedEnvironment() {
const int index = Environment::kContextEmbedderDataIndex;
context_->SetAlignedPointerInEmbedderData(index, nullptr);
}
Local<Context> context_;
};

Local<String> script_source(args[0]->ToString(args.GetIsolate()));
if (script_source.IsEmpty())
return; // Exception pending.
Context::Scope context_scope(Debug::GetDebugContext());
Local<Context> debug_context = Debug::GetDebugContext();
Environment* env = Environment::GetCurrent(args);
ScopedEnvironment env_scope(debug_context, env);
Context::Scope context_scope(debug_context);
Local<Script> script = Script::Compile(script_source);
if (script.IsEmpty())
return; // Exception pending.
Expand Down
4 changes: 4 additions & 0 deletions test/fixtures/vm-run-in-debug-context.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
if (process.argv[2] === 'handle-fatal-exception')
process._fatalException = process.exit.bind(null, 42);

require('vm').runInDebugContext('*');
23 changes: 23 additions & 0 deletions test/parallel/test-vm-debug-context.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
var common = require('../common');
var assert = require('assert');
var vm = require('vm');
var spawn = require('child_process').spawn;

assert.throws(function() {
vm.runInDebugContext('*');
Expand All @@ -25,3 +26,25 @@ assert.strictEqual(vm.runInDebugContext(), undefined);
assert.strictEqual(vm.runInDebugContext(0), 0);
assert.strictEqual(vm.runInDebugContext(null), null);
assert.strictEqual(vm.runInDebugContext(undefined), undefined);

// See https://github.com/iojs/io.js/issues/1190, fatal errors should not
// crash the process.
var script = common.fixturesDir + '/vm-run-in-debug-context.js';
var proc = spawn(process.execPath, [script]);
var data = [];
proc.stdout.on('data', assert.fail);
proc.stderr.on('data', data.push.bind(data));
proc.once('exit', common.mustCall(function(exitCode, signalCode) {
assert.equal(exitCode, 1);
assert.equal(signalCode, null);
var haystack = Buffer.concat(data).toString('utf8');
assert(/SyntaxError: Unexpected token \*/.test(haystack));
}));

var proc = spawn(process.execPath, [script, 'handle-fatal-exception']);
proc.stdout.on('data', assert.fail);
proc.stderr.on('data', assert.fail);
proc.once('exit', common.mustCall(function(exitCode, signalCode) {
assert.equal(exitCode, 42);
assert.equal(signalCode, null);
}));