Skip to content
Open
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
**Internal:**

- Support downstream Xbox SDK specifying networking initialization mechanism ([#1359](https://github.com/getsentry/sentry-native/pull/1359))
- Add before crash hook to allow modifying other callbacks before crash handling ([#1366](https://github.com/getsentry/sentry-native/pull/1366))

## 0.10.1

Expand Down
30 changes: 30 additions & 0 deletions include/sentry.h
Original file line number Diff line number Diff line change
Expand Up @@ -989,6 +989,36 @@ typedef sentry_value_t (*sentry_event_function_t)(
SENTRY_API void sentry_options_set_before_send(
sentry_options_t *opts, sentry_event_function_t func, void *user_data);

/**
* Type of the `before_crash` callback.
*
* The `before_crash` callback is invoked immediately when a crash is detected,
* before any crash processing begins. This provides an opportunity to perform
* cleanup or disable logging operations at the earliest possible moment during
* crash handling.
*
* Unlike `on_crash`, this callback does not receive an event object and is not
* intended for event filtering or modification. It is purely for performing
* early crash-time operations like flushing logs, closing files, or other
* cleanup tasks.
*
* The callback has a void return type as it is not intended to filter or
* modify crash events.
*
* This function may be invoked inside of a signal handler and must be safe for
* that purpose, see https://man7.org/linux/man-pages/man7/signal-safety.7.html.
* On Windows, it may be called from inside of a `UnhandledExceptionFilter`.
*/
typedef void (*sentry_before_crash_function_t)(void *user_data);

/**
* Sets the `before_crash` callback.
*
* See the `sentry_before_crash_function_t` typedef above for more information.
*/
SENTRY_API void sentry_options_set_before_crash(sentry_options_t *opts,
sentry_before_crash_function_t func, void *user_data);

/**
* Type of the `on_crash` callback.
*
Expand Down
6 changes: 6 additions & 0 deletions src/backends/sentry_backend_breakpad.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ breakpad_backend_callback(const google_breakpad::MinidumpDescriptor &descriptor,
void *UNUSED(context), bool succeeded)
#endif
{
SENTRY_WITH_OPTIONS (options) {
if (options->before_crash_func) {
options->before_crash_func(options->before_crash_data);
}
}

SENTRY_INFO("entering breakpad minidump callback");

// this is a bit strange, according to docs, `succeeded` should be true when
Expand Down
7 changes: 7 additions & 0 deletions src/backends/sentry_backend_crashpad.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,13 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context)
{
sentry__page_allocator_enable();
# endif

SENTRY_WITH_OPTIONS (options) {
if (options->before_crash_func) {
options->before_crash_func(options->before_crash_data);
}
}

SENTRY_INFO("flushing session and queue before crashpad handler");

bool should_dump = true;
Expand Down
6 changes: 6 additions & 0 deletions src/backends/sentry_backend_inproc.c
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,12 @@ make_signal_event(
static void
handle_ucontext(const sentry_ucontext_t *uctx)
{
SENTRY_WITH_OPTIONS (options) {
if (options->before_crash_func) {
options->before_crash_func(options->before_crash_data);
}
Comment on lines +528 to +529

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential bug: The before_crash_func is called from a signal handler before the SDK's signal-safety mechanisms are enabled via sentry__enter_signal_handler(), bypassing existing protections.
  • Description: The new before_crash_func is invoked from within a signal handler context in backends like inproc, breakpad, and crashpad. However, it is called before the SDK's own signal-safety mechanisms are enabled via sentry__enter_signal_handler(). This function is responsible for setting up spinlocks and a special page allocator to ensure subsequent operations are async-signal-safe. By calling the user-provided function before this setup, the SDK bypasses its own protections. This is inconsistent with existing callbacks like on_crash_func, which are called after the safety mechanisms are active. If a user's before_crash_func performs any non-async-signal-safe operations, it will likely lead to a deadlock or memory corruption, preventing the original crash from being reported.

  • Suggested fix: Move the invocation of before_crash_func to be after the call to sentry__enter_signal_handler() in all relevant backends (sentry_backend_inproc.c, sentry_backend_breakpad.cpp, and sentry_backend_crashpad.cpp). This ensures the SDK's signal-safety infrastructure is active before executing user-provided code, consistent with other crash callbacks.
    severity: 0.85, confidence: 0.9

Did we get this right? 👍 / 👎 to inform future reviews.

}

SENTRY_INFO("entering signal handler");

const struct signal_slot *sig_slot = NULL;
Expand Down
8 changes: 8 additions & 0 deletions src/sentry_options.c
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,14 @@ sentry_options_set_before_send(
opts->before_send_data = user_data;
}

void
sentry_options_set_before_crash(sentry_options_t *opts,
sentry_before_crash_function_t func, void *user_data)
{
opts->before_crash_func = func;
opts->before_crash_data = user_data;
}

void
sentry_options_set_on_crash(
sentry_options_t *opts, sentry_crash_function_t func, void *user_data)
Expand Down
2 changes: 2 additions & 0 deletions src/sentry_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ struct sentry_options_s {
void *on_crash_data;
sentry_transaction_function_t before_transaction_func;
void *before_transaction_data;
sentry_before_crash_function_t before_crash_func;
void *before_crash_data;

/* Experimentally exposed */
double traces_sample_rate;
Expand Down
1 change: 1 addition & 0 deletions tests/unit/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ add_executable(sentry_test_unit
sentry_testsupport.h
test_attachments.c
test_basic.c
test_before_crash.c
test_consent.c
test_concurrency.c
test_embedded_info.c
Expand Down
110 changes: 110 additions & 0 deletions tests/unit/test_before_crash.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
#include "sentry_core.h"
#include "sentry_options.h"
#include "sentry_testsupport.h"
#include <stdbool.h>

static volatile bool g_before_crash_called = false;
static volatile int g_before_crash_call_count = 0;
static volatile void *g_before_crash_user_data = NULL;

static void
test_before_crash_func(void *user_data)
{
g_before_crash_called = true;
g_before_crash_call_count++;
g_before_crash_user_data = user_data;
}

static void
reset_before_crash_state(void)
{
g_before_crash_called = false;
g_before_crash_call_count = 0;
g_before_crash_user_data = NULL;
}

SENTRY_TEST(before_crash_func_call)
{
void *user_data = (void *)1;
reset_before_crash_state();

SENTRY_TEST_OPTIONS_NEW(options);
sentry_options_set_dsn(options, "https://[email protected]/42");
sentry_options_set_before_crash(options, test_before_crash_func, user_data);
sentry_options_set_auto_session_tracking(options, false);

TEST_CHECK_INT_EQUAL(sentry_init(options), 0);

// Verify before_crash_func is set correctly in options
TEST_CHECK(options->before_crash_func == test_before_crash_func);
TEST_CHECK(options->before_crash_data == user_data);

// Call the before_crash_func directly to test it
if (options->before_crash_func) {
options->before_crash_func(options->before_crash_data);
}

sentry_close();

// Verify the before_crash_func was called correctly
TEST_CHECK(g_before_crash_called);
TEST_CHECK_INT_EQUAL(g_before_crash_call_count, 1);
TEST_CHECK(g_before_crash_user_data == user_data);
}

SENTRY_TEST(before_crash_func_not_set)
{
reset_before_crash_state();

SENTRY_TEST_OPTIONS_NEW(options);
sentry_options_set_dsn(options, "https://[email protected]/42");
// Do not set before_crash_func
sentry_options_set_auto_session_tracking(options, false);

TEST_CHECK_INT_EQUAL(sentry_init(options), 0);

// Verify before_crash_func is not set
TEST_CHECK(options->before_crash_func == NULL);

sentry_close();

// Verify the before_crash_func was not called
TEST_CHECK(!g_before_crash_called);
TEST_CHECK_INT_EQUAL(g_before_crash_call_count, 0);
}

SENTRY_TEST(before_crash_func_change)
{
reset_before_crash_state();

SENTRY_TEST_OPTIONS_NEW(options);
sentry_options_set_dsn(options, "https://[email protected]/42");

// Set initial before_crash_func
void *user_data1 = (void *)2;
sentry_options_set_before_crash(
options, test_before_crash_func, user_data1);

// Change to different user data
void *user_data2 = (void *)3;
sentry_options_set_before_crash(
options, test_before_crash_func, user_data2);

sentry_options_set_auto_session_tracking(options, false);
TEST_CHECK_INT_EQUAL(sentry_init(options), 0);

// Verify the updated user_data is set
TEST_CHECK(options->before_crash_func == test_before_crash_func);
TEST_CHECK(options->before_crash_data == user_data2);

// Call the before_crash_func to test it
if (options->before_crash_func) {
options->before_crash_func(options->before_crash_data);
}

sentry_close();

// Verify the updated user_data was used
TEST_CHECK(g_before_crash_called);
TEST_CHECK(g_before_crash_user_data == user_data2);
}
11 changes: 7 additions & 4 deletions tests/unit/tests.inc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ XX(basic_spans)
XX(basic_tracing_context)
XX(basic_transaction)
XX(basic_write_envelope_to_file)
XX(before_crash_func_call)
XX(before_crash_func_change)
XX(before_crash_func_not_set)
XX(bgworker_flush)
XX(breadcrumb_without_type_or_message_still_valid)
XX(build_id_parser)
Expand Down Expand Up @@ -67,6 +70,10 @@ XX(dsn_with_ending_forward_slash_will_be_cleaned)
XX(dsn_with_non_http_scheme_is_invalid)
XX(dsn_without_project_id_is_invalid)
XX(dsn_without_url_scheme_is_invalid)
XX(embedded_info_basic)
XX(embedded_info_disabled)
XX(embedded_info_format)
XX(embedded_info_sentry_version)
XX(empty_transport)
XX(event_with_id)
XX(exception_without_type_or_value_still_valid)
Expand Down Expand Up @@ -204,7 +211,3 @@ XX(value_unicode)
XX(value_user)
XX(value_wrong_type)
XX(write_raw_envelope_to_file)
XX(embedded_info_basic)
XX(embedded_info_disabled)
XX(embedded_info_format)
XX(embedded_info_sentry_version)
Loading