[ios] Support guarding concurrent reads and writes to Annotations

Since iOS reads Annotations in-process, this CL updates the iOS
intermediate dump handler to check each Annotation to see if it supports
guarding concurrent reads and writes using ScopedSpinGuard.

For any such Annotation, the in-process dump handler now tries (without
spinning) to obtain the ScopedSpinGuard for the Annotation before
reading its memory.

If the ScopedSpinGuard cannot immediately be obtained, the in-process
dump handler just skips writing the memory of the Annotation to the
intermediate dump. (I'd like to follow up and thread down a Params
object so we can experiment with adding an optional timeout to make
this more reliable.)

Change-Id: Ie6c9849fac94ab89b36364b07aea62326cabe552
Bug: crashpad:437
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4031730
Commit-Queue: Ben Hamilton <benhamilton@google.com>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>

client/ios_handler/in_process_intermediate_dump_handler.cc

@@ -22,12 +22,15 @@
 #include <time.h>
 
 #include <iterator>
+#include <optional>
 
 #include "build/build_config.h"
 #include "snapshot/snapshot_constants.h"
 #include "util/ios/ios_intermediate_dump_writer.h"
 #include "util/ios/raw_logging.h"
+#include "util/ios/scoped_vm_map.h"
 #include "util/ios/scoped_vm_read.h"
