fix: adapt blocking of handlers for Linux and Windows (#109)

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<ucontext_t*>(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<ucontext_t*>(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<std::string> argv = BuildHandlerArgvStrings(
-      handler, database, metrics_dir, url, http_proxy, annotations, arguments, attachments);
+  std::vector<std::string> 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<std::string, std::string>& annotations,
     const std::vector<std::string>& arguments,
     const std::vector<base::FilePath>& attachments) {
-  std::vector<std::string> argv = BuildHandlerArgvStrings(
-      handler, database, metrics_dir, url, http_proxy, annotations, arguments, attachments);
+  std::vector<std::string> 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_);

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();