diff --git a/CHANGELOG.md b/CHANGELOG.md index 0aee5f006..1e6505c46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ **Fixes**: - Linux: handle `ENOSYS` in `read_safely` to fix empty module list in seccomp-restricted environments. ([#1655](https://github.com/getsentry/sentry-native/pull/1655)) +- macOS: avoid stdio deadlock in breakpad exception handler. ([#1656](https://github.com/getsentry/sentry-native/pull/1656)) ## 0.13.7 diff --git a/include/sentry.h b/include/sentry.h index 14aa8ecd3..4b132d264 100644 --- a/include/sentry.h +++ b/include/sentry.h @@ -1439,9 +1439,15 @@ SENTRY_API void sentry_options_set_logger( /** * Enables or disables console logging after a crash. + * * When disabled, Sentry will not invoke logger callbacks after a crash * has been detected. This can be useful to avoid potential issues during - * crash handling that logging might cause. This is enabled by default. + * crash handling that logging might cause. + * + * Enabled by default, except for the breakpad backend on macOS, where + * it defaults to off because the in-process Mach exception handler can + * deadlock on stdio locks held by threads that were suspended at crash + * time. */ SENTRY_API void sentry_options_set_logger_enabled_when_crashed( sentry_options_t *opts, int enabled); diff --git a/src/backends/sentry_backend_breakpad.cpp b/src/backends/sentry_backend_breakpad.cpp index a3f80b99b..db329eb0d 100644 --- a/src/backends/sentry_backend_breakpad.cpp +++ b/src/backends/sentry_backend_breakpad.cpp @@ -84,7 +84,7 @@ breakpad_backend_callback(const google_breakpad::MinidumpDescriptor &descriptor, } } - SENTRY_INFO("entering breakpad minidump callback"); + SENTRY_SIGNAL_SAFE_LOG("INFO entering breakpad minidump callback"); // this is a bit strange, according to docs, `succeeded` should be true when // a minidump file was successfully generated. however, when running our @@ -147,7 +147,7 @@ breakpad_backend_callback(const google_breakpad::MinidumpDescriptor &descriptor, uctx = &uctx_data; #endif - SENTRY_DEBUG("invoking `on_crash` hook"); + SENTRY_SIGNAL_SAFE_LOG("DEBUG invoking `on_crash` hook"); sentry_value_t result = options->on_crash_func(uctx, event, options->on_crash_data); should_handle = !sentry_value_is_null(result); @@ -165,7 +165,8 @@ breakpad_backend_callback(const google_breakpad::MinidumpDescriptor &descriptor, bool capture_screenshot = options->attach_screenshot; #ifdef SENTRY_PLATFORM_WINDOWS if (capture_screenshot && options->before_screenshot_func) { - SENTRY_DEBUG("invoking `before_screenshot` hook"); + SENTRY_SIGNAL_SAFE_LOG( + "DEBUG invoking `before_screenshot` hook"); capture_screenshot = options->before_screenshot_func( event, options->before_screenshot_data) != 0; @@ -214,7 +215,8 @@ breakpad_backend_callback(const google_breakpad::MinidumpDescriptor &descriptor, sentry__path_remove(dump_path); sentry__path_free(dump_path); } else { - SENTRY_DEBUG("event was discarded by the `on_crash` hook"); + SENTRY_SIGNAL_SAFE_LOG( + "DEBUG event was discarded by the `on_crash` hook"); sentry_value_decref(event); } @@ -223,7 +225,7 @@ breakpad_backend_callback(const google_breakpad::MinidumpDescriptor &descriptor, sentry__transport_dump_queue(options->transport, options->run); // and restore the old transport } - SENTRY_INFO("crash has been captured"); + SENTRY_SIGNAL_SAFE_LOG("INFO crash has been captured"); #ifndef SENTRY_PLATFORM_WINDOWS sentry__leave_signal_handler(); diff --git a/src/backends/sentry_backend_inproc.c b/src/backends/sentry_backend_inproc.c index a52371d82..e2b1d97a3 100644 --- a/src/backends/sentry_backend_inproc.c +++ b/src/backends/sentry_backend_inproc.c @@ -32,31 +32,6 @@ #endif #include -/** - * Signal/async-safe logging macro for use in signal handlers or other - * contexts where stdio and malloc are unsafe. Only supports static strings. - */ -#ifdef SENTRY_PLATFORM_UNIX -# include -# define SENTRY_SIGNAL_SAFE_LOG(msg) \ - do { \ - static const char _msg[] = "[sentry] " msg "\n"; \ - (void)!write(STDERR_FILENO, _msg, sizeof(_msg) - 1); \ - } while (0) -#elif defined(SENTRY_PLATFORM_WINDOWS) -# define SENTRY_SIGNAL_SAFE_LOG(msg) \ - do { \ - static const char _msg[] = "[sentry] " msg "\n"; \ - OutputDebugStringA(_msg); \ - HANDLE _stderr = GetStdHandle(STD_ERROR_HANDLE); \ - if (_stderr && _stderr != INVALID_HANDLE_VALUE) { \ - DWORD _written; \ - WriteFile(_stderr, _msg, (DWORD)(sizeof(_msg) - 1), &_written, \ - NULL); \ - } \ - } while (0) -#endif - /** * Inproc Backend Introduction * diff --git a/src/sentry_batcher.c b/src/sentry_batcher.c index 8bf44ebd8..6130de4ea 100644 --- a/src/sentry_batcher.c +++ b/src/sentry_batcher.c @@ -155,9 +155,9 @@ sentry__batcher_flush(sentry_batcher_t *batcher, bool crash_safe) while (!sentry__atomic_compare_swap(&batcher->flushing, 0, 1)) { const int max_attempts = 200; if (++attempts > max_attempts) { - SENTRY_WARN( - "sentry__batcher_flush: timeout waiting for flushing " - "lock in crash-safe mode"); + SENTRY_SIGNAL_SAFE_LOG( + "WARN sentry__batcher_flush: timeout waiting for " + "flushing lock in crash-safe mode"); return false; } diff --git a/src/sentry_logger.h b/src/sentry_logger.h index b6c8febed..51ee723fd 100644 --- a/src/sentry_logger.h +++ b/src/sentry_logger.h @@ -51,4 +51,29 @@ void sentry__logger_disable(void); #define SENTRY_FATAL(message) sentry__logger_log(SENTRY_LEVEL_FATAL, message) +/** + * Signal/async-safe logging macro for use in signal handlers or other + * contexts where stdio and malloc are unsafe. Only supports static strings. + */ +#ifdef SENTRY_PLATFORM_UNIX +# include +# define SENTRY_SIGNAL_SAFE_LOG(msg) \ + do { \ + static const char _msg[] = "[sentry] " msg "\n"; \ + (void)!write(STDERR_FILENO, _msg, sizeof(_msg) - 1); \ + } while (0) +#elif defined(SENTRY_PLATFORM_WINDOWS) +# define SENTRY_SIGNAL_SAFE_LOG(msg) \ + do { \ + static const char _msg[] = "[sentry] " msg "\n"; \ + OutputDebugStringA(_msg); \ + HANDLE _stderr = GetStdHandle(STD_ERROR_HANDLE); \ + if (_stderr && _stderr != INVALID_HANDLE_VALUE) { \ + DWORD _written; \ + WriteFile(_stderr, _msg, (DWORD)(sizeof(_msg) - 1), &_written, \ + NULL); \ + } \ + } while (0) +#endif + #endif diff --git a/src/sentry_logs.c b/src/sentry_logs.c index e75fa9c38..b176aeef5 100644 --- a/src/sentry_logs.c +++ b/src/sentry_logs.c @@ -520,12 +520,12 @@ sentry__logs_shutdown(uint64_t timeout) void sentry__logs_flush_crash_safe(void) { - SENTRY_DEBUG("crash-safe logs flush"); + SENTRY_SIGNAL_SAFE_LOG("DEBUG crash-safe logs flush"); sentry_batcher_t *batcher = sentry__batcher_peek(&g_batcher); if (batcher) { sentry__batcher_flush_crash_safe(batcher); } - SENTRY_DEBUG("crash-safe logs flush complete"); + SENTRY_SIGNAL_SAFE_LOG("DEBUG crash-safe logs flush complete"); } uintptr_t diff --git a/src/sentry_metrics.c b/src/sentry_metrics.c index 6c45923f9..e7e83ce3f 100644 --- a/src/sentry_metrics.c +++ b/src/sentry_metrics.c @@ -165,12 +165,12 @@ sentry__metrics_shutdown(uint64_t timeout) void sentry__metrics_flush_crash_safe(void) { - SENTRY_DEBUG("crash-safe metrics flush"); + SENTRY_SIGNAL_SAFE_LOG("DEBUG crash-safe metrics flush"); sentry_batcher_t *batcher = sentry__batcher_peek(&g_batcher); if (batcher) { sentry__batcher_flush_crash_safe(batcher); } - SENTRY_DEBUG("crash-safe metrics flush complete"); + SENTRY_SIGNAL_SAFE_LOG("DEBUG crash-safe metrics flush complete"); } uintptr_t diff --git a/src/sentry_options.c b/src/sentry_options.c index 45aa9bbc6..d8a173278 100644 --- a/src/sentry_options.c +++ b/src/sentry_options.c @@ -49,7 +49,19 @@ sentry_options_new(void) opts->system_crash_reporter_enabled = false; opts->attach_screenshot = false; opts->crashpad_wait_for_upload = false; + // On macOS, breakpad suspends all other threads of the process before + // writing the minidump and invokes our backend callback from inside + // WriteMinidumpWithException() while those threads are still suspended. + // Any vfprintf-based log call from that context can deadlock waiting for + // a stdio lock held by a suspended thread (e.g. libcurl's verbose output + // on stderr). Default off here; users who accept the risk can still opt + // in via `sentry_options_set_logger_enabled_when_crashed(opts, 1)`. +#if defined(SENTRY_BACKEND_BREAKPAD) && defined(SENTRY_PLATFORM_DARWIN) \ + && !defined(SENTRY_PLATFORM_IOS) + opts->enable_logging_when_crashed = false; +#else opts->enable_logging_when_crashed = true; +#endif opts->propagate_traceparent = false; opts->crashpad_limit_stack_capture_to_sp = false; opts->enable_metrics = true;