Don't fail minidump write if memory snapshot read fails

On Windows (and probably elsewhere) it's possible that something else on
the system changes the memory map between when a memory snapshot range
is added to the minidump, and when the process's memory is actually read
from the target and written to the .dmp file. As a result, failing the
Read() should not result in aborting the minidump's write, which it
previously would have.

Bug: crashpad:234
Change-Id: Ib24e255a34fa2e1758621d3955ebc7a0f96166e2
Reviewed-on: https://chromium-review.googlesource.com/1096452
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>

minidump/minidump_memory_writer.cc

@@ -52,7 +52,17 @@ bool SnapshotMinidumpMemoryWriter::WriteObject(
                                                           file_writer);
 
   // This will result in MemorySnapshotDelegateRead() being called.
-  return memory_snapshot_->Read(this);
+  if (!memory_snapshot_->Read(this)) {
+    // If the Read() fails (perhaps because the process' memory map has changed
+    // since it the range was captured), write an empty block of memory. It
+    // would be nice to instead not include this memory, but at this point in
+    // the writing process, it would be difficult to amend the minidump's
+    // structure. See https://crashpad.chromium.org/234 for background.
+    std::vector<uint8_t> empty(memory_snapshot_->Size(), 0xfe);
+    MemorySnapshotDelegateRead(empty.data(), empty.size());
+  }
+
+  return true;
 }
 
 const MINIDUMP_MEMORY_DESCRIPTOR*

minidump/minidump_memory_writer_test.cc

@@ -188,6 +188,47 @@ TEST(MinidumpMemoryWriter, TwoMemoryRegions) {
   }
 }
 
+TEST(MinidumpMemoryWriter, RegionReadFails) {
+  MinidumpFileWriter minidump_file_writer;
+  auto memory_list_writer = std::make_unique<MinidumpMemoryListWriter>();
+
+  constexpr uint64_t kBaseAddress = 0xfedcba9876543210;
+  constexpr size_t kSize = 0x1000;
+  constexpr uint8_t kValue = 'm';
+
+  auto memory_writer =
+      std::make_unique<TestMinidumpMemoryWriter>(kBaseAddress, kSize, kValue);
+
+  // Make the read of that memory fail.
+  memory_writer->SetShouldFailRead(true);
+
+  memory_list_writer->AddMemory(std::move(memory_writer));
+
+  ASSERT_TRUE(minidump_file_writer.AddStream(std::move(memory_list_writer)));
+
+  StringFile string_file;
+  ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file));
+
+  const MINIDUMP_MEMORY_LIST* memory_list = nullptr;
+  ASSERT_NO_FATAL_FAILURE(
+      GetMemoryListStream(string_file.string(), &memory_list, 1));
+
+  MINIDUMP_MEMORY_DESCRIPTOR expected;
+  expected.StartOfMemoryRange = kBaseAddress;
+  expected.Memory.DataSize = kSize;
+  expected.Memory.Rva =
+      sizeof(MINIDUMP_HEADER) + sizeof(MINIDUMP_DIRECTORY) +
+      sizeof(MINIDUMP_MEMORY_LIST) +
+      memory_list->NumberOfMemoryRanges * sizeof(MINIDUMP_MEMORY_DESCRIPTOR);
+  ExpectMinidumpMemoryDescriptorAndContents(
+      &expected,
+      &memory_list->MemoryRanges[0],
+      string_file.string(),
+      0xfe,  // Not kValue ('m'), but the value that the implementation inserts
+             // if memory is unreadable.
+      true);
+}
+
 class TestMemoryStream final : public internal::MinidumpStreamWriter {
  public:
   TestMemoryStream(uint64_t base_address, size_t size, uint8_t value)

minidump/test/minidump_memory_writer_test_util.cc

@@ -31,6 +31,10 @@ TestMinidumpMemoryWriter::TestMinidumpMemoryWriter(uint64_t base_address,
 TestMinidumpMemoryWriter::~TestMinidumpMemoryWriter() {
 }
 
+void TestMinidumpMemoryWriter::SetShouldFailRead(bool should_fail) {
+  test_snapshot_.SetShouldFailRead(should_fail);
+}
+
 void ExpectMinidumpMemoryDescriptor(
     const MINIDUMP_MEMORY_DESCRIPTOR* expected,
     const MINIDUMP_MEMORY_DESCRIPTOR* observed) {

minidump/test/minidump_memory_writer_test_util.h

@@ -40,6 +40,8 @@ class TestMinidumpMemoryWriter final : public SnapshotMinidumpMemoryWriter {
   TestMinidumpMemoryWriter(uint64_t base_address, size_t size, uint8_t value);
   ~TestMinidumpMemoryWriter();
 
+  void SetShouldFailRead(bool should_fail);
+
  private:
   TestMemorySnapshot test_snapshot_;
 

snapshot/test/test_memory_snapshot.cc

@@ -21,7 +21,7 @@ namespace crashpad {
 namespace test {
 
 TestMemorySnapshot::TestMemorySnapshot()
-    : address_(0), size_(0), value_('\0') {
+    : address_(0), size_(0), value_('\0'), should_fail_(false) {
 }
 
 TestMemorySnapshot::~TestMemorySnapshot() {
@@ -36,6 +36,10 @@ size_t TestMemorySnapshot::Size() const {
 }
 
 bool TestMemorySnapshot::Read(Delegate* delegate) const {
+  if (should_fail_) {
+    return false;
+  }
+
   if (size_ == 0) {
     return delegate->MemorySnapshotDelegateRead(nullptr, size_);
   }

snapshot/test/test_memory_snapshot.h

@@ -40,6 +40,8 @@ class TestMemorySnapshot final : public MemorySnapshot {
   //!     called. This value will be repeated Size() times.
   void SetValue(char value) { value_ = value; }
 
+  void SetShouldFailRead(bool should_fail) { should_fail_ = true; }
+
   // MemorySnapshot:
 
   uint64_t Address() const override;
@@ -52,6 +54,7 @@ class TestMemorySnapshot final : public MemorySnapshot {
   uint64_t address_;
   size_t size_;
   char value_;
+  bool should_fail_;
 
   DISALLOW_COPY_AND_ASSIGN(TestMemorySnapshot);
 };