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

node: reset signal handler to SIG_DFL on FreeBSD #1218

Closed
wants to merge 6 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Mar 20, 2015

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

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
@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

Also, cc @trevnorris @jbergstroem

@jbergstroem
Copy link
Member

Can confirm that it fixes the segfault. I need to read up in the manpages before calling it a bug though.

@bnoordhuis
Copy link
Member

@indutny You mention SA_SIGINFO. Does that mean that with SA_SIGINFO | SA_RESETHAND it does work?

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

Yeah, basically SA_RESETHAND resets to default signal implementation and this resets all other flags, including SA_SIGINFO. So the handler is invoked as there was no SA_SIGINFO.

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

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

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;
}

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

NOTE: SA_SIGINFO is used internally in libthr, and libthr overrides libc's sigaction when linking with -pthread.

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

@bnoordhuis sorry, got your question work. It is the SA_SIGINFO | SA_RESETHAND that does not work, both of them works fine when using separately.

@bnoordhuis
Copy link
Member

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);
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

@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.

@bnoordhuis
Copy link
Member

Well, alright. LGTM with nit.

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

@bnoordhuis may I ask you to land it? I'm on a terrible internet atm.

@jbergstroem
Copy link
Member

@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.
Edit2: After fixing the bug, I can say that it returns a similar interrupt.

@@ -0,0 +1,2 @@
// NOTE: Was crashing on FreeBSD
process.kill(process.pid, 'SIGINT');
Copy link
Contributor

Choose a reason for hiding this comment

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

missing .js

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

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?

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

Huh, crashes on CI... WTF, it works just fine in a vbox. @jbergstroem does this patch work for you?

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

Oh, very interesting. It crashes when I run it with gmake test, works just fine when I run directly.

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

Ahaha, I think it is just related to the way test runner checks the exit code :) No actual crash is happening.

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

Another CI run: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/351/ should work fine now.

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

Yay, appears to be passing now!

@indutny
Copy link
Member Author

indutny commented Mar 20, 2015

@jbergstroem @bnoordhuis : could you PTAL and LGTY?

@jbergstroem
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants