Update Crashpad to 70e0f9215346ea62e89dd4ff7ffdfdd948858403

3215ed908690 [client] Optionally support ScopedSpinGuard in Annotation
212b8f6b8cb1 [client] New RingBufferAnnotation
c7d9c710f2b2 [ios] Support guarding concurrent reads and writes to
             Annotations
0adab59836b1 ios: Validate exception code buffer size before read
485805c6fea2 Fix test that relied on NDEBUG always disabling DCHECK
04b2ab5bba91 port: fix non-glibc desktop linux build
70e0f9215346 Fix StringPiece compile issue in Chromium

Change-Id: I988195f97a7ab63ea140f96954190415ac9a2925
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4266111
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Auto-Submit: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1107205}

third_party/crashpad/README.chromium

@@ -2,7 +2,7 @@ Name: Crashpad
 Short Name: crashpad
 URL: https://crashpad.chromium.org/
 Version: unknown
-Revision: 9158eb7caa811b684c6bec7f9e9b5f52389d0ce0
+Revision: 70e0f9215346ea62e89dd4ff7ffdfdd948858403
 License: Apache 2.0
 License File: crashpad/LICENSE
 Security Critical: yes

third_party/crashpad/crashpad/client/BUILD.gn

@@ -120,6 +120,7 @@ static_library("common") {
     "crashpad_info.cc",
     "crashpad_info.h",
     "length_delimited_ring_buffer.h",
+    "ring_buffer_annotation.h",
     "settings.cc",
     "settings.h",
     "simple_address_range_bag.h",
@@ -148,6 +149,18 @@ static_library("common") {
   configs += [ "../build:flock_always_supported_defines" ]
 }
 
+crashpad_executable("ring_buffer_annotation_load_test") {
+  testonly = true
+  sources = [
+    "ring_buffer_annotation_load_test_main.cc",
+  ]
+  deps = [
+    ":client",
+    "../tools:tool_support",
+    "$mini_chromium_source_parent:base",
+  ]
+}
+
 source_set("client_test") {
   testonly = true
 
@@ -157,6 +170,7 @@ source_set("client_test") {
     "crash_report_database_test.cc",
     "length_delimited_ring_buffer_test.cc",
     "prune_crash_reports_test.cc",
+    "ring_buffer_annotation_test.cc",
     "settings_test.cc",
     "simple_address_range_bag_test.cc",
     "simple_string_dictionary_test.cc",

third_party/crashpad/crashpad/client/annotation.h

@@ -17,6 +17,7 @@
 
 #include <algorithm>
 #include <atomic>
+#include <optional>
 #include <ostream>
 
 #include <stdint.h>
@@ -27,6 +28,7 @@
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_piece.h"
 #include "build/build_config.h"
+#include "util/synchronization/scoped_spin_guard.h"
 
 namespace crashpad {
 #if BUILDFLAG(IS_IOS)
@@ -95,6 +97,20 @@ class Annotation {
     kUserDefinedStart = 0x8000,
   };
 
+  //! \brief Mode used to guard concurrent reads from writes.
+  enum class ConcurrentAccessGuardMode : bool {
+    //! \!brief Annotation does not guard reads from concurrent
+    //!     writes. Annotation values can be corrupted if the process crashes
+    //!     mid-write and the handler tries to read from the Annotation while
+    //!     being written to.
+    kUnguarded = false,
+
+    //! \!brief Annotation guards reads from concurrent writes using
+    //!     ScopedSpinGuard. Clients must use TryCreateScopedSpinGuard()
+    //!     before reading or writing the data in this Annotation.
+    kScopedSpinGuard = true,
+  };
+
   //! \brief Creates a user-defined Annotation::Type.
   //!
   //! This exists to remove the casting overhead of `enum class`.
@@ -133,12 +149,11 @@ class Annotation {
   //! \param[in] value_ptr A pointer to the value for the annotation. The
   //!     pointer may not be changed once associated with an annotation, but
   //!     the data may be mutated.
-  constexpr Annotation(Type type, const char name[], void* const value_ptr)
-      : link_node_(nullptr),
-        name_(name),
-        value_ptr_(value_ptr),
-        size_(0),
-        type_(type) {}
+  constexpr Annotation(Type type, const char name[], void* value_ptr)
+      : Annotation(type,
+                   name,
+                   value_ptr,
+                   ConcurrentAccessGuardMode::kUnguarded) {}
 
   Annotation(const Annotation&) = delete;
   Annotation& operator=(const Annotation&) = delete;
@@ -173,7 +188,58 @@ class Annotation {
   const char* name() const { return name_; }
   const void* value() const { return value_ptr_; }
 
+  ConcurrentAccessGuardMode concurrent_access_guard_mode() const {
+    return concurrent_access_guard_mode_;
+  }
+
+  //! \brief If this Annotation guards concurrent access using ScopedSpinGuard,
+  //!     tries to obtain the spin guard and returns the result.
+  //!
+  //! \param[in] timeout_ns The timeout in nanoseconds after which to give up
+  //!     trying to obtain the spin guard.
+  //! \return std::nullopt if the spin guard could not be obtained within
+  //!     timeout_ns, or the obtained spin guard otherwise.
+  std::optional<ScopedSpinGuard> TryCreateScopedSpinGuard(uint64_t timeout_ns) {
+    // This can't use DCHECK_EQ() because ostream doesn't support
+    // operator<<(bool).
+    DCHECK(concurrent_access_guard_mode_ ==
+           ConcurrentAccessGuardMode::kScopedSpinGuard);
+    if (concurrent_access_guard_mode_ ==
+        ConcurrentAccessGuardMode::kUnguarded) {
+      return std::nullopt;
+    }
+    return ScopedSpinGuard::TryCreateScopedSpinGuard(timeout_ns,
+                                                     spin_guard_state_);
+  }
+
  protected:
+  //! \brief Constructs a new annotation.
+  //!
+  //! Upon construction, the annotation will not be included in any crash
+  //! reports until \sa SetSize() is called with a value greater than `0`.
+  //!
+  //! \param[in] type The data type of the value of the annotation.
+  //! \param[in] name A `NUL`-terminated C-string name for the annotation. Names
+  //!     do not have to be unique, though not all crash processors may handle
+  //!     Annotations with the same name. Names should be constexpr data with
+  //!     static storage duration.
+  //! \param[in] value_ptr A pointer to the value for the annotation. The
+  //!     pointer may not be changed once associated with an annotation, but
+  //!     the data may be mutated.
+  //! \param[in] concurrent_access_guard_mode Mode used to guard concurrent
+  //!     reads from writes.
+  constexpr Annotation(Type type,
+                       const char name[],
+                       void* value_ptr,
+                       ConcurrentAccessGuardMode concurrent_access_guard_mode)
+      : link_node_(nullptr),
+        name_(name),
+        value_ptr_(value_ptr),
+        size_(0),
+        type_(type),
+        concurrent_access_guard_mode_(concurrent_access_guard_mode),
+        spin_guard_state_() {}
+
   friend class AnnotationList;
 #if BUILDFLAG(IS_IOS)
   friend class internal::InProcessIntermediateDumpHandler;
@@ -193,6 +259,11 @@ class Annotation {
   void* const value_ptr_;
   ValueSizeType size_;
   const Type type_;
+
+  //! \brief Mode used to guard concurrent reads from writes.
+  const ConcurrentAccessGuardMode concurrent_access_guard_mode_;
+
+  SpinGuardState spin_guard_state_;
 };
 
 //! \brief An \sa Annotation that stores a `NUL`-terminated C-string value.

third_party/crashpad/crashpad/client/annotation_test.cc

@@ -21,11 +21,58 @@
 #include "client/crashpad_info.h"
 #include "gtest/gtest.h"
 #include "test/gtest_death.h"
+#include "util/misc/clock.h"
+#include "util/synchronization/scoped_spin_guard.h"
+#include "util/thread/thread.h"
 
 namespace crashpad {
 namespace test {
 namespace {
 
+class SpinGuardAnnotation final : public Annotation {
+ public:
+  SpinGuardAnnotation(Annotation::Type type, const char name[])
+      : Annotation(type,
+                   name,
+                   &value_,
+                   ConcurrentAccessGuardMode::kScopedSpinGuard) {}
+
+  bool Set(bool value, uint64_t spin_guard_timeout_ns) {
+    auto guard = TryCreateScopedSpinGuard(spin_guard_timeout_ns);
+    if (!guard) {
+      return false;
+    }
+    value_ = value;
+    SetSize(sizeof(value_));
+    return true;
+  }
+
+ private:
+  bool value_;
+};
+
+class ScopedSpinGuardUnlockThread final : public Thread {
+ public:
+  ScopedSpinGuardUnlockThread(ScopedSpinGuard scoped_spin_guard,
+                              uint64_t sleep_time_ns)
+      : scoped_spin_guard_(std::move(scoped_spin_guard)),
+        sleep_time_ns_(sleep_time_ns) {}
+
+ private:
+  void ThreadMain() override {
+    SleepNanoseconds(sleep_time_ns_);
+
+    // Move the ScopedSpinGuard member into a local variable which is
+    // destroyed when ThreadMain() returns.
+    ScopedSpinGuard local_scoped_spin_guard(std::move(scoped_spin_guard_));
+
+    // After this point, local_scoped_spin_guard will be destroyed and unlocked.
+  }
+
+  ScopedSpinGuard scoped_spin_guard_;
+  const uint64_t sleep_time_ns_;
+};
+
 class Annotation : public testing::Test {
  public:
   void SetUp() override {
@@ -108,6 +155,62 @@ TEST_F(Annotation, StringType) {
   EXPECT_EQ("loooo", annotation.value());
 }
 
+TEST_F(Annotation, BaseAnnotationShouldNotSupportSpinGuard) {
+  char buffer[1024];
+  crashpad::Annotation annotation(
+      crashpad::Annotation::Type::kString, "no-spin-guard", buffer);
+  EXPECT_EQ(annotation.concurrent_access_guard_mode(),
+            crashpad::Annotation::ConcurrentAccessGuardMode::kUnguarded);
+#if !DCHECK_IS_ON()
+  // This fails a DCHECK() in debug builds, so only test it when DCHECK()
+  // is not on.
+  EXPECT_EQ(std::nullopt, annotation.TryCreateScopedSpinGuard(0));
+#endif
+}
+
+TEST_F(Annotation, CustomAnnotationShouldSupportSpinGuardAndSet) {
+  constexpr crashpad::Annotation::Type kType =
+      crashpad::Annotation::UserDefinedType(1);
+  SpinGuardAnnotation spin_guard_annotation(kType, "spin-guard");
+  EXPECT_EQ(spin_guard_annotation.concurrent_access_guard_mode(),
+            crashpad::Annotation::ConcurrentAccessGuardMode::kScopedSpinGuard);
+  EXPECT_TRUE(spin_guard_annotation.Set(true, 0));
+  EXPECT_EQ(1U, spin_guard_annotation.size());
+}
+
+TEST_F(Annotation, CustomAnnotationSetShouldFailIfRunConcurrently) {
+  constexpr crashpad::Annotation::Type kType =
+      crashpad::Annotation::UserDefinedType(1);
+  SpinGuardAnnotation spin_guard_annotation(kType, "spin-guard");
+  auto guard = spin_guard_annotation.TryCreateScopedSpinGuard(0);
+  EXPECT_NE(std::nullopt, guard);
+  // This should fail, since the guard is already held and the timeout is 0.
+  EXPECT_FALSE(spin_guard_annotation.Set(true, 0));
+}
+
+TEST_F(Annotation,
+       CustomAnnotationSetShouldSucceedIfSpinGuardUnlockedAsynchronously) {
+  constexpr crashpad::Annotation::Type kType =
+      crashpad::Annotation::UserDefinedType(1);
+  SpinGuardAnnotation spin_guard_annotation(kType, "spin-guard");
+  auto guard = spin_guard_annotation.TryCreateScopedSpinGuard(0);
+  EXPECT_NE(std::nullopt, guard);
+  // Pass the guard off to a background thread which unlocks it after 1 ms.
+  constexpr uint64_t kSleepTimeNs = 1000000;  // 1 ms
+  ScopedSpinGuardUnlockThread unlock_thread(std::move(guard.value()),
+                                            kSleepTimeNs);
+  unlock_thread.Start();
+
+  // Try to set the annotation with a 100 ms timeout.
+  constexpr uint64_t kSpinGuardTimeoutNanos = 100000000;  // 100 ms
+
+  // This should succeed after 1 ms, since the timeout is much larger than the
+  // time the background thread holds the guard.
+  EXPECT_TRUE(spin_guard_annotation.Set(true, kSpinGuardTimeoutNanos));
+
+  unlock_thread.Join();
+}
+
 TEST(StringAnnotation, ArrayOfString) {
   static crashpad::StringAnnotation<4> annotations[] = {
       {"test-1", crashpad::StringAnnotation<4>::Tag::kArray},

third_party/crashpad/crashpad/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,