Mac: ensure crashes are forwarded to system crash reporter

Historically, we have forwarded exceptions from Crashpad to Apple's
system reporting tool explicitly. However, starting in 10.15, the crash
reporter stopped paying attention to EXC_CRASH type exceptions.
Instead, it now listens to EXC_CORPSE_NOTIFY, which is raised postmortem
if and only if an EXC_CRASH handler returns MACH_RCV_PORT_DIED. But we
specifically were *not* doing that in order to avoid multiple reports
(one from EXC_CRASH and one from EXC_CORPSE_NOTIFY) between 10.11 and
10.14.

This change stops forwarding EXC_CRASH exceptions explicitly and *does*
return MACH_RCV_PORT_DIED if they should be forwarded

Bug: chromium:388545119
Change-Id: I36a781390386b9e164072fcc602809723a2dc2ee
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/6242575
Reviewed-by: Mark Mentovai <[email protected]>

Author: speednoisemovement

client/crashpad_client_mac.cc

@@ -64,13 +64,13 @@ std::string FormatArgumentInt(const std::string& name, int value) {
 // reasons, an EXC_CRASH exception will be sent. See 10.9.5
 // xnu-2422.115.4/bsd/kern/kern_exit.c proc_prepareexit().
 //
-// EXC_RESOURCE and EXC_GUARD do not become signals or EXC_CRASH exceptions. The
-// host-level exception handler in the kernel does not receive these exception
-// types, and even if it did, it would not map them to signals. Instead, the
-// first Mach service loaded by the root (process ID 1) launchd with a boolean
-// “ExceptionServer” property in its job dictionary (regardless of its value) or
-// with any subdictionary property will become the host-level exception handler
-// for EXC_CRASH, EXC_RESOURCE, and EXC_GUARD. See 10.9.5
+// EXC_RESOURCE and EXC_GUARD (pre-macOS 13) do not become signals or EXC_CRASH
+// exceptions. The host-level exception handler in the kernel does not receive
+// these exception types, and even if it did, it would not map them to signals.
+// Instead, the first Mach service loaded by the root (process ID 1) launchd
+// with a boolean “ExceptionServer” property in its job dictionary (regardless
+// of its value) or with any subdictionary property will become the host-level
+// exception handler for EXC_CRASH, EXC_RESOURCE, and EXC_GUARD. See 10.9.5
 // launchd-842.92.1/src/core.c job_setup_exception_port(). Normally, this job is
 // com.apple.ReportCrash.Root, the systemwide Apple Crash Reporter. Since it is
 // impossible to receive EXC_RESOURCE and EXC_GUARD exceptions through the
@@ -83,8 +83,15 @@ std::string FormatArgumentInt(const std::string& name, int value) {
 // so AND them with ExcMaskValid(). EXC_MASK_CRASH is always supported.
 bool SetCrashExceptionPorts(exception_handler_t exception_handler) {
   ExceptionPorts exception_ports(ExceptionPorts::kTargetTypeTask, TASK_NULL);
+
+  exception_mask_t mask = EXC_MASK_CRASH | EXC_MASK_RESOURCE;
+  if (MacOSVersionNumber() < 13'00'00) {
+    // EXC_GUARD is delivered as an EXC_CRASH macOS 13 and later.
+    mask |= EXC_MASK_GUARD;
+  }
+
   return exception_ports.SetExceptionPort(
-      (EXC_MASK_CRASH | EXC_MASK_RESOURCE | EXC_MASK_GUARD) & ExcMaskValid(),
+      mask & ExcMaskValid(),
       exception_handler,
       EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES,
       MACHINE_THREAD_STATE);

handler/mac/crash_report_exception_handler.cc

@@ -31,6 +31,7 @@
 #include "util/mach/bootstrap.h"
 #include "util/mach/exc_client_variants.h"
 #include "util/mach/exception_behaviors.h"
+#include "util/mach/exception_ports.h"
 #include "util/mach/exception_types.h"
 #include "util/mach/mach_extensions.h"
 #include "util/mach/mach_message.h"
@@ -188,59 +189,79 @@ kern_return_t CrashReportExceptionHandler::CatchMachException(
     }
   }
 
-  if (client_options.system_crash_reporter_forwarding != TriState::kDisabled &&
-      (exception == EXC_CRASH ||
-       exception == EXC_RESOURCE ||
-       exception == EXC_GUARD)) {
-    // Don’t forward simulated exceptions such as kMachExceptionSimulated to the
-    // system crash reporter. Only forward the types of exceptions that it would
-    // receive under normal conditions. Although the system crash reporter is
-    // able to deal with other exceptions including simulated ones, forwarding
-    // them to the system crash reporter could present the system’s crash UI for
-    // processes that haven’t actually crashed, and could result in reports not
-    // actually associated with crashes being sent to the operating system
-    // vendor.
-    base::apple::ScopedMachSendRight system_crash_reporter_handler(
-        SystemCrashReporterHandler());
-    if (system_crash_reporter_handler.get()) {
-      // Make copies of mutable out parameters so that the system crash reporter
-      // can’t influence the state returned by this method.
-      thread_state_flavor_t flavor_forward = *flavor;
-      mach_msg_type_number_t new_state_forward_count = *new_state_count;
-      std::vector<natural_t> new_state_forward(
-          new_state, new_state + new_state_forward_count);
-
-      // The system crash reporter requires the behavior to be
-      // EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES. It uses the identity
-      // parameters but doesn’t appear to use the state parameters, including
-      // |flavor|, and doesn’t care if they are 0 or invalid. As long as an
-      // identity is available (checked above), any other exception behavior is
-      // converted to what the system crash reporter wants, with the caveat that
-      // problems may arise if the state wasn’t available and the system crash
-      // reporter changes in the future to use it. However, normally, the state
-      // will be available.
-      kern_return_t kr = UniversalExceptionRaise(
-          EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES,
-          system_crash_reporter_handler.get(),
-          thread,
-          task,
-          exception,
-          code,
-          code_count,
-          &flavor_forward,
-          old_state,
-          old_state_count,
-          new_state_forward_count ? &new_state_forward[0] : nullptr,
-          &new_state_forward_count);
-      MACH_LOG_IF(WARNING, kr != KERN_SUCCESS, kr) << "UniversalExceptionRaise";
+  if (client_options.system_crash_reporter_forwarding != TriState::kDisabled) {
+    if (exception == EXC_CRASH) {
+      // For exception handlers that respond to state-carrying behaviors, when
+      // the handler is called by the kernel (as it is normally), the kernel
+      // will attempt to set a new thread state when the exception handler
+      // returns successfully. Other code that mimics the kernel’s
+      // exception-delivery semantics may implement the same or similar
+      // behavior. In some situations, it is undesirable to set a new thread
+      // state. If the exception handler were to return unsuccessfully, however,
+      // the kernel would continue searching for an exception handler at a wider
+      // (task or host) scope. This may also be undesirable.
+      //
+      // If such exception handlers return `MACH_RCV_PORT_DIED`, the kernel will
+      // not set a new thread state and will also not search for another
+      // exception handler. See 15.3 xnu-11215.84.4/osfmk/kern/exception.c.
+      // `exception_deliver()` will only set a new thread state if the handler’s
+      // return code was `MACH_MSG_SUCCESS` (a synonym for `KERN_SUCCESS`), and
+      // subsequently, `exception_triage()` will not search for a new handler if
+      // the handler’s return code was `KERN_SUCCESS` or `MACH_RCV_PORT_DIED`.
+      //
+      // Another effect of returning `MACH_RCV_PORT_DIED` for `EXC_CRASH` is
+      // that an `EXC_CORPSE_NOTIFY` exception is generated. Starting with macOS
+      // 10.15, for the system crash reporter to generate a report,
+      // `EXC_CORPSE_NOTIFY` *must* be generated and forwarding `EXC_CRASH` (as
+      // we do below with `EXC_RESOURCE` and pre-macOS 13 `EXC_GUARD`) is not
+      // sufficient. Between macOS 10.11 and macOS 10.14 (inclusive), both
+      // forwarding as below, and causing `EXC_CORPSE_NOTIFY` to be generated
+      // are sufficient (and in fact, if we do both, two crash reports are
+      // generated).
+      return MACH_RCV_PORT_DIED;
+    }
+    if (exception == EXC_RESOURCE || exception == EXC_GUARD) {
+      // Only forward the types of exceptions that the crash reporter would
+      // receive under normal conditions. Otherwise, system crash reporter could
+      // present the system’s crash UI for processes that haven’t actually
+      // crashed, and could result in reports not actually associated with
+      // crashes being sent to the operating system vendor.
+      base::apple::ScopedMachSendRight system_crash_reporter_handler(
+          SystemCrashReporterHandler());
+      if (system_crash_reporter_handler.get()) {
+        // Make copies of mutable out parameters so that the system crash
+        // reporter can’t influence the state returned by this method.
+        thread_state_flavor_t flavor_forward = *flavor;
+        mach_msg_type_number_t new_state_forward_count = *new_state_count;
+        std::vector<natural_t> new_state_forward;
+        if (new_state_forward_count) {
+          new_state_forward.assign(new_state,
+                                   new_state + new_state_forward_count);
+        }
+        kern_return_t kr = UniversalExceptionRaise(
+            EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES,
+            system_crash_reporter_handler.get(),
+            thread,
+            task,
+            exception,
+            code,
+            code_count,
+            &flavor_forward,
+            old_state,
+            old_state_count,
+            new_state_forward_count ? &new_state_forward[0] : nullptr,
+            &new_state_forward_count);
+        MACH_LOG_IF(WARNING, kr != KERN_SUCCESS, kr)
+            << "UniversalExceptionRaise";
+      }
     }
   }
 
   ExcServerCopyState(
       behavior, old_state, old_state_count, new_state, new_state_count);
 
   Metrics::ExceptionCaptureResult(Metrics::CaptureResult::kSuccess);
-  return ExcServerSuccessfulReturnValue(exception, behavior, false);
+  return KERN_SUCCESS;
 }
 
 }  // namespace crashpad

tools/run_with_crashpad.md

@@ -33,9 +33,9 @@ along with any arguments specified (_ARG…_) with the new exception port in
 effect.
 
 On macOS, the exception port is configured to receive exceptions of type
-`EXC_CRASH`, `EXC_RESOURCE`, and `EXC_GUARD`. The exception behavior is
-configured as `EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES`. The thread
-state flavor is set to `MACHINE_THREAD_STATE`.
+`EXC_CRASH`, `EXC_RESOURCE`, and `EXC_GUARD` (until macOS 13). The exception
+behavior is configured as `EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES`. The
+thread state flavor is set to `MACHINE_THREAD_STATE`.
 
 Programs that use the Crashpad client library directly will not normally use
 this tool. This tool exists to allow programs that are unaware of Crashpad to be

util/mach/exception_types.cc

@@ -135,26 +135,27 @@ exception_type_t ExcCrashRecoverOriginalException(
 bool ExcCrashCouldContainException(exception_type_t exception) {
   // EXC_CRASH should never be wrapped in another EXC_CRASH.
   //
-  // EXC_RESOURCE and EXC_GUARD are software exceptions that are never wrapped
-  // in EXC_CRASH. The only time EXC_CRASH is generated is for processes exiting
-  // due to an unhandled core-generating signal or being killed by SIGKILL for
-  // code-signing reasons. Neither of these apply to EXC_RESOURCE or EXC_GUARD.
-  // See 10.10 xnu-2782.1.97/bsd/kern/kern_exit.c proc_prepareexit(). Receiving
-  // these exception types wrapped in EXC_CRASH would lose information because
-  // their code[0] uses all 64 bits (see ExceptionSnapshotMac::Initialize()) and
-  // the code[0] recovered from EXC_CRASH only contains 20 significant bits.
+  // EXC_RESOURCE is a software exceptions that is never wrapped in EXC_CRASH.
+  // The same applies to EXC_GUARD below macOS 13. The only time EXC_CRASH is
+  // generated is for processes exiting due to an unhandled core-generating
+  // signal or being killed by SIGKILL for code-signing reasons. Neither of
+  // these apply to EXC_RESOURCE or EXC_GUARD. See 10.10
+  // xnu-2782.1.97/bsd/kern/kern_exit.c proc_prepareexit(). Receiving these
+  // exception types wrapped in EXC_CRASH would lose information because their
+  // code[0] uses all 64 bits (see ExceptionSnapshotMac::Initialize()) and the
+  // code[0] recovered from EXC_CRASH only contains 20 significant bits.
   //
   // EXC_CORPSE_NOTIFY may be generated from EXC_CRASH, but the opposite should
   // never occur.
   //
   // kMachExceptionSimulated is a non-fatal Crashpad-specific pseudo-exception
   // that never exists as an exception within the kernel and should thus never
   // be wrapped in EXC_CRASH.
-  return exception != EXC_CRASH &&
-         exception != EXC_RESOURCE &&
-         exception != EXC_GUARD &&
-         exception != EXC_CORPSE_NOTIFY &&
-         exception != kMachExceptionSimulated;
+  if (MacOSVersionNumber() < 13'00'00 && exception == EXC_GUARD) {
+    return false;
+  }
+  return exception != EXC_CRASH && exception != EXC_RESOURCE &&
+         exception != EXC_CORPSE_NOTIFY && exception != kMachExceptionSimulated;
 }
 
 int32_t ExceptionCodeForMetrics(exception_type_t exception,

util/mach/exception_types_test.cc

@@ -124,7 +124,6 @@ TEST(ExceptionTypes, ExcCrashCouldContainException) {
   EXPECT_TRUE(ExcCrashCouldContainException(EXC_RPC_ALERT));
   EXPECT_FALSE(ExcCrashCouldContainException(EXC_CRASH));
   EXPECT_FALSE(ExcCrashCouldContainException(EXC_RESOURCE));
-  EXPECT_FALSE(ExcCrashCouldContainException(EXC_GUARD));
   EXPECT_FALSE(ExcCrashCouldContainException(EXC_CORPSE_NOTIFY));
   EXPECT_FALSE(ExcCrashCouldContainException(kMachExceptionSimulated));
 }