Add Update method to CrashpadInfo

This change adds a method to update a CrashpadInfo stream.

As part of this change, AddUserDataMinidumpStream() now returns a handle
to the added stream. This handle can be passed to
UpdateUserDataMinidumpStream() if a new version of the stream needs to
be attached to the crash report.

This method is needed for e.g., allowing Chrome's System Profile to
update, as it contains some data that takes a while to collect.

Bug: crashpad:474
Change-Id: I19e935a6904d8843215582e5606b189479ee338b
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5260024
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Jesse McKenna <jessemckenna@google.com>

client/BUILD.gn

@@ -174,6 +174,7 @@ source_set("client_test") {
     "annotation_list_test.cc",
     "annotation_test.cc",
     "crash_report_database_test.cc",
+    "crashpad_info_test.cc",
     "length_delimited_ring_buffer_test.cc",
     "prune_crash_reports_test.cc",
     "ring_buffer_annotation_test.cc",

client/crashpad_info.cc

@@ -16,6 +16,7 @@
 
 #include <type_traits>
 
+#include "base/numerics/safe_conversions.h"
 #include "build/build_config.h"
 #include "util/misc/address_sanitizer.h"
 #include "util/misc/from_pointer_cast.h"
@@ -33,6 +34,21 @@ namespace {
 // understand new versions.
 constexpr uint32_t kCrashpadInfoVersion = 1;
 
+// Creates a `UserDataMinidumpStreamListEntry` with the given fields, and
+// returns a pointer to it. The caller takes ownership of the returned object.
+crashpad::internal::UserDataMinidumpStreamListEntry* CreateListEntry(
+    uint64_t next,
+    uint32_t stream_type,
+    const void* data,
+    size_t size) {
+  auto to_be_added = new crashpad::internal::UserDataMinidumpStreamListEntry();
+  to_be_added->next = next;
+  to_be_added->stream_type = stream_type;
+  to_be_added->base_address = crashpad::FromPointerCast<uint64_t>(data);
+  to_be_added->size = base::checked_cast<uint64_t>(size);
+  return to_be_added;
+}
+
 }  // namespace
 
 namespace crashpad {
@@ -123,16 +139,50 @@ CrashpadInfo::CrashpadInfo()
       user_data_minidump_stream_head_(nullptr),
       annotations_list_(nullptr) {}
 
-void CrashpadInfo::AddUserDataMinidumpStream(uint32_t stream_type,
-                                             const void* data,
-                                             size_t size) {
-  auto to_be_added = new internal::UserDataMinidumpStreamListEntry();
-  to_be_added->next =
-      FromPointerCast<uint64_t>(user_data_minidump_stream_head_);
-  to_be_added->stream_type = stream_type;
-  to_be_added->base_address = FromPointerCast<uint64_t>(data);
-  to_be_added->size = base::checked_cast<uint64_t>(size);
-  user_data_minidump_stream_head_ = to_be_added;
+UserDataMinidumpStreamHandle* CrashpadInfo::AddUserDataMinidumpStream(
+    uint32_t stream_type,
+    const void* data,
+    size_t size) {
+  user_data_minidump_stream_head_ = CreateListEntry(
+      crashpad::FromPointerCast<uint64_t>(user_data_minidump_stream_head_),
+      stream_type,
+      data,
+      size);
+  return user_data_minidump_stream_head_;
+}
+
+UserDataMinidumpStreamHandle* CrashpadInfo::UpdateUserDataMinidumpStream(
+    UserDataMinidumpStreamHandle* stream_to_update,
+    uint32_t stream_type,
+    const void* data,
+    size_t size) {
+  // Create a new stream that points to the node `stream_to_update` points to.
+  const auto new_stream =
+      CreateListEntry(stream_to_update->next, stream_type, data, size);
+
+  // If `stream_to_update` is head of the list, replace the head with
+  // `new_stream`.
+  if (stream_to_update == user_data_minidump_stream_head_) {
+    user_data_minidump_stream_head_ = new_stream;
+  } else {
+    // Otherwise, find the node before `stream_to_update`, and make it point to
+    // `new_stream` instead.
+    auto current = user_data_minidump_stream_head_;
+    while (current) {
+      auto next = reinterpret_cast<internal::UserDataMinidumpStreamListEntry*>(
+          current->next);
+      if (next == stream_to_update) {
+        current->next = FromPointerCast<uint64_t>(new_stream);
+        break;
+      }
+      current = next;
+    }
+    CHECK(current)
+        << "Tried to update a UserDataMinidumpStream that doesn't exist";
+  }
+
+  delete stream_to_update;
+  return new_stream;
 }
 
 }  // namespace crashpad

client/crashpad_info.h

@@ -56,6 +56,8 @@ struct UserDataMinidumpStreamListEntry {
 
 }  // namespace internal
 
+using UserDataMinidumpStreamHandle = internal::UserDataMinidumpStreamListEntry;
+
 //! \brief A structure that can be used by a Crashpad-enabled program to
 //!     provide information to the Crashpad crash handler.
 //!
@@ -221,9 +223,41 @@ struct CrashpadInfo {
   //!     which is `0xffff`.
   //! \param[in] data The base pointer of the stream data.
   //! \param[in] size The size of the stream data.
-  void AddUserDataMinidumpStream(uint32_t stream_type,
-                                 const void* data,
-                                 size_t size);
+  //! \return A handle to the added stream, for use in calling
+  //!     UpdateUserDataMinidumpStream() if needed.
+  UserDataMinidumpStreamHandle* AddUserDataMinidumpStream(uint32_t stream_type,
+                                                          const void* data,
+                                                          size_t size);
+
+  //! \brief Replaces the given stream with an updated stream.
+  //!
+  //! Creates a new memory block referencing the given \a data and \a size with
+  //! type \a stream_type. The memory referred to be \a data and \a size is
+  //! owned by the caller and must remain valid while it is in effect for the
+  //! CrashpadInfo object.
+  //!
+  //! Frees \a stream_to_update and returns a new handle to the updated stream.
+  //!
+  //! \param[in] stream_to_update A handle to the stream to be updated, received
+  //!     from either AddUserDataMinidumpStream() or previous calls to this
+  //!     function.
+  //! \param[in] stream_type The stream type identifier to use. This should be
+  //!     normally be larger than `MINIDUMP_STREAM_TYPE::LastReservedStream`
+  //!     which is `0xffff`.
+  //! \param[in] data The base pointer of the stream data.
+  //! \param[in] size The size of the stream data.
+  //! \return A handle to the new memory block that references the updated data,
+  //!     for use in calling this method again if needed.
+  UserDataMinidumpStreamHandle* UpdateUserDataMinidumpStream(
+      UserDataMinidumpStreamHandle* stream_to_update,
+      uint32_t stream_type,
+      const void* data,
+      size_t size);
+
+  internal::UserDataMinidumpStreamListEntry*
+  GetUserDataMinidumpStreamHeadForTesting() {
+    return user_data_minidump_stream_head_;
+  }
 
   enum : uint32_t {
     kSignature = 'CPad',

client/crashpad_info_test.cc

@@ -0,0 +1,131 @@
+// Copyright 2024 The Crashpad Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "client/crashpad_info.h"
+
+#include <string>
+
+#include "gtest/gtest.h"
+
+namespace crashpad {
+namespace test {
+namespace {
+
+constexpr uint32_t kTestStreamType = 0x33333;
+
+class CrashpadInfoTest : public testing::Test {
+ protected:
+  CrashpadInfo& crashpad_info() { return crashpad_info_; }
+
+  // Returns the current head of the list of streams in `crashpad_info_`. Note
+  // that the returned pointer is invalidated if a stream is added or updated.
+  internal::UserDataMinidumpStreamListEntry* GetCurrentHead() {
+    return crashpad_info().GetUserDataMinidumpStreamHeadForTesting();
+  }
+
+  // Returns a pointer to the next node in the list after the given `node`.
+  internal::UserDataMinidumpStreamListEntry* GetNext(
+      internal::UserDataMinidumpStreamListEntry* node) {
+    return reinterpret_cast<internal::UserDataMinidumpStreamListEntry*>(
+        node->next);
+  }
+
+  internal::UserDataMinidumpStreamListEntry* initial_head() {
+    return initial_head_;
+  }
+
+  internal::UserDataMinidumpStreamListEntry* initial_tail() {
+    return initial_tail_;
+  }
+
+ private:
+  void SetUp() override {
+    ASSERT_EQ(nullptr, GetCurrentHead());
+
+    // Create a simple test list with the structure
+    // `initial_head_` -> `initial_tail_`.
+    initial_tail_ = AddStream(0x11111, kInitialTailData);
+    initial_head_ = AddStream(0x22222, kInitialHeadData);
+
+    // Validate the list's contents.
+    auto current = GetCurrentHead();
+    ASSERT_EQ(initial_head_, current);
+    ASSERT_EQ(kInitialHeadData, reinterpret_cast<char*>(current->base_address));
+    current = GetNext(current);
+    ASSERT_EQ(initial_tail_, current);
+    ASSERT_EQ(nullptr, GetNext(current));
+  }
+
+  internal::UserDataMinidumpStreamListEntry* AddStream(uint32_t stream_type,
+                                                       const char* data) {
+    return reinterpret_cast<internal::UserDataMinidumpStreamListEntry*>(
+        crashpad_info().AddUserDataMinidumpStream(
+            stream_type, data, strlen(data)));
+  }
+
+  CrashpadInfo crashpad_info_;
+
+  static constexpr char kInitialHeadData[] = "head";
+  static constexpr char kInitialTailData[] = "tail";
+
+  internal::UserDataMinidumpStreamListEntry* initial_head_ = nullptr;
+  internal::UserDataMinidumpStreamListEntry* initial_tail_ = nullptr;
+};
+
+// Tests that updating the head of the list updates the head pointer, the new
+// head contains the updated data, and the updated node points to the next node.
+TEST_F(CrashpadInfoTest, UpdateUserDataMinidumpStreamHead) {
+  const std::string new_data = "this is a new string";
+  const auto new_entry = crashpad_info().UpdateUserDataMinidumpStream(
+      initial_head(), kTestStreamType, new_data.data(), new_data.size());
+  const auto head = GetCurrentHead();
+  EXPECT_EQ(new_entry, head);
+  EXPECT_EQ(new_data.data(), reinterpret_cast<char*>(head->base_address));
+  EXPECT_EQ(new_data.size(), head->size);
+  EXPECT_EQ(kTestStreamType, head->stream_type);
+  EXPECT_EQ(initial_tail(), GetNext(head));
+}
+
+// Tests that updating the tail of the list results in a tail pointing to
+// nullptr, and that the node before the updated node points to it.
+TEST_F(CrashpadInfoTest, UpdateUserDataMinidumpStreamTail) {
+  const std::string new_data = "new";
+  const auto new_entry = crashpad_info().UpdateUserDataMinidumpStream(
+      initial_tail(), kTestStreamType, new_data.data(), new_data.size());
+  const auto tail = GetNext(GetCurrentHead());
+  EXPECT_EQ(new_entry, tail);
+  EXPECT_EQ(nullptr, GetNext(tail));
+}
+
+// Tests that the handle returned from updating an entry is usable for updating
+// the entry again.
+TEST_F(CrashpadInfoTest, UpdateUserDataMinidumpStreamMultipleTimes) {
+  // Update the entry at the head; the updated entry should become the new head.
+  const std::string new_data = "new";
+  const auto new_entry_1 = crashpad_info().UpdateUserDataMinidumpStream(
+      initial_head(), kTestStreamType, new_data.data(), new_data.size());
+  EXPECT_EQ(new_entry_1, GetCurrentHead());
+
+  // Update the updated entry again; another new entry should replace it as
+  // head.
+  const auto new_entry_2 = crashpad_info().UpdateUserDataMinidumpStream(
+      new_entry_1, kTestStreamType, new_data.data(), new_data.size());
+  EXPECT_NE(new_entry_1, new_entry_2);
+  EXPECT_EQ(new_entry_2, GetCurrentHead());
+  EXPECT_EQ(initial_tail(), GetNext(GetCurrentHead()));
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace crashpad