-
-
Notifications
You must be signed in to change notification settings - Fork 165
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: disable sigaltstack on Android #901
Conversation
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.
🔥
Codecov Report
@@ Coverage Diff @@
## master #901 +/- ##
==========================================
- Coverage 86.49% 82.59% -3.91%
==========================================
Files 40 53 +13
Lines 3674 7372 +3698
Branches 0 1185 +1185
==========================================
+ Hits 3178 6089 +2911
- Misses 496 1175 +679
- Partials 0 108 +108 |
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!
g_signal_stack.ss_sp = sentry_malloc(SIGNAL_STACK_SIZE); | ||
if (!g_signal_stack.ss_sp) { | ||
return 1; | ||
} | ||
g_signal_stack.ss_size = SIGNAL_STACK_SIZE; | ||
g_signal_stack.ss_flags = 0; | ||
sigaltstack(&g_signal_stack, 0); | ||
|
||
# endif | ||
sigemptyset(&g_sigaction.sa_mask); | ||
g_sigaction.sa_sigaction = handle_signal; | ||
g_sigaction.sa_flags = SA_SIGINFO | 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.
Just double checking: We still should set the SA_ONSTACK
flag, even if we don't allocate the altstack ourselves right?
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, SA_ONSTACK
tells the signal handler to use the alt stack or falls back to the process stack. Since the assumption is that every thread has an alt stack, we should be good.
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- disable sigaltstack on Android ([#901](https://github.com/getsentry/sentry-native/pull/901)) If none of the above apply, you can opt out of this check by adding |
Hello, our project has recently collected a lot of crashes related to Android signal handling. We happened to see this bug fix, which seems to be highly related to the crashes we encountered. However, I didn't find how this fix would cause a crash.
As far as I know, the stacks allocated by bionic are only referenced for deallocating which should be harmless.
Could you please provide more details about these crashes? I haven't found any evidence to suggest that setting up stacks repeatedly would lead to a crash. |
Hi @etnlGD,
The most significant change to our previous implementation is that the default
Yeah, the reference is not the reason for crashes, but rather the way the
The ART team urged us not to set up a This doesn't mean we won't touch the topic again, but it would have to be a non-default behavior where users have more control over which threads should be configured with that stack. |
I am creating this as a tracking branch for a pre-release. Disabling "our"
sigaltstack
on Android is probably not the solution that will end up in a release, but it should provide a sound basis for broader testing in the downstream SDKs.