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

feat: introduce optional handler strategy #1027

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 38 additions & 0 deletions include/sentry.h
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,14 @@ typedef enum {
SENTRY_USER_CONSENT_REVOKED = 0,
} sentry_user_consent_t;

/**
* The crash handler strategy.
*/
typedef enum {
SENTRY_HANDLER_STRATEGY_DEFAULT = 0,
SENTRY_HANDLER_STRATEGY_CHAIN_AT_START = 1,
} sentry_handler_strategy_t;

/**
* Creates a new options struct.
* Can be freed with `sentry_options_free`.
Expand Down Expand Up @@ -1479,6 +1487,36 @@ SENTRY_EXPERIMENTAL_API void sentry_options_set_traces_sample_rate(
SENTRY_EXPERIMENTAL_API double sentry_options_get_traces_sample_rate(
sentry_options_t *opts);

#ifdef SENTRY_PLATFORM_LINUX

/**
* Returns the currently set strategy for the handler.
*
* This option does only work for the `inproc` backend on `Linux` and `Android`.
*
* The main use-case is when the Native SDK is used in the context of the
* CLR/Mono runtimes which convert some POSIX signals into managed-code
* exceptions and discontinue the signal chain.
*
* If this happens and we invoke the previous handler at the end (i.e., after
* our handler processed the signal, which is the default strategy) we will end
* up sending a native crash in addition to the managed code exception (which
* will either generate another crash-event if uncaught or could be handled in
* the managed code and neither terminate the application nor create a crash
* event). To correctly process the signals of CLR/Mono applications, we must
* invoke the runtime handler at the start of our handler.
*/
SENTRY_EXPERIMENTAL_API sentry_handler_strategy_t
sentry_options_get_handler_strategy(const sentry_options_t *opts);

/**
* Sets the handler strategy.
*/
SENTRY_EXPERIMENTAL_API void sentry_options_set_handler_strategy(
sentry_options_t *opts, sentry_handler_strategy_t handler_strategy);

#endif // SENTRY_PLATFORM_LINUX

/* -- Session APIs -- */

typedef enum {
Expand Down
59 changes: 34 additions & 25 deletions src/backends/sentry_backend_inproc.c
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,6 @@
#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).
*/
#define SIGNAL_DEF(Sig, Desc) \
{ \
Sig, #Sig, Desc \
Expand Down Expand Up @@ -558,6 +537,10 @@ handle_ucontext(const sentry_ucontext_t *uctx)

#ifdef SENTRY_PLATFORM_UNIX
// give us an allocator we can use safely in signals before we tear down.
// TODO: for `SENTRY_HANDLER_STRATEGY_CHAIN_AT_START` to work together with
// our page allocator we must find a way to maintain memory usage.
// We might be able to move this "enable" far enough down, because I
// think we do not allocate before we hit `make_signal_event()`.
sentry__page_allocator_enable();

// inform the sentry_sync system that we're in a signal handler. This will
Expand All @@ -566,12 +549,36 @@ handle_ucontext(const sentry_ucontext_t *uctx)
sentry__enter_signal_handler();
#endif

sentry_value_t event = make_signal_event(sig_slot, uctx);
#ifdef SENTRY_PLATFORM_UNIX
sentry_handler_strategy_t handler_strategy
= SENTRY_HANDLER_STRATEGY_DEFAULT;
#endif

SENTRY_WITH_OPTIONS (options) {
sentry__write_crash_marker(options);
#ifdef SENTRY_PLATFORM_LINUX
handler_strategy = sentry_options_get_handler_strategy(options);

// CLR/Mono convert signals provoked by "managed" native code into
// managed code exceptions. In these cases, we shouldn't react to
// the signal at all and let their handler discontinue the signal
// chain by invoking the runtime handler before we process the signal.
if (handler_strategy == SENTRY_HANDLER_STRATEGY_CHAIN_AT_START) {
// there is a good chance that we won't return from the previous
// handler and that would mean we couldn't enter this handler with
// the next signal coming in if we didn't "leave" here.
sentry__leave_signal_handler();

invoke_signal_handler(
uctx->signum, uctx->siginfo, (void *)uctx->user_context);

// let's re-enter because it means this was an actual native crash
sentry__enter_signal_handler();
}
#endif

sentry_value_t event = make_signal_event(sig_slot, uctx);
bool should_handle = true;
sentry__write_crash_marker(options);

if (options->on_crash_func) {
SENTRY_TRACE("invoking `on_crash` hook");
Expand Down Expand Up @@ -613,8 +620,10 @@ handle_ucontext(const sentry_ucontext_t *uctx)
// forward as we're not restoring the page allocator.
reset_signal_handlers();
sentry__leave_signal_handler();
invoke_signal_handler(
uctx->signum, uctx->siginfo, (void *)uctx->user_context);
if (handler_strategy == SENTRY_HANDLER_STRATEGY_DEFAULT) {
invoke_signal_handler(
uctx->signum, uctx->siginfo, (void *)uctx->user_context);
}
#endif
}

Expand Down
18 changes: 18 additions & 0 deletions src/sentry_options.c
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ sentry_options_new(void)
opts->shutdown_timeout = SENTRY_DEFAULT_SHUTDOWN_TIMEOUT;
opts->traces_sample_rate = 0.0;
opts->max_spans = 0;
opts->handler_strategy = SENTRY_HANDLER_STRATEGY_DEFAULT;

return opts;
}
Expand Down Expand Up @@ -594,3 +595,20 @@ sentry_options_set_backend(sentry_options_t *opts, sentry_backend_t *backend)
sentry__backend_free(opts->backend);
opts->backend = backend;
}

#ifdef SENTRY_PLATFORM_LINUX

sentry_handler_strategy_t
sentry_options_get_handler_strategy(const sentry_options_t *opts)
{
return opts->handler_strategy;
}

void
sentry_options_set_handler_strategy(
sentry_options_t *opts, sentry_handler_strategy_t handler_strategy)
{
opts->handler_strategy = handler_strategy;
}

#endif // SENTRY_PLATFORM_LINUX
1 change: 1 addition & 0 deletions src/sentry_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ typedef struct sentry_options_s {
long user_consent;
long refcount;
uint64_t shutdown_timeout;
sentry_handler_strategy_t handler_strategy;
} sentry_options_t;

/**
Expand Down
Loading