Repository navigation
Segmentation faults with concurrent access? #104
Description
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
nkallen commentedon Oct 8, 2013
Well, at a minimum this is easy to reproduce -- both the segfault and the nondeterministic tests. The backtrace I got is
nkallen commentedon Oct 8, 2013
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
The error comes from
and
giterr_last()is implemented thus:So setting baton->error like this is OBVIOUSLY WRONG, since it will later cross a thread boundary. I need to duplicate the
git_errorstruct. This is not hard to fix.However, my guess is this is not the only problem going on with your example.
nkallen commentedon Oct 8, 2013
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 changebaton->error = giterr_last()tobaton->error = giterr_dup(giterr_last())inasyncFunction.cc.ejs. Note that you'll need to dup the string error message as well.papandreou commentedon Oct 8, 2013
Thanks for looking into this!
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?
nkallen commentedon Oct 9, 2013
I can no longer reproduce the segfault. Feel free to reopen if there is still a problem.
papandreou commentedon Oct 9, 2013
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:
The
Failed to open '/home/andreas/work/node-gitfakefs/test/testrepo.git/refs/heads/a1410fe23cc0c126d5546a366545a88e1d649759': No such file or directoryerror must originally have happened because of this code that tries to be clever and detect whethertreeishrefers to a branch, a tag, or a commit: https://github.com/papandreou/node-gitfakefs/blob/c1354e9c711d6661d348e88d22a6761b77eb2210/lib/GitFakeFs.js#L111-L149I'm on a 32 bit Ubuntu, could that be significant?
nkallen commentedon Oct 9, 2013
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?
nkallen commentedon Oct 9, 2013
Also, when you run tests, why does glob run twice?? Mine only runs once:
papandreou commentedon Oct 10, 2013
Ah, sorry, my bad. You're right. I did screw up the
npm linking. I've released my fork asnodegit-papandreouin 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!
papandreou commentedon Oct 10, 2013
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.