Skip to content
Closed
Show file tree
Hide file tree
Changes from 7 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 *)0xDEADC0DE;
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 *)0x11111111;
sentry_options_set_before_crash(
options, test_before_crash_func, user_data1);

// Change to different user data
void *user_data2 = (void *)0x22222222;
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