Skip to content

Segmentation faults with concurrent access? #104

Closed
Closed
Segmentation faults with concurrent access?#104

Description

@papandreou
papandreou
opened on Oct 7, 2013

Hello again!

While developing https://github.com/papandreou/node-gitfakefs I stumbled upon what seems to be a bug in nodegit caused by calling the library asynchronously without waiting for the previous operation to finish.

The problem is triggered by a specific test in my test suite, and I was able to get a backtrace with gdb:

andreas@andreas-ThinkPad-T500:~/work/node-gitfakefs$ gdb --args ./node_modules/mocha/bin/mocha
GNU gdb (Ubuntu/Linaro 7.4-2012.04-0ubuntu2.1) 7.4-2012.04
Copyright (C) 2012 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-linux-gnu".
For bug reporting instructions, please see:
<http://bugs.launchpad.net/gdb-linaro/>...
"/home/andreas/work/node-gitfakefs/node_modules/mocha/bin/mocha": not in executable format: File format not recognized
(gdb) run
Starting program: /home/andreas/.local/bin/node ./node_modules/mocha/bin/mocha
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/i386-linux-gnu/libthread_db.so.1".
[New Thread 0xb7fdab40 (LWP 32315)]

  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․
Program received signal SIGSEGV, Segmentation fault.
0xb7fdd424 in __kernel_vsyscall ()
(gdb) backtrace
#0  0xb7fdd424 in __kernel_vsyscall ()
#1  0xb7cf2526 in kill () from /lib/i386-linux-gnu/libc.so.6
#2  0x08206efc in uv_kill (pid=32314, signum=11) at ../deps/uv/src/unix/process.c:434
#3  0x081a1031 in node::Kill(v8::Arguments const&) ()
#4  0x083100d9 in v8::internal::Builtin_HandleApiCall(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) ()
#5  0x4360a336 in ?? ()
#6  0x4362a97c in ?? ()
#7  0x50f12c3c in ?? ()
#8  0x4360db41 in ?? ()
#9  0x4362d328 in ?? ()
#10 0x4360db41 in ?? ()
#11 0x43621c32 in ?? ()
#12 0x43612c2a in ?? ()
#13 0x0833021d in v8::internal::Invoke(bool, v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, bool*) ()
#14 0x08331962 in v8::internal::Execution::Call(v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, bool*, bool) ()
#15 0x082f396e in v8::Function::Call(v8::Handle<v8::Object>, int, v8::Handle<v8::Value>*) ()
#16 0x081a2d45 in node::EmitExit(v8::Handle<v8::Object>) ()
#17 0x081a3fe2 in node::Start(int, char**) ()
#18 0x081986bb in main ()

To reproduce it, clone papandreou/node-gitfakefs@613d0f16, npm link it against nodegit master, then run: mocha -g glob. That causes the segmentation fault at least half of the time.

The test has a lot of moving parts. I tried to narrow it down further, but I've been unsuccessful so far. I wanted to raise the issue here before spending more time on it, as one of you guys might immediately go ah-hah! when seeing that stacktrace.

The reason why I know it's related to concurrent access is that I found a workaround in papandreou/node-gitfakefs@358eac4: It's wrapping accesses to nodegit in a mutex.

While debugging the issue I've also seen these weird errors, which seem to bear witness to some kind of memory corruption:

  1) GitWrappedFs patching the built-in fs module "in-place" should make the glob work on the contents directory:
     Error: @��@��e 'refs/heads/d35056a9711a2497e8c6a957fa6f0d13cb94f8b1' not found

  1) GitWrappedFs patching the built-in fs module "in-place" should make the glob work on the contents directory:
     Error: Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/packed-refs': No such file or directory

Let me know if there's anything else I can do at this point.

Activity

  1. nkallen commented on Oct 8, 2013

    @nkallen
    nkallen
    on Oct 8, 2013
    Contributor

    Well, at a minimum this is easy to reproduce -- both the segfault and the nondeterministic tests. The backtrace I got is

    #0  0x00007fff8f2e86b0 in strlen ()
