diff --git a/minidump/minidump_memory_writer.cc b/minidump/minidump_memory_writer.cc index 0e5f0861..868c356e 100644 --- a/minidump/minidump_memory_writer.cc +++ b/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 empty(memory_snapshot_->Size(), 0xfe); + MemorySnapshotDelegateRead(empty.data(), empty.size()); + } + + return true; } const MINIDUMP_MEMORY_DESCRIPTOR* diff --git a/minidump/minidump_memory_writer_test.cc b/minidump/minidump_memory_writer_test.cc index acff9c29..60b7fa8a 100644 --- a/minidump/minidump_memory_writer_test.cc +++ b/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(); + + constexpr uint64_t kBaseAddress = 0xfedcba9876543210; + constexpr size_t kSize = 0x1000; + constexpr uint8_t kValue = 'm'; + + auto memory_writer = + std::make_unique(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) diff --git a/minidump/test/minidump_memory_writer_test_util.cc b/minidump/test/minidump_memory_writer_test_util.cc index 03a0e18a..f198abeb 100644 --- a/minidump/test/minidump_memory_writer_test_util.cc +++ b/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) { diff --git a/minidump/test/minidump_memory_writer_test_util.h b/minidump/test/minidump_memory_writer_test_util.h index db198909..0890cd0e 100644 --- a/minidump/test/minidump_memory_writer_test_util.h +++ b/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_; diff --git a/snapshot/test/test_memory_snapshot.cc b/snapshot/test/test_memory_snapshot.cc index a3e9ebd9..95aba406 100644 --- a/snapshot/test/test_memory_snapshot.cc +++ b/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_); } diff --git a/snapshot/test/test_memory_snapshot.h b/snapshot/test/test_memory_snapshot.h index ae17ee48..011e6c6f 100644 --- a/snapshot/test/test_memory_snapshot.h +++ b/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); };