diff --git a/CHANGELOG.md b/CHANGELOG.md index b85251c3a..17db286e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - Auto-populate `event.user.id` with a persistent per-installation UUID when no explicit user ID is set. ([#1661](https://github.com/getsentry/sentry-native/pull/1661)) +**Fixes**: + +- Native/Breakpad/Windows: fixed capturing abort(). ([#1708](https://github.com/getsentry/sentry-native/pull/1708)) + ## 0.14.0 **Breaking / Important behavior changes**: diff --git a/src/backends/native/sentry_crash_handler.c b/src/backends/native/sentry_crash_handler.c index 5418d2a55..6b6671d45 100644 --- a/src/backends/native/sentry_crash_handler.c +++ b/src/backends/native/sentry_crash_handler.c @@ -9,6 +9,7 @@ #include "sentry_alloc.h" #include "sentry_core.h" #include "sentry_logger.h" +#include "sentry_os.h" #include "sentry_sync.h" #include @@ -878,6 +879,14 @@ sentry__crash_handler_shutdown(void) static sentry_crash_ipc_t *g_crash_ipc = NULL; static LPTOP_LEVEL_EXCEPTION_FILTER g_previous_filter = NULL; +static LONG WINAPI crash_exception_filter(EXCEPTION_POINTERS *exception_info); + +static void +crash_sigabrt_handler(EXCEPTION_POINTERS *exception_pointers) +{ + crash_exception_filter(exception_pointers); +} + /** * Windows exception filter (crash handler) */ @@ -973,6 +982,7 @@ sentry__crash_handler_init(sentry_crash_ipc_t *ipc) // Install exception filter g_previous_filter = SetUnhandledExceptionFilter(crash_exception_filter); + sentry__win32_install_sigabrt_handler(crash_sigabrt_handler); SENTRY_DEBUG("crash handler initialized (Windows SEH)"); return 0; @@ -987,6 +997,8 @@ sentry__crash_handler_shutdown(void) g_previous_filter = NULL; } + sentry__win32_restore_sigabrt_handler(); + g_crash_ipc = NULL; SENTRY_DEBUG("crash handler shutdown"); diff --git a/src/backends/sentry_backend_breakpad.cpp b/src/backends/sentry_backend_breakpad.cpp index 3f0f4cdea..7eed398e6 100644 --- a/src/backends/sentry_backend_breakpad.cpp +++ b/src/backends/sentry_backend_breakpad.cpp @@ -56,6 +56,14 @@ extern "C" { #endif #ifdef SENTRY_PLATFORM_WINDOWS +static void +handle_sigabrt(EXCEPTION_POINTERS *exception_pointers) +{ + sentry_ucontext_t uctx = {}; + uctx.exception_ptrs = *exception_pointers; + sentry_handle_exception(&uctx); +} + static bool breakpad_backend_callback(const wchar_t *breakpad_dump_path, const wchar_t *minidump_id, void *UNUSED(context), @@ -299,6 +307,9 @@ breakpad_backend_startup( backend->data = new (std::nothrow) google_breakpad::ExceptionHandler( current_run_folder->path_w, nullptr, breakpad_backend_callback, nullptr, google_breakpad::ExceptionHandler::HANDLER_EXCEPTION); + if (backend->data) { + sentry__win32_install_sigabrt_handler(handle_sigabrt); + } #elif defined(SENTRY_PLATFORM_MACOS) // If process is being debugged and there are breakpoints set it will cause // task_set_exception_ports to crash the whole process and debugger @@ -320,6 +331,10 @@ breakpad_backend_startup( static void breakpad_backend_shutdown(sentry_backend_t *backend) { +#ifdef SENTRY_PLATFORM_WINDOWS + sentry__win32_restore_sigabrt_handler(); +#endif + const auto *eh = static_cast(backend->data); backend->data = nullptr; diff --git a/src/backends/sentry_backend_inproc.c b/src/backends/sentry_backend_inproc.c index 6b7d6b395..d6c73ff81 100644 --- a/src/backends/sentry_backend_inproc.c +++ b/src/backends/sentry_backend_inproc.c @@ -13,7 +13,6 @@ #include "sentry_options.h" #if defined(SENTRY_PLATFORM_WINDOWS) # include "sentry_os.h" -# include #endif #include "sentry_scope.h" #include "sentry_screenshot.h" @@ -585,47 +584,10 @@ static const struct signal_slot SIGNAL_DEFINITIONS[SIGNAL_COUNT] = { static LONG WINAPI handle_exception(EXCEPTION_POINTERS *); -// SIGABRT handling on Windows: abort() calls the signal handler but doesn't -// go through the unhandled exception filter. We register a SIGABRT handler -// that captures context and calls into our exception handler. -static void (*g_previous_sigabrt_handler)(int) = NULL; - static void -handle_sigabrt(int signum) +handle_sigabrt(EXCEPTION_POINTERS *exception_pointers) { - (void)signum; - - // Capture the current CPU context - CONTEXT context; - RtlCaptureContext(&context); - - // Create a synthetic exception record for abort - EXCEPTION_RECORD record; - memset(&record, 0, sizeof(record)); - record.ExceptionCode = STATUS_FATAL_APP_EXIT; - record.ExceptionFlags = EXCEPTION_NONCONTINUABLE; -# if defined(_M_AMD64) - record.ExceptionAddress = (PVOID)context.Rip; -# elif defined(_M_IX86) - record.ExceptionAddress = (PVOID)context.Eip; -# elif defined(_M_ARM64) - record.ExceptionAddress = (PVOID)context.Pc; -# endif - - EXCEPTION_POINTERS exception_pointers; - exception_pointers.ContextRecord = &context; - exception_pointers.ExceptionRecord = &record; - - handle_exception(&exception_pointers); - - // If we get here, call the previous handler or terminate - if (g_previous_sigabrt_handler && g_previous_sigabrt_handler != SIG_DFL - && g_previous_sigabrt_handler != SIG_IGN) { - g_previous_sigabrt_handler(signum); - } - - // Terminate the process - abort() must not return - TerminateProcess(GetCurrentProcess(), 3); + handle_exception(exception_pointers); } static int @@ -649,8 +611,7 @@ startup_inproc_backend( g_previous_handler = SetUnhandledExceptionFilter(&handle_exception); SetErrorMode(SEM_FAILCRITICALERRORS); - // Register SIGABRT handler - abort() doesn't go through the UEF - g_previous_sigabrt_handler = signal(SIGABRT, handle_sigabrt); + sentry__win32_install_sigabrt_handler(handle_sigabrt); return 0; } @@ -666,10 +627,7 @@ shutdown_inproc_backend(sentry_backend_t *backend) SetUnhandledExceptionFilter(current_handler); } - // Restore previous SIGABRT handler (unconditionally, since SIG_DFL is - // typically NULL on MSVC and a conditional check would skip restoration) - signal(SIGABRT, g_previous_sigabrt_handler); - g_previous_sigabrt_handler = NULL; + sentry__win32_restore_sigabrt_handler(); if (backend) { backend->data = NULL; diff --git a/src/sentry_os.c b/src/sentry_os.c index fefcf01eb..e1bb18cb9 100644 --- a/src/sentry_os.c +++ b/src/sentry_os.c @@ -12,6 +12,77 @@ #ifdef SENTRY_PLATFORM_WINDOWS +# include +# include + +static sentry__win32_abort_handler_t g_sigabrt_handler = NULL; +static void (*g_previous_sigabrt_handler)(int) = NULL; +static bool g_sigabrt_installed = false; + +static void +handle_sigabrt(int signum) +{ + (void)signum; + + // Capture the current CPU context + CONTEXT context; + RtlCaptureContext(&context); + + // Create a synthetic exception record for abort + EXCEPTION_RECORD record; + memset(&record, 0, sizeof(record)); + record.ExceptionCode = STATUS_FATAL_APP_EXIT; + record.ExceptionFlags = EXCEPTION_NONCONTINUABLE; +# if defined(_M_AMD64) + record.ExceptionAddress = (PVOID)context.Rip; +# elif defined(_M_IX86) + record.ExceptionAddress = (PVOID)context.Eip; +# elif defined(_M_ARM64) + record.ExceptionAddress = (PVOID)context.Pc; +# endif + + EXCEPTION_POINTERS exception_pointers; + exception_pointers.ContextRecord = &context; + exception_pointers.ExceptionRecord = &record; + + if (g_sigabrt_handler) { + g_sigabrt_handler(&exception_pointers); + } + + // If we get here, call the previous handler or terminate + if (g_previous_sigabrt_handler && g_previous_sigabrt_handler != SIG_DFL + && g_previous_sigabrt_handler != SIG_IGN) { + g_previous_sigabrt_handler(signum); + } + + // Terminate the process - abort() must not return + TerminateProcess(GetCurrentProcess(), 3); +} + +void +sentry__win32_install_sigabrt_handler(sentry__win32_abort_handler_t handler) +{ + g_sigabrt_handler = handler; + if (!g_sigabrt_installed) { + g_previous_sigabrt_handler = signal(SIGABRT, handle_sigabrt); + g_sigabrt_installed = true; + } +} + +void +sentry__win32_restore_sigabrt_handler(void) +{ + if (g_sigabrt_installed) { + // Restore previous SIGABRT handler (unconditionally, since SIG_DFL is + // typically NULL on MSVC and a conditional check would skip + // restoration) + signal(SIGABRT, g_previous_sigabrt_handler); + g_sigabrt_installed = false; + } + g_previous_sigabrt_handler = NULL; + g_sigabrt_handler = NULL; +} + # if !defined(SENTRY_PLATFORM_XBOX) # include # include diff --git a/src/sentry_os.h b/src/sentry_os.h index 7e7bf55dd..05ed866b6 100644 --- a/src/sentry_os.h +++ b/src/sentry_os.h @@ -18,6 +18,11 @@ void sentry__set_default_thread_stack_guarantee(void); void sentry__init_cached_kernel32_functions(void); void sentry__get_system_time(LPFILETIME filetime); +typedef void (*sentry__win32_abort_handler_t)(EXCEPTION_POINTERS *); +void sentry__win32_install_sigabrt_handler( + sentry__win32_abort_handler_t handler); +void sentry__win32_restore_sigabrt_handler(void); + #endif sentry_value_t sentry__get_os_context(void); diff --git a/tests/test_integration_native.py b/tests/test_integration_native.py index 0267d294c..fd4bb37a4 100644 --- a/tests/test_integration_native.py +++ b/tests/test_integration_native.py @@ -231,6 +231,22 @@ def test_native_sigabrt(cmake, httpserver): assert waiting.result +def test_native_abort(cmake, httpserver): + """Test abort() handling with native backend""" + tmp_path = cmake(["sentry_example"], {"SENTRY_BACKEND": "native"}) + + httpserver.expect_oneshot_request("/api/123456/envelope/").respond_with_data("OK") + + with httpserver.wait(timeout=10) as waiting: + run_crash( + tmp_path, + "sentry_example", + ["log", "stdout", "abort"], + env=dict(os.environ, SENTRY_DSN=make_dsn(httpserver)), + ) + assert waiting.result + + def test_native_multiple_crashes(cmake, httpserver): """Test handling multiple crashes in sequence""" tmp_path = cmake(["sentry_example"], {"SENTRY_BACKEND": "native"}) diff --git a/tests/test_integration_stdout.py b/tests/test_integration_stdout.py index 7fd6336aa..9d8f66cb3 100644 --- a/tests/test_integration_stdout.py +++ b/tests/test_integration_stdout.py @@ -186,23 +186,41 @@ def test_inproc_crash_stdout(cmake): assert_inproc_crash(envelope) +@pytest.mark.parametrize( + "backend", + [ + "inproc", + pytest.param( + "breakpad", + marks=pytest.mark.skipif( + not has_breakpad or is_qemu, reason="test needs breakpad backend" + ), + ), + ], +) @pytest.mark.skipif(is_qemu, reason="unreliable under qemu-user") -def test_inproc_abort_stdout(cmake): - """Test that a normal abort() call is captured by inproc backend. +def test_abort_stdout(cmake, backend): + """Test that a normal abort() call is captured by inproc and breakpad backends. This verifies that our SIGABRT handling changes (which bail out early for abort() on the handler thread or during recursion) don't break normal abort() capture from user code. """ - tmp_path, output = run_stdout_for("inproc", cmake, ["attachment", "abort"]) + tmp_path, output = run_stdout_for(backend, cmake, ["attachment", "abort"]) envelope = Envelope.deserialize(output) assert_crash_timestamp(has_files, tmp_path) - assert_meta(envelope, integration="inproc") + assert_meta(envelope, integration=backend) assert_breadcrumb(envelope) assert_attachment(envelope) - assert_inproc_crash(envelope) + if backend == "inproc": + assert_inproc_crash(envelope) + elif backend == "breakpad": + assert_minidump(envelope) + assert_breakpad_crash(envelope) + else: + pytest.fail(f"unsupported backend: {backend}") @pytest.mark.skipif(is_qemu, reason="unreliable under qemu-user")