(gdb) backtrace
#0  0x00007fff8f2e86b0 in strlen ()
#1  0x000000010014612d in v8::String::New ()
#2  0x000000010210c934 in GitRepo::GetReferenceAfterWork (req=<value temporarily unavailable, due to optimizations>) at ../src/repo.cc:901
#3  0x000000010012d4cd in uv__work_done ()
#4  0x00000001001241d3 in uv__async_event ()
#5  0x000000010012433f in uv__async_io ()
#6  0x0000000100130ffb in uv__io_poll ()
#7  0x0000000100124ddc in uv_run ()
#8  0x0000000100009cbf in node::Start ()
#9  0x00000001000018f4 in start ()

  2. nkallen commented on Oct 8, 2013

    @nkallen
    nkallen
    on Oct 8, 2013
    Contributor

    Off the top of my head, I don't know what's going. If you could provide a smaller test case, that would make this much easier to debug.

    The backtrace above is from

    Exception::Error(String::New(baton->error->message))

    The error comes from

      if (result != GIT_OK) {
    baton->error = giterr_last();
  }

    and giterr_last() is implemented thus:

    const git_error *giterr_last(void)
{
        return GIT_GLOBAL->last_error;
}

    So setting baton->error like this is OBVIOUSLY WRONG, since it will later cross a thread boundary. I need to duplicate the git_error struct. This is not hard to fix.

    However, my guess is this is not the only problem going on with your example.

  3. nkallen commented on Oct 8, 2013

    @nkallen
    nkallen
    on Oct 8, 2013
    Contributor

    I'm really busy this week. If you could be a dear and fix this one yourself I'd be forever grateful. The convention is to create a "dup" function in copy.cc. Then just change baton->error = giterr_last() to baton->error = giterr_dup(giterr_last()) in asyncFunction.cc.ejs. Note that you'll need to dup the string error message as well.

  4. papandreou commented on Oct 8, 2013

    @papandreou
    papandreou
    on Oct 8, 2013
    ContributorAuthor

    Thanks for looking into this!

    If you could be a dear and fix this one yourself I'd be forever grateful

    Did that in #105. As you predicted it doesn't fix the original issue, though. I'm not sure when I'll have the time to reduce the test case (I have a workaround). Do you have the slightest hunch about what it could be related to?

  5. nkallen commented on Oct 9, 2013

    @nkallen
    nkallen
    on Oct 9, 2013
    Contributor

    I can no longer reproduce the segfault. Feel free to reopen if there is still a problem.

  6. nkallen
    closed this as completedon Oct 9, 2013

  7. papandreou commented on Oct 9, 2013

    @papandreou
    papandreou
    on Oct 9, 2013
    ContributorAuthor

    Unfortunately it still happens for me with nodegit master with papandreou/node-gitfakefs@613d0f1. I've since split it up into two projects, https://github.com/papandreou/node-gitfakefs and https://github.com/papandreou/node-fsplusgit, and the offending test is now in the latter repository. It seems to have stopped segfaulting, though, but "old" error messages seem to show up when unrelated methods are called later:

    andreas@andreas-ThinkPad-T500:~/work/node-fsplusgit$ mocha -g glob

  ․glob error [Error: Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/refs/heads/53fe6bbfa6a15f88356aab7bf6240c920604e470': No such file or directory]
․glob error [Error: Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/refs/heads/53fe6bbfa6a15f88356aab7bf6240c920604e470': No such file or directory]
glob error [Error: Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/refs/heads/a1410fe23cc0c126d5546a366545a88e1d649759': No such file or directory]
glob error [Error: Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/refs/heads/a1410fe23cc0c126d5546a366545a88e1d649759': No such file or directory]
glob error [Error: Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/refs/tags/39c5c09d660b1bac8eb66898e88f72907ccbb223': No such file or directory]
glob error [Error: Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/refs/tags/39c5c09d660b1bac8eb66898e88f72907ccbb223': No such file or directory]
glob error [Error: Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/packed-refs': No such file or directory]
glob error [Error: Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/packed-refs': No such file or directory]


  0 passing (156ms)
  2 failing

  1) FsPlusGit patching the built-in fs module "in-place" should make the glob work on the gitFakeFs directory:
     Error: Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/refs/heads/53fe6bbfa6a15f88356aab7bf6240c920604e470': No such file or directory


  2) FsPlusGit patching the built-in fs module "in-place" should make the glob work on the gitFakeFs directory:
     Error: Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/refs/heads/53fe6bbfa6a15f88356aab7bf6240c920604e470': No such file or directory

    The Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/refs/heads/a1410fe23cc0c126d5546a366545a88e1d649759': No such file or directory error must originally have happened because of this code that tries to be clever and detect whether treeish refers to a branch, a tag, or a commit: https://github.com/papandreou/node-gitfakefs/blob/c1354e9c711d6661d348e88d22a6761b77eb2210/lib/GitFakeFs.js#L111-L149

    I'm on a 32 bit Ubuntu, could that be significant?

    andreas@andreas-ThinkPad-T500:~$ uname -a
Linux andreas-ThinkPad-T500 3.2.0-54-generic-pae #82-Ubuntu SMP Tue Sep 10 20:29:22 UTC 2013 i686 i686 i386 GNU/Linux
  8. nkallen
    reopened this on Oct 9, 2013

  9. nkallen commented on Oct 9, 2013

    @nkallen
    nkallen
    on Oct 9, 2013
    Contributor

    I just checked out your code. I can't reproduce it anymore. It can't be a platform difference??! I dunno man. Can you triple check that you have a totally clean build? Like blow everything away and install https://github.com/papandreou/node-fsplusgit; don't use npm link, etc?

  10. nkallen commented on Oct 9, 2013

    @nkallen
    nkallen
    on Oct 9, 2013
    Contributor

    Also, when you run tests, why does glob run twice?? Mine only runs once:

    192:node-fsplusgit nkallen$ mocha -g glob

  ․

  1 passing (197ms)

  11. papandreou commented on Oct 10, 2013

    @papandreou
    papandreou
    on Oct 10, 2013
    ContributorAuthor

    Ah, sorry, my bad. You're right. I did screw up the npm linking. I've released my fork as nodegit-papandreou in order to get those new index entry features, and when I tested it I was using that instead of nodegit@master. After cleaning that up I can confirm that your fixes do work, and I've just removed the mutex construct without any incident.

    Thanks a lot!

  12. papandreou
    closed this as completedon Oct 10, 2013

  13. papandreou commented on Oct 10, 2013

    @papandreou
    papandreou
    on Oct 10, 2013
    ContributorAuthor

    Also, when you run tests, why does glob run twice??

    Hmm, yeah, thanks for mentioning that. It went away after applying your fixes, but it kinda does indicate a bug somewhere. I might be calling a callback twice in some error handling code.

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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions