-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
node: reset signal handler to SIG_DFL on FreeBSD #1218
Conversation
FreeBSD has a nasty bug with SA_RESETHAND reseting the SA_SIGINFO, that is in turn set for a libthr wrapper. This leads to a crash. Work around the issue by manually setting SIG_DFL in the signal handler. Fix: nodejs/node-v0.x-archive#9326
Also, cc @trevnorris @jbergstroem |
Can confirm that it fixes the segfault. I need to read up in the manpages before calling it a bug though. |
@indutny You mention SA_SIGINFO. Does that mean that with |
Yeah, basically Considering that this behavior is fixed in FreeBSD master branch - I suppose it should be a bug after all :) Looks like I forgot to give a link: freebsd/freebsd-src@4501dad |
And a test case from node.js issue: #include <assert.h>
#include <pthread.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
static void handler(int sig, siginfo_t* info, void* u) {
fprintf(stderr, "oh %d %p %p\n", sig, info, u);
fflush(stderr);
raise(sig);
}
int main() {
int r;
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
sa.sa_sigaction = handler;
sa.sa_flags = SA_RESETHAND | SA_SIGINFO;
sigfillset(&sa.sa_mask);
r = sigaction(SIGINT, &sa, NULL);
assert(r == 0);
kill(getpid(), SIGINT);
return 0;
} |
NOTE: SA_SIGINFO is used internally in libthr, and libthr overrides libc's |
@bnoordhuis sorry, got your question work. It is the |
Right, I understand now. I assume this is going to be fixed in 10.2? Perhaps we can simply wait it out and note it in the known issues list until then. I'm not really a fan of adding hacks for OS bugs, they tend to stick around for a long time. |
struct sigaction sa; | ||
memset(&sa, 0, sizeof(sa)); | ||
sa.sa_handler = SIG_DFL; | ||
sigfillset(&sa.sa_mask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you have to call sigfillset() here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@bnoordhuis the thing is that io.js is crashing at SIGINT on FreeBSD at the moment. I'm quite sure we should fix this and revert once FreeBSD guys will reply to this and if they'll tell us that they are going to backport the fix. |
Well, alright. LGTM with nit. |
@bnoordhuis may I ask you to land it? I'm on a terrible internet atm. |
@indutny thanks for the link. What about FreeBSD systems that has the patch (assume it lands or you're on -current)? I have a -current but it actually fails on v8; need to investigate. Edit: another v8 bug. I'll send it upstream. |
@@ -0,0 +1,2 @@ | |||
// NOTE: Was crashing on FreeBSD | |||
process.kill(process.pid, 'SIGINT'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing .js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/350/ with a fixes |
it seems that the patch is already in stable/10, shame on me :( But it wasn't released yet. @jbergstroem what do you mean by "similar interrupt"? Works or broken? |
Huh, crashes on CI... WTF, it works just fine in a vbox. @jbergstroem does this patch work for you? |
Oh, very interesting. It crashes when I run it with |
Ahaha, I think it is just related to the way test runner checks the exit code :) No actual crash is happening. |
Another CI run: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/351/ should work fine now. |
Yay, appears to be passing now! |
@jbergstroem @bnoordhuis : could you PTAL and LGTY? |
@indutny What I meant was that the signal emitted was similar between a "fixed" and "broken" FreeBSD: exiting non-null. With catching exit code in the test it LGTM. |
FreeBSD has a nasty bug with SA_RESETHAND reseting the SA_SIGINFO,
that is in turn set for a libthr wrapper. This leads to a crash.
Work around the issue by manually setting SIG_DFL in the signal
handler.
Fix: nodejs/node-v0.x-archive#9326
R=@bnoordhuis