ios: Fix crash in ObjcExceptionPreprocessor.

ObjcExceptionPreprocessor is a 'reasonable effort' attempt to catch an
NSException minidump at time the exception is thrown as opposed to when the application terminates due to the exception. If multiple
exceptions are thrown at the same time, Crashpad should correctly
report the final uncaught exception, but the minidump may not
represent the full `caught-at-thrown` minidump.

 - Don't assume ObjcExceptionPreprocessor throws an NSException.
 - Don't retain/release the exception. Instead of calling isEqual,
   just use a simple pointer comparison.
 - Make last_exception atomic.

Bug: crashpad: 445, 446
Change-Id: I9f2f2041e96aa9818c63937025e507487ae9d03d
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4317110
Reviewed-by: Ben Hamilton <benhamilton@google.com>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>

client/ios_handler/exception_processor.mm

@@ -38,6 +38,7 @@
 #include <sys/types.h>
 #include <unwind.h>
 
+#include <atomic>
 #include <exception>
 #include <type_traits>
 #include <typeinfo>
@@ -218,7 +219,7 @@ class ExceptionPreprocessorState {
   // preprocessor didn't catch anything, so pass the frames or just the context
   // to the exception_delegate.
   void FinalizeUncaughtNSException(id exception) {
-    if ([last_exception_ isEqual:exception] &&
+    if (last_exception() == exception &&
         !last_handled_intermediate_dump_.empty() &&
         exception_delegate_->MoveIntermediateDumpAtPathToPending(
             last_handled_intermediate_dump_)) {
@@ -227,8 +228,11 @@ class ExceptionPreprocessorState {
     }
 
     std::string name, reason;
-    SetNSExceptionAnnotations(exception, name, reason);
-    NSArray<NSNumber*>* address_array = [exception callStackReturnAddresses];
+    NSArray<NSNumber*>* address_array = nil;
+    if ([exception isKindOfClass:[NSException class]]) {
+      SetNSExceptionAnnotations(exception, name, reason);
+      address_array = [exception callStackReturnAddresses];
+    }
 
     if ([address_array count] > 0) {
       static StringAnnotation<256> name_key("UncaughtNSException");
@@ -258,11 +262,8 @@ class ExceptionPreprocessorState {
   // Restore the objc_setExceptionPreprocessor and NSUncaughtExceptionHandler.
   void Uninstall();
 
-  NSException* last_exception() { return last_exception_; }
-  void set_last_exception(NSException* exception) {
-    [last_exception_ release];
-    last_exception_ = [exception retain];
-  }
+  void* last_exception() { return last_exception_; }
+  void set_last_exception(void* exception) { last_exception_ = exception; }
 
  private:
   ExceptionPreprocessorState() = default;
@@ -272,9 +273,11 @@ class ExceptionPreprocessorState {
   // HANDLE_UNCAUGHT_NSEXCEPTION.
   base::FilePath last_handled_intermediate_dump_;
 
-  // Recorded last NSException in case the exception is caught and thrown again
-  // (without using objc_exception_rethrow.)
-  NSException* last_exception_ = nil;
+  // Recorded last NSException pointer in case the exception is caught and
+  // thrown again (without using objc_exception_rethrow) as an
+  // unsafe_unretained reference. Stored as a void* as the only safe
+  // operation is pointer comparison.
+  std::atomic<void*> last_exception_ = nil;
 
   ObjcExceptionDelegate* exception_delegate_ = nullptr;
   objc_exception_preprocessor next_preprocessor_ = nullptr;
@@ -291,7 +294,9 @@ static __attribute__((noinline)) id HANDLE_UNCAUGHT_NSEXCEPTION(
     id exception,
     const char* sinkhole) {
   std::string name, reason;
-  SetNSExceptionAnnotations(exception, name, reason);
+  if ([exception isKindOfClass:[NSException class]]) {
+    SetNSExceptionAnnotations(exception, name, reason);
+  }
   LOG(WARNING) << "Handling Objective-C exception name: " << name
                << " reason: " << reason << " with sinkhole: " << sinkhole;
   NativeCPUContext cpu_context{};
@@ -326,7 +331,7 @@ id ObjcExceptionPreprocessor(id exception) {
   // ignore it.
   ExceptionPreprocessorState* preprocessor_state =
       ExceptionPreprocessorState::Get();
-  if ([preprocessor_state->last_exception() isEqual:exception]) {
+  if (preprocessor_state->last_exception() == exception) {
     return preprocessor_state->MaybeCallNextPreprocessor(exception);
   }
   preprocessor_state->set_last_exception(exception);
@@ -339,9 +344,14 @@ id ObjcExceptionPreprocessor(id exception) {
   static StringAnnotation<1024> lastexception_bt("lastexception_bt");
   auto* key = seen_first_exception ? &lastexception : &firstexception;
   auto* bt_key = seen_first_exception ? &lastexception_bt : &firstexception_bt;
-  NSString* value = [NSString
-      stringWithFormat:@"%@ reason %@", [exception name], [exception reason]];
-  key->Set(base::SysNSStringToUTF8(value));
+
+  if ([exception isKindOfClass:[NSException class]]) {
+    NSString* value = [NSString
+        stringWithFormat:@"%@ reason %@", [exception name], [exception reason]];
+    key->Set(base::SysNSStringToUTF8(value));
+  } else {
+    key->Set(base::SysNSStringToUTF8([exception description]));
+  }
 
   // This exception preprocessor runs prior to the one in libobjc, which sets
   // the -[NSException callStackReturnAddresses].

test/ios/crash_type_xctest.mm

@@ -169,6 +169,16 @@
       isEqualToString:@"NSInternalInconsistencyException"]);
 }
 
+- (void)testNotAnNSException {
+  [rootObject_ crashNotAnNSException];
+  // When @throwing something other than an NSException the
+  // UncaughtExceptionHandler is not called, so the application SIGABRTs.
+  [self verifyCrashReportException:EXC_SOFT_SIGNAL];
+  NSNumber* report_exception;
+  XCTAssertTrue([rootObject_ pendingReportExceptionInfo:&report_exception]);
+  XCTAssertEqual(report_exception.intValue, SIGABRT);
+}
+
 - (void)testUnhandledNSException {
   [rootObject_ crashUnhandledNSException];
   [self verifyCrashReportException:crashpad::kMachExceptionFromNSException];
@@ -374,6 +384,17 @@
   XCTAssertEqual([rootObject_ pendingReportCount], 1);
 }
 
+- (void)testSimultaneousNSException {
+  [rootObject_ catchConcurrentNSException];
+
+  // The app should not crash
+  XCTAssertTrue(app_.state == XCUIApplicationStateRunningForeground);
+
+  // No report should be generated.
+  [rootObject_ processIntermediateDumps];
+  XCTAssertEqual([rootObject_ pendingReportCount], 0);
+}
+
 - (void)testCrashInHandlerReentrant {
   XCTAssertTrue(app_.state == XCUIApplicationStateRunningForeground);
   rootObject_ = [EDOClientService rootObjectWithPort:12345];

test/ios/host/cptest_application_delegate.mm

@@ -341,6 +341,10 @@ UIWindow* GetAnyWindow() {
   });
 }
 
+- (void)crashNotAnNSException {
+  @throw @"Boom";
+}
+
 - (void)crashUnhandledNSException {
   std::thread t([self]() {
     @autoreleasepool {
@@ -504,6 +508,34 @@ class CrashThread : public crashpad::Thread {
   mach_thread.Join();
 }
 
+class ThrowNSExceptionThread : public crashpad::Thread {
+ public:
+  explicit ThrowNSExceptionThread() : Thread() {}
+
+ private:
+  void ThreadMain() override {
+    for (int i = 0; i < 300; ++i) {
+      @try {
+        NSArray* empty_array = @[];
+        [empty_array objectAtIndex:42];
+      } @catch (NSException* exception) {
+      } @finally {
+      }
+    }
+  }
+};
+
+- (void)catchConcurrentNSException {
+  std::vector<ThrowNSExceptionThread> race_threads(30);
+  for (ThrowNSExceptionThread& race_thread : race_threads) {
+    race_thread.Start();
+  }
+
+  for (ThrowNSExceptionThread& race_thread : race_threads) {
+    race_thread.Join();
+  }
+}
+
 - (void)crashInHandlerReentrant {
   crashpad::CrashpadClient client_;
   client_.SetMachExceptionCallbackForTesting(abort);

test/ios/host/cptest_shared_object.h

@@ -73,6 +73,9 @@
 // Trigger a crash with an uncaught NSException.
 - (void)crashNSException;
 
+// Trigger a crash throwing something that isn't an NSException (an NSString).
+- (void)crashNotAnNSException;
+
 // Trigger a crash with an uncaught and unhandled NSException.
 - (void)crashUnhandledNSException;
 
@@ -103,6 +106,9 @@
 // Triggers a simulataneous Mach exception and signal in different threads.
 - (void)crashConcurrentSignalAndMach;
 
+// Triggers simultaneous caught NSExceptions
+- (void)catchConcurrentNSException;
+
 // Triggers a SIGABRT signal while handling an NSException to test reentrant
 // exceptions.
 - (void)crashInHandlerReentrant;