From 3215ed908690697d6b2567ac1ca3f68a0f024014 Mon Sep 17 00:00:00 2001 From: Ben Hamilton Date: Tue, 31 Jan 2023 14:28:15 -0700 Subject: [PATCH] [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 Commit-Queue: Ben Hamilton Reviewed-by: Robert Sesek --- client/annotation.h | 83 ++++++++++++-- client/annotation_test.cc | 102 ++++++++++++++++++ .../crashpad_types/image_annotation_reader.cc | 2 + 3 files changed, 181 insertions(+), 6 deletions(-) diff --git a/client/annotation.h b/client/annotation.h index c4ab074d..e80e7664 100644 --- a/client/annotation.h +++ b/client/annotation.h @@ -17,6 +17,7 @@ #include #include +#include #include #include @@ -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) - : 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; @@ -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 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. diff --git a/client/annotation_test.cc b/client/annotation_test.cc index 5512e3fb..f1a4aa7d 100644 --- a/client/annotation_test.cc +++ b/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}, diff --git a/snapshot/crashpad_types/image_annotation_reader.cc b/snapshot/crashpad_types/image_annotation_reader.cc index ea9cb632..b95a8a8b 100644 --- a/snapshot/crashpad_types/image_annotation_reader.cc +++ b/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