-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix signal handler #2152
Fix signal handler #2152
Conversation
@mholt - any chance you could try this against your original issue (before you fixed it)? |
Good question, I've moved the |
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.
LGTM 🚀 , just added a minor comment.
if (sigaction(signum, NULL, &st) < 0) { | ||
goto fix_signal_error; | ||
} | ||
st.sa_flags |= SA_ONSTACK; |
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.
Should we skip fixup the signal handler if the SA_ONSTACK
flag is already set?
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.
No harm in installing this one over it i guess? We know that either gtk or webkit do install one without SA_ONSTACK
so we should be good regardless.
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.
Yeah absolutely, would maybe just save some cycles for internal setting up the handler in the kernel when it's already fine.But that can be neglected 😄
@leaanthony I will try this out ASAP! Is it OK if I run with |
Thanks for this fix @leaanthony - This was helpful for me in solving a pesky bug in pact-go where we consume a rust shared library, in various languages including golang via cgo. The issue only crops up where the rust shared library calls out to external plugins and manages their process lifecycle. |
I'm glad it was useful 🙏 |
This PR sets up a new signal handler in C that overrides the current one (in C) so that
SA_ONSTACK
is used. When used against the nil dereference example @mholt gave, it appears to work as expected.Closes #2134