+#include "util/synchronization/scoped_spin_guard.h"
 
 namespace crashpad {
 namespace internal {
@@ -1163,6 +1166,13 @@ void InProcessIntermediateDumpHandler::WriteCrashpadAnnotationsList(
   IOSIntermediateDumpWriter::ScopedArray annotations_array(
       writer, IntermediateDumpKey::kAnnotationObjects);
   ScopedVMRead<Annotation> current;
+
+  // Use vm_read() to ensure that the linked-list AnnotationList head (which is
+  // a dummy node of type kInvalid) is valid and copy its memory into a
+  // newly-allocated buffer.
+  //
+  // In the case where the pointer has been clobbered or the memory range is not
+  // readable, skip reading all the Annotations.
   if (!current.Read(annotation_list->head())) {
     CRASHPAD_RAW_LOG("Unable to read annotation");
     return;
@@ -1173,6 +1183,12 @@ void InProcessIntermediateDumpHandler::WriteCrashpadAnnotationsList(
        index < kMaxNumberOfAnnotations;
        ++index) {
     ScopedVMRead<Annotation> node;
+
+    // Like above, use vm_read() to ensure that the node in the linked list is
+    // valid and copy its memory into a newly-allocated buffer.
+    //
+    // In the case where the pointer has been clobbered or the memory range is
+    // not readable, skip reading this and all further Annotations.
     if (!node.Read(current->link_node())) {
       CRASHPAD_RAW_LOG("Unable to read annotation");
       return;
@@ -1187,6 +1203,36 @@ void InProcessIntermediateDumpHandler::WriteCrashpadAnnotationsList(
       continue;
     }
 
+    // For Annotations which support guarding reads from concurrent writes, map
+    // their memory read-write using vm_remap(), then declare a ScopedSpinGuard
+    // which lives for the duration of the read.
+    ScopedVMMap<Annotation> mapped_node;
+    std::optional<ScopedSpinGuard> annotation_guard;
+    if (node->concurrent_access_guard_mode() ==
+        Annotation::ConcurrentAccessGuardMode::kScopedSpinGuard) {
+      constexpr vm_prot_t kDesiredProtection = VM_PROT_WRITE | VM_PROT_READ;
+      if (!mapped_node.Map(node.get()) ||
+          (mapped_node.CurrentProtection() & kDesiredProtection) !=
+              kDesiredProtection) {
+        CRASHPAD_RAW_LOG("Unable to map annotation");
+
+        // Skip this annotation rather than giving up entirely, since the linked
+        // node should still be valid.
+        continue;
+      }
+
+      // TODO(https://crbug.com/crashpad/438): Pass down a `params` object into
+      // this method to optionally enable a timeout here.
+      constexpr uint64_t kTimeoutNanoseconds = 0;
+      annotation_guard =
+          mapped_node->TryCreateScopedSpinGuard(kTimeoutNanoseconds);
+      if (!annotation_guard) {
+        // This is expected if the process is writing to the Annotation, so
+        // don't log here and skip the annotation.
+        continue;
+      }
+    }
+
     IOSIntermediateDumpWriter::ScopedArrayMap annotation_map(writer);
     WritePropertyCString(writer,
                          IntermediateDumpKey::kAnnotationName,

test/ios/BUILD.gn

@@ -70,6 +70,7 @@ source_set("xcuitests") {
   deps = [
     "../../build:ios_enable_arc",
     "../../build:ios_xctest",
+    "../../client:common",
     "../../test/ios/host:app_shared_sources",
     "../../third_party/edo",
     "../../util",

test/ios/crash_type_xctest.mm

@@ -15,8 +15,11 @@
 #import <XCTest/XCTest.h>
 #include <objc/runtime.h>
 
+#include <vector>
+
 #import "Service/Sources/EDOClientService.h"
 #include "build/build_config.h"
+#include "client/length_delimited_ring_buffer.h"
 #import "test/ios/host/cptest_shared_object.h"
 #include "util/mach/exception_types.h"
 #include "util/mach/mach_extensions.h"
@@ -322,6 +325,31 @@
       isEqualToString:@"same-name 3"]);
   XCTAssertTrue([[dict[@"objects"][2] valueForKeyPath:@"#TEST# one"]
       isEqualToString:@"moocow"]);
+  // Ensure `ring_buffer` is present but not `busy_ring_buffer`.
+  XCTAssertEqual(1u, [dict[@"ringbuffers"] count]);
+  NSData* ringBufferNSData =
+      [dict[@"ringbuffers"][0] valueForKeyPath:@"#TEST# ring_buffer"];
+  crashpad::RingBufferData ringBufferData;
+  XCTAssertTrue(ringBufferData.DeserializeFromBuffer(ringBufferNSData.bytes,
+                                                     ringBufferNSData.length));
+  crashpad::LengthDelimitedRingBufferReader reader(ringBufferData);
+
+  std::vector<uint8_t> ringBufferEntry;
+  XCTAssertTrue(reader.Pop(ringBufferEntry));
+  NSString* firstEntry = [[NSString alloc] initWithBytes:ringBufferEntry.data()
+                                                  length:ringBufferEntry.size()
+                                                encoding:NSUTF8StringEncoding];
+  XCTAssertEqualObjects(firstEntry, @"hello");
+  ringBufferEntry.clear();
+
+  XCTAssertTrue(reader.Pop(ringBufferEntry));
+  NSString* secondEntry = [[NSString alloc] initWithBytes:ringBufferEntry.data()
+                                                   length:ringBufferEntry.size()
+                                                 encoding:NSUTF8StringEncoding];
+  XCTAssertEqualObjects(secondEntry, @"goodbye");
+  ringBufferEntry.clear();
+
+  XCTAssertFalse(reader.Pop(ringBufferEntry));
 }
 
 - (void)testDumpWithoutCrash {

test/ios/host/cptest_application_delegate.mm

@@ -38,6 +38,7 @@
 #include "client/crash_report_database.h"
 #include "client/crashpad_client.h"
 #include "client/crashpad_info.h"
+#include "client/ring_buffer_annotation.h"
 #include "client/simple_string_dictionary.h"
 #include "client/simulate_crash.h"
 #include "snapshot/minidump/process_snapshot_minidump.h"
@@ -58,6 +59,9 @@ using Report = crashpad::CrashReportDatabase::Report;
 
 namespace {
 
+constexpr crashpad::Annotation::Type kRingBufferType =
+    crashpad::Annotation::UserDefinedType(42);
+
 base::FilePath GetDatabaseDir() {
   base::FilePath database_dir([NSFileManager.defaultManager
                                   URLsForDirectory:NSDocumentDirectory
@@ -251,7 +255,8 @@ UIWindow* GetAnyWindow() {
   NSDictionary* dict = @{
     @"simplemap" : [@{} mutableCopy],
     @"vector" : [@[] mutableCopy],
-    @"objects" : [@[] mutableCopy]
+    @"objects" : [@[] mutableCopy],
+    @"ringbuffers" : [@[] mutableCopy],
   };
   for (const auto* module : process_snapshot->Modules()) {
     for (const auto& kv : module->AnnotationsSimpleMap()) {
@@ -262,14 +267,18 @@ UIWindow* GetAnyWindow() {
       [dict[@"vector"] addObject:@(annotation.c_str())];
     }
     for (const auto& annotation : module->AnnotationObjects()) {
-      if (annotation.type !=
+      if (annotation.type ==
           static_cast<uint16_t>(crashpad::Annotation::Type::kString)) {
-        continue;
+        std::string value(
-      }
+            reinterpret_cast<const char*>(annotation.value.data()),
-      std::string value(reinterpret_cast<const char*>(annotation.value.data()),
             annotation.value.size());
         [dict[@"objects"]
             addObject:@{@(annotation.name.c_str()) : @(value.c_str())}];
+      } else if (annotation.type == static_cast<uint16_t>(kRingBufferType)) {
+        NSData* data = [NSData dataWithBytes:annotation.value.data()
+                                      length:annotation.value.size()];
+        [dict[@"ringbuffers"] addObject:@{@(annotation.name.c_str()) : data}];
+      }
     }
   }
   return [dict passByValue];
@@ -418,12 +427,23 @@ UIWindow* GetAnyWindow() {
       "#TEST# same-name"};
   static crashpad::StringAnnotation<32> test_annotation_four{
       "#TEST# same-name"};
+  static crashpad::RingBufferAnnotation<32> test_ring_buffer_annotation(
+      kRingBufferType, "#TEST# ring_buffer");
+  static crashpad::RingBufferAnnotation<32> test_busy_ring_buffer_annotation(
+      kRingBufferType, "#TEST# busy_ring_buffer");
 
   test_annotation_one.Set("moocow");
   test_annotation_two.Set("this will be cleared");
   test_annotation_three.Set("same-name 3");
   test_annotation_four.Set("same-name 4");
   test_annotation_two.Clear();
+  test_ring_buffer_annotation.Push("hello", 5);
+  test_ring_buffer_annotation.Push("goodbye", 7);
+  test_busy_ring_buffer_annotation.Push("busy", 4);
+  // Take the scoped spin guard on `test_busy_ring_buffer_annotation` to mimic
+  // an in-flight `Push()` so its contents are not included in the dump.
+  auto guard = test_busy_ring_buffer_annotation.TryCreateScopedSpinGuard(
+      /*timeout_nanos=*/0);
   abort();
 }