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

runtime: debug output for fatal signal during signal handling. #50952

Open
prattmic opened this issue Feb 1, 2022 · 4 comments
Open

runtime: debug output for fatal signal during signal handling. #50952

prattmic opened this issue Feb 1, 2022 · 4 comments
Labels
NeedsDecision
Milestone

Comments

@prattmic
Copy link
Member

@prattmic prattmic commented Feb 1, 2022

#50936 is a crash due to SIGSEGV while in the SIGPROF signal handler. Since our signal handlers have all signals masked, the SIGSEGV is handled via the default termination action by the kernel, exit via signal 11. This results in no output to stderr at all to aid in debugging.

I think we should try to get a bit more debug info out of such crashes.

One approach may be to unmask some signals during signal handling to detect nested signals. Something like:

func sighandler(...) {
  if getg().m.insig {
    // Also print critical siginfo details: PC, addr
    throw("signal during signal handling")
  }
  
  getg().m.insig = true
  unblocksig(SIGSEGV, SIGBUS, SIGILL, SIGFPE, etc)
  ...

  // Reset insig, mask (does kernel do this?).
}

This approach keeps all signals masked in the sigaction, which limits the risk of infinite signal recursion. The downside being an extra syscall required in the signal handler.

(Edit: What we should do with C signal handlers is unclear. Do we unmask prior to calling a C signal handler? (I'd lean against this)).

cc @aclements @cherrymui @mknyszek

@prattmic prattmic added this to the Go1.19 milestone Feb 1, 2022
@bcmills
Copy link
Member

@bcmills bcmills commented Feb 1, 2022

One approach may be to unmask some signals during signal handling to detect nested signals.

I would worry somewhat about blowing out the signal stack in that case, but I suppose that if we're going to terminate the process either way a dump from a corrupted stack might be preferable to no dump at all.

@aclements
Copy link
Member

@aclements aclements commented Feb 1, 2022

I would worry somewhat about blowing out the signal stack in that case

We would definitely want to abort hard the moment we get a nested signal. I like @prattmic 's idea of waiting to unblock the signals until after we've checked for a nested signal because it ensures we can't go deeper than two signals.

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 1, 2022

What we should do with C signal handlers is unclear. Do we unmask prior to calling a C signal handler?

In general we should call pthread_sigmask or equivalent to make the signal mask match the sa_mask with which the C handler was registered. (We probably do not do that today. If we don't, I consider that a bug.)

@prattmic
Copy link
Member Author

@prattmic prattmic commented Feb 1, 2022

I would worry somewhat about blowing out the signal stack in that case, but I suppose that if we're going to terminate the process either way a dump from a corrupted stack might be preferable to no dump at all.

I worried about this as well, but looking at the (Linux) kernel source, I'm pretty sure that if you are already executing on the sigaltstack then signal delivery does not reset SP to the top of the sigaltstack (it just adds to the bottom of the stack as if sigaltstack is disabled). I'd need to double check this in a real program.

So the stack itself wouldn't get corrupted. Though, if we do a normal throw() with full stack trace [1], I suspect we won't actually include the original signal stack trace in the output without more changes to the traceback code.

[1] I'm not sure this is a great idea, but since signals will be masked, failure should result in immediate termination, so give it a shot I guess?

@toothrot toothrot added the NeedsDecision label Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision
Projects
None yet
Development

No branches or pull requests

4 participants