Skip to content

debug: unhandled exception from runInDebugContext causes segfault #1190

Closed
Closed
debug: unhandled exception from runInDebugContext causes segfault#1190
Labels
vmIssues and PRs related to the vm subsystem.

Description

@ofrobots
ofrobots
opened on Mar 18, 2015

The following test-case causes a segfault:

$ cat test1.js
require('vm').runInDebugContext('*');
$ ~/src/io.js/iojs test1.js
[1]    96829 segmentation fault  ~/src/io.js/iojs test1.js

With a debug build:

$ ~/src/io.js/iojs_g test1.js
FATAL ERROR: v8::Context::GetAlignedPointerFromEmbedderData() Index too large
[1]    97178 abort      ~/src/io.js/iojs_g test1.js

I think that the issue is that node::Environment has not been initialized for the debug context.

Here's a (truncated) stack-trace from the debug build:

* thread #1: tid = 0x1074ac, 0x00007fff96467286 libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x00007fff96467286 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff8ab3842f libsystem_pthread.dylib`pthread_kill + 90
    frame #2: 0x00007fff98192b53 libsystem_c.dylib`abort + 129
    frame #3: 0x000000010097200c iojs_g`node::OnFatalError(location=0x0000000100b06df9, message=0x0000000100b0ab07) + 124 at node.cc:2119
    frame #4: 0x00000001001fe002 iojs_g`v8::Utils::ReportApiFailure(location=0x0000000100b06df9, message=0x0000000100b0ab07) + 98 at api.cc:184
    frame #5: 0x00000001002223df iojs_g`v8::Utils::ApiCheck(condition=false, location=0x0000000100b06df9, message=0x0000000100b0ab07) + 47 at api.h:181
    frame #6: 0x00000001001ff627 iojs_g`v8::EmbedderDataFor(context=0x000000010204a3a8, index=32, can_grow=false, location=0x0000000100b06df9) + 279 at api.cc:570
    frame #7: 0x00000001001ff82f iojs_g`v8::Context::SlowGetAlignedPointerFromEmbedderData(this=0x000000010204a3a8, index=32) + 47 at api.cc:602
    frame #8: 0x000000010022321b iojs_g`v8::Context::GetAlignedPointerFromEmbedderData(this=0x000000010204a3a8, index=32) + 27 at v8.h:7440
    frame #9: 0x0000000100957815 iojs_g`node::Environment::GetCurrent(context=Local<v8::Context> at 0x00007fff5fbfe398) + 37 at env-inl.h:147
    frame #10: 0x00000001009572f2 iojs_g`node::Environment::GetCurrent(isolate=0x0000000102007600) + 34 at env-inl.h:142
    frame #11: 0x0000000100972040 iojs_g`node::FatalException(isolate=0x0000000102007600, error=(val_ = v8::Value * = 0x000000010204a380), message=(val_ = v8::Message * = 0x000000010204a370)) + 48 at node.cc:2135
    frame #12: 0x00000001009728e5 iojs_g`node::OnMessage(message=(val_ = v8::Message * = 0x000000010204a370), error=(val_ = v8::Value * = 0x000000010204a380)) + 53 at node.cc:2181
    frame #13: 0x00000001006aa677 iojs_g`v8::internal::MessageHandler::ReportMessage(isolate=0x0000000102007600, loc=0x00007fff5fbfe688, message=Handle<v8::internal::Object> at 0x00007fff5fbfe648) + 727 at messages.cc:117
    frame #14: 0x000000010064944d iojs_g`v8::internal::Isolate::ReportPendingMessages(this=0x0000000102007600) + 413 at isolate.cc:1384
    frame #15: 0x00000001003ef0d9 iojs_g`v8::internal::Compiler::CompileScript(source=Handle<v8::internal::String> at 0x00007fff5fbfe9d0,
...
...

Frame 9 is trying to get the node::Environment from the debug context, but that is going to fail, since we never Set EmbedderData in that Context.

Wrapping the test case in a try/catch works around the issue.

I tested with 1.5.2, 1.1.0 and 0.12.0. All of them crash as above.

