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.

Sign up for GitHub

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

Jump to bottom

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

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

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

prattmic opened this issue Feb 1, 2022 · 5 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@prattmic
Copy link
Member

prattmic commented Feb 1, 2022 •
edited
Loading

#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
Contributor

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

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
Contributor

bcmills commented Feb 1, 2022 •
edited
Loading

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 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 Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 4, 2022
@ianlancetaylor
Copy link
Member

Rolling forward to 1.20. Please comment if you disagree. Thanks.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.19, Go1.20 Jun 24, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@prattmic prattmic moved this to Triage Backlog in Go Compiler / Runtime Jul 25, 2022
@mknyszek mknyszek moved this from Triage Backlog to Todo in Go Compiler / Runtime Nov 23, 2022
@mknyszek mknyszek modified the milestones: Go1.20, Go1.21 Nov 30, 2022
@mknyszek mknyszek modified the milestones: Go1.21, Backlog Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Go Compiler / Runtime
Status: Todo
Release Blockers
Status: No status
Milestone
Backlog
Development

No branches or pull requests
7 participants
@toothrot @mknyszek @prattmic @aclements @ianlancetaylor @bcmills @gopherbot