[client] Optionally support ScopedSpinGuard in Annotation

This CL optionally integrates ScopedSpinGuard (an atomic boolean) with
crashpad::Annotation.

Subclasses of Annotation can choose to integrate ScopedSpinGuard into
their Set(...) methods to ensure reads and writes are serialized.

I didn't integrate this into StringAnnotation in this CL, but it'd be
pretty trivial to do in a follow-up.

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

client/annotation.h

@@ -17,6 +17,7 @@
 
 #include <algorithm>
 #include <atomic>
+#include <optional>
 
 #include <stdint.h>
 #include <string.h>
@@ -26,6 +27,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)
@@ -94,6 +96,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`.
@@ -132,12 +148,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)
+  constexpr Annotation(Type type, const char name[], void* value_ptr)
-      : link_node_(nullptr),
+      : Annotation(type,
-        name_(name),
+                   name,
-        value_ptr_(value_ptr),
+                   value_ptr,
-        size_(0),
+                   ConcurrentAccessGuardMode::kUnguarded) {}
-        type_(type) {}
 
   Annotation(const Annotation&) = delete;
   Annotation& operator=(const Annotation&) = delete;
@@ -172,7 +187,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;
@@ -192,6 +258,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.

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,61 @@ 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);
+#ifdef NDEBUG
+  // This fails a DCHECK() in debug builds, so only test it for NDEBUG builds.
+  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},

snapshot/crashpad_types/image_annotation_reader.cc

@@ -43,6 +43,8 @@ struct Annotation {
   typename Traits::Address value;
   uint32_t size;
   uint16_t type;
+  crashpad::Annotation::ConcurrentAccessGuardMode concurrent_access_guard_mode;
+  bool spin_guard_state;
 };
 
 template <class Traits>