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

fix: disable sigaltstack on Android #901

Merged
merged 3 commits into from
Nov 10, 2023
Merged

fix: disable sigaltstack on Android #901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe7df4f
fix: disable sigaltstack on android
supervacuus Nov 10, 2023
3c7f3f8
changelog
supervacuus Nov 10, 2023
71937b7
Merge branch 'master' into fix/disable_sigaltstack_on_android
supervacuus Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

**Fixes**:

- Disable sigaltstack on Android ([#901](https://github.com/getsentry/sentry-native/pull/901))
- Prevent stuck crashpad-client on Windows ([#902](https://github.com/getsentry/sentry-native/pull/902), [crashpad#89](https://github.com/getsentry/crashpad/pull/89))

## 0.6.6
Expand Down
35 changes: 33 additions & 2 deletions src/backends/sentry_backend_inproc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,31 @@
#include "transports/sentry_disk_transport.h"
#include <string.h>

/**
* Android's bionic libc seems to allocate alternate signal handler stacks for
* every thread and also references them from their internal maintenance
* structs.
*
* The way we currently set up our sigaltstack seems to interfere with this
* setup and causes crashes whenever an ART signal handler touches the thread
* that called `sentry_init()`.
*
* In addition to this problem, it also means there is no need for our own
* sigaltstack on Android since our signal handler will always be running on
* an alternate stack managed by bionic.
*
* Note: In bionic the sigaltstacks for 32-bit devices have a size of 16KiB and
* on 64-bit devices they have 32KiB. The size of our own was set to 64KiB
* independent of the device. If this is a problem, we need figure out
* together with Google if there is a way in which our configs can coexist.
*
* Both breakpad and crashpad are way more defensive in the setup of their
* signal stacks and take existing stacks into account (or reuse them).
*/
#ifndef SENTRY_PLATFORM_ANDROID
# define SETUP_SIGALTSTACK
#endif

#define SIGNAL_DEF(Sig, Desc) \
{ \
Sig, #Sig, Desc \
Expand All @@ -32,8 +57,9 @@ struct signal_slot {
# define SIGNAL_STACK_SIZE 65536
static struct sigaction g_sigaction;
static struct sigaction g_previous_handlers[SIGNAL_COUNT];
# ifdef SETUP_SIGALTSTACK
static stack_t g_signal_stack;

# endif
static const struct signal_slot SIGNAL_DEFINITIONS[SIGNAL_COUNT] = {
SIGNAL_DEF(SIGILL, "IllegalInstruction"),
SIGNAL_DEF(SIGTRAP, "Trap"),
Expand Down Expand Up @@ -87,14 +113,15 @@ startup_inproc_backend(
}

// install our own signal handler
# ifdef SETUP_SIGALTSTACK
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;
Copy link
Member

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?

Copy link
Collaborator Author

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.

Expand All @@ -107,10 +134,12 @@ startup_inproc_backend(
static void
shutdown_inproc_backend(sentry_backend_t *UNUSED(backend))
{
# ifdef SETUP_SIGALTSTACK
g_signal_stack.ss_flags = SS_DISABLE;
sigaltstack(&g_signal_stack, 0);
sentry_free(g_signal_stack.ss_sp);
g_signal_stack.ss_sp = NULL;
# endif
reset_signal_handlers();
}

Expand Down Expand Up @@ -472,6 +501,8 @@ make_signal_event(
void *backtrace[MAX_FRAMES];
size_t frame_count
= sentry_unwind_stack_from_ucontext(uctx, &backtrace[0], MAX_FRAMES);
SENTRY_TRACEF(
"captured backtrace from ucontext with %lu frames", frame_count);
// if unwinding from a ucontext didn't yield any results, try again with a
// direct unwind. this is most likely the case when using `libbacktrace`,
// since that does not allow to unwind from a ucontext at all.
Expand Down