Activity

  1. Fishrock123
    added
    debugger
    on Mar 18, 2015

  2. ofrobots commented on Mar 19, 2015

    @ofrobots
    ofrobots
    on Mar 19, 2015
    ContributorAuthor

    Here's a patch that fixes the segfault

    diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 6985a33..1a8bd07 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -233,10 +233,14 @@ class ContextifyContext {


   static void RunInDebugContext(const FunctionCallbackInfo<Value>& args) {
+    // TODO(ofrobots): maybe only do this once
+    Local<Context> context = Debug::GetDebugContext();
+    Environment::GetCurrent(args)->AssignToContext(context);
+
     Local<String> script_source(args[0]->ToString(args.GetIsolate()));
     if (script_source.IsEmpty())
       return;  // Exception pending.
-    Context::Scope context_scope(Debug::GetDebugContext());
+    Context::Scope context_scope(context);
     Local<Script> script = Script::Compile(script_source);
     if (script.IsEmpty())
       return;  // Exception pending.

    However, don't go merging it just yet. While the simplified test-case (above) does get fixed, the following slightly bigger test-case (that is closer to what I am trying to do) still segfaults.

    var vm = require('vm');
var Debug = vm.runInDebugContext('Debug');

Debug.setListener(function(evt, execState, eventData) {
  console.log('debug event ' + evt);
  if (evt === 1) {
    var mirror = execState.frame(0).evaluate('process.env');
    console.log(mirror.properties());
  }
});

debugger;

    This segfaults in a similar way:

    $ ~/src/io.js/iojs test2.js
debug event 1
debug event 5
[1]    85147 segmentation fault  ~/src/io.js/iojs ~/src/test/test2.js

    I suspect that there is yet another context active at this point.

    The crash occurs on mirror.properties(). The interesting thing about process.env is that it has a named interceptor. That's why we end up trying to fetch the Environment from the current Context, but we apparently haven't assigned the Environment to that Context yet (perhaps because it was created by V8).

    Here's what I think we need to do:

    • Make sure we can Assign the Environment in all Contexts that get created. or
    • Figure out a different way of squirreling away the Environment pointer.

    @indutny thoughts?

  3. Fishrock123
    added
    vmIssues and PRs related to the vm subsystem.
    on Mar 19, 2015

  4. ofrobots commented on Mar 20, 2015

    @ofrobots
    ofrobots
    on Mar 20, 2015
    ContributorAuthor

    This is related to nodejs/node-v0.x-archive#9156. I am trying to come up with a solution.
    /cc @indutny.

  5. ofrobots commented on Mar 21, 2015

    @ofrobots
    ofrobots
    on Mar 21, 2015
    ContributorAuthor

    @bnoordhuis Looking at commit 756b622, you might have some, ehm, context about this problem too.

    There are a bunch of contexts at play here. At the time RunInDebugContext is called, V8 creates the Debug context, but since there is no listener or message handler installed at this point, the context gets destructed before we return to RunInDebugContext. At the time the listener is installed, a new Debug context will be created by V8.

    We never had an opportunity to Assign node::Environment to that context.

    Here's my question: wouldn't it make more sense for node to associate node::Environment with the Isolate instead of the Context. Clearly a given node::Environment can be relevant to multiple contexts.

    Perhaps the fix is to store the Environment into Isolate::SetData(kIslolateSlot) – where Environment::IsolateData is presently stored, and make IsolateData an immediate field of Environment. I am not familiar enough with the lifetimes of Environment and Environment::IsolateData to know for sure if this matches the current semantics.

    Let me know if this makes sense. I can send a PR.
    EDIT: syntax.

  6. bnoordhuis commented on Mar 22, 2015

    @bnoordhuis
    bnoordhuis
    on Mar 22, 2015
    Member

    Thanks for the bug report, should be fixed by cf081a4.

  7. bnoordhuis
    closed this as completedon Mar 22, 2015

  8. ofrobots commented on Mar 22, 2015

    @ofrobots
    ofrobots
    on Mar 22, 2015
    ContributorAuthor

    The second testcase I posted still fails even with your fix.

    $ ./iojs_g ~/src/test/test2.js
debug event 1
debug event 5
FATAL ERROR: v8::Context::GetAlignedPointerFromEmbedderData() Index too large

    The testcase is in #1190 (comment).

  9. Fishrock123
    reopened this on Mar 22, 2015

  10. bnoordhuis commented on Mar 22, 2015

    @bnoordhuis
    bnoordhuis
    on Mar 22, 2015
    Member

    I'm not sure I agree that qualifies as an io.js bug. Let me adapt the test so it's a little easier to talk about:

    require('vm').runInDebugContext('Debug').setListener(ondebugevent);

function ondebugevent(evt, exc) {
  if (evt === 1) exc.frame(0).evaluate('process.env').properties();
}

function breakpoint() { debugger; }

breakpoint();

    ondebugevent runs in the debug context. exc.frame(0).evaluate('process.env') evaluates the process.env enumerator in the scope of breakpoint but without entering breakpoint's context first. I'm inclined to say that's a V8 bug.

    Anyway, that's why it segfaults, because node::EnvEnumerator() is called with the wrong context. It's called with v8::Isolate::GetCurrentContext() == v8::Debug::GetDebugContext().

    (v8::Isolate::GetCallingContext() and v8::Isolate::GetEnteredContext() return the right context but have problems of their own. The first is wrong when C++ enters a new scope with v8::Context::Scope, the second is wrong when there is a C++ -> JS -> C++ call sequence on the stack where the JS stack frame changes the active context.)

    But whatever, I think we can work around this quirk with a little effort. I'll open a PR in a few.

  11. bnoordhuis commented on Mar 23, 2015

    @bnoordhuis
    bnoordhuis
    on Mar 23, 2015
    Member

    Fixed again in 7e88a93. Please file new issues if this turns into a bug whack-a-mole.

  12. bnoordhuis
    closed this as completedon Mar 23, 2015

  13. ofrobots commented on Mar 23, 2015

    @ofrobots
    ofrobots
    on Mar 23, 2015
    ContributorAuthor

    Awesome! Thanks a lot for fixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    vmIssues and PRs related to the vm subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions