diff --git a/client/crashpad_client_linux.cc b/client/crashpad_client_linux.cc index 238413709..da2ee866c 100644 --- a/client/crashpad_client_linux.cc +++ b/client/crashpad_client_linux.cc @@ -207,16 +207,30 @@ class SignalHandler { static void HandleOrReraiseSignal(int signo, siginfo_t* siginfo, void* context) { - if (handler_->first_chance_handler_ && - handler_->first_chance_handler_( - signo, siginfo, static_cast(context))) { - return; - } - // Only handle the first fatal signal observed. If another thread receives a // crash signal, it waits for the first dump to complete instead of // requesting another. if (!handler_->disabled_.test_and_set()) { + // but we also need to ensure that a handling thread that gets preempted + // with a non-masked signal, can proceed to handle that signal. + SignalHandler::handling_signal_on_thread_ = true; + } + if (SignalHandler::handling_signal_on_thread_) { + // TODO(supervacuus): + // On Linux the first-chance handler is executed in the context of a + // signal-handler, which can be asynchronous (crashpad handles `SIGQUIT`) + // and even interrupt itself if the first-chance handler crashes with a + // non-masked synchronous signal. This means we not only need to be safe + // with multiple threads raising signals with thread-specific signal- + // masks, but also with reentrancy from the same thread. Adapting our + // first-chance handler to this would mean to adapt the pipeline, because + // even if the handler was safe wrt the above scenarios, there is + // currently no way in the event model to correlate crashes this way. + if (handler_->first_chance_handler_ && + handler_->first_chance_handler_( + signo, siginfo, static_cast(context))) { + return; + } handler_->HandleCrash(signo, siginfo, context); handler_->WakeThreads(); if (handler_->last_chance_handler_ && @@ -274,10 +288,12 @@ class SignalHandler { #else std::atomic_flag disabled_; #endif + thread_local static bool handling_signal_on_thread_; static SignalHandler* handler_; }; SignalHandler* SignalHandler::handler_ = nullptr; +thread_local bool SignalHandler::handling_signal_on_thread_ = false; // Launches a single use handler to snapshot this process. class LaunchAtCrashHandler : public SignalHandler { @@ -470,8 +486,14 @@ bool CrashpadClient::StartHandler( return false; } - std::vector argv = BuildHandlerArgvStrings( - handler, database, metrics_dir, url, http_proxy, annotations, arguments, attachments); + std::vector argv = BuildHandlerArgvStrings(handler, + database, + metrics_dir, + url, + http_proxy, + annotations, + arguments, + attachments); argv.push_back(FormatArgumentInt("initial-client-fd", handler_sock.get())); argv.push_back("--shared-client-connection"); @@ -694,8 +716,14 @@ bool CrashpadClient::StartHandlerAtCrash( const std::map& annotations, const std::vector& arguments, const std::vector& attachments) { - std::vector argv = BuildHandlerArgvStrings( - handler, database, metrics_dir, url, http_proxy, annotations, arguments, attachments); + std::vector argv = BuildHandlerArgvStrings(handler, + database, + metrics_dir, + url, + http_proxy, + annotations, + arguments, + attachments); auto signal_handler = LaunchAtCrashHandler::Get(); return signal_handler->Initialize(&argv, nullptr, &unhandled_signals_); diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index cd1b206b0..63e2e66cd 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -155,10 +155,6 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { return EXCEPTION_CONTINUE_SEARCH; } - if (first_chance_handler_ && first_chance_handler_(exception_pointers)) { - return EXCEPTION_CONTINUE_SEARCH; - } - // Otherwise, we know the handler startup has succeeded, and we can continue. // Tracks whether a thread has already entered UnhandledExceptionHandler. @@ -180,6 +176,17 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { SleepEx(INFINITE, false); } + // TODO(supervacuus): + // On Windows the first-chance handler is executed inside the UEF which is + // synchronous with respect to thread it is executing on. However, any other + // running thread can also raise an exception and therefore will also execute + // a UEF, at which point they would run concurrently. Adapting to this would + // mean to adapt the pipeline, because even if the handler was UEF-safe, + // there is currently no way in which to correlate non-nested crashes. + if (first_chance_handler_ && first_chance_handler_(exception_pointers)) { + return EXCEPTION_CONTINUE_SEARCH; + } + // Otherwise, we're the first thread, so record the exception pointer and // signal the crash handler. g_crash_exception_information.thread_id = GetCurrentThreadId();