Support parsing minidump files that are created by Breakpad.

[clayborg]

Summary

Breakpad files sometimes extra 4 bytes of padding after the 32 bit count for memory, module and thread lists. I also created bare minimum minidump files that contain both padded and not padded files that test each functionality works correctly.

[labath]

@markmentovai, @lemo, do you know under which circumstances do these extra 4 bytes get emitted? Is there any chance we could document this better than just saying "sometimes"?

unittests/Process/minidump/MinidumpParserTest.cpp

90 ↗

(On Diff #156398)

You'll also need to add these files to unittests/Process/minidump/CMakeLists.txt. Otherwise the tests won't work from cmake.

151–152 ↗

(On Diff #156398)

You can avoid the if statement by changing the previous check to ASSERT_TRUE. ASSERT_*** macros automatically terminate the test if they fail so you won't get a crash there.

[clayborg]

The "sometimes" depends on how compilers pad the structures that define the lists. Since the structs look like:

struct MemoryList {
  uint32_t count;
  MemoryDescriptor descriptors[];
};

The compiler might end up padding and extra 4 bytes depending on what is in the struct that follows the count. I can clarify this in my comments.

[markmentovai]

This has got to be padding.

Breakpad’s structure definitions don’t nail down alignment as they probably should. A 32-bit system writing an MDRawMemoryList will write what’s intended, but a 64-bit one will need to insert four bytes of padding after number_of_memory_ranges to make sure that the array elements that follow are aligned.

Oops.

Microsoft’s own structure definitions don’t have this problem, and neither do Crashpad’s (example).

I think it’d be preferable to fix Breakpad to not write bogus minidumps. It’s absolutely wrong, and other code will have interoperability problems too.

Greg, if we fix Breakpad, will you still need this for old dumps, or are you OK wiping the slate clean?

[clayborg]

In D49579#1170099, @markmentovai wrote:

This has got to be padding.

Breakpad’s structure definitions don’t nail down alignment as they probably should. A 32-bit system writing an MDRawMemoryList will write what’s intended, but a 64-bit one will need to insert four bytes of padding after number_of_memory_ranges to make sure that the array elements that follow are aligned.

Oops.

Microsoft’s own structure definitions don’t have this problem, and neither do Crashpad’s (example).

I think it’d be preferable to fix Breakpad to not write bogus minidumps. It’s absolutely wrong, and other code will have interoperability problems too.

Greg, if we fix Breakpad, will you still need this for old dumps, or are you OK wiping the slate clean?

We have years of minidumps that already have this issue so it must work unfortunately.

[clayborg]

Improve comments to indicate why padding might be there, fix a test case to use the right file, and add all new .dmp files to the CMakeLists.txt

[clayborg]

Remove xcode project changes.

[clayborg]

BTW: The padding fix was inspired from the breakpad sources as they do this exact same thing.

[markmentovai]

I know, that was a mistake.

(Unfortunately, I reviewed it. 11 years ago. And now I feel responsible for all of those malformed minidumps floating around out there.)

If you do need to do this, it seems fine to me, since it’s basically the same exact workaround. If we fix the Breakpad writer up to write minidumps correctly (as Crashpad and dbghelp do), the workaround should go dormant, so it’s no problem from my perspective.

[clayborg]

Thanks for the info. Pavel? This good to go? I have another patch that adds the ARM and ARM64 support that will quickly follow.

[labath]

The extra padding is unfortunate, but I guess we have to live with it now.

Looks good. Thanks.

lldb/trunk/source/Plugins/Process/minidump/MinidumpTypes.cpp

const MinidumpThread *MinidumpThread::Parse(llvm::ArrayRef<uint8_t> &data) {
   if (error.Fail())
     return nullptr;
 
   return thread;
 }
 
 llvm::ArrayRef<MinidumpThread>
 MinidumpThread::ParseThreadList(llvm::ArrayRef<uint8_t> &data) {
+  const auto orig_size = data.size();
   const llvm::support::ulittle32_t *thread_count;
   Status error = consumeObject(data, thread_count);
   if (error.Fail() || *thread_count * sizeof(MinidumpThread) > data.size())
     return {};
 
+  // Compilers might end up padding an extra 4 bytes depending on how the
+  // structure is padded by the compiler and the #pragma pack settings.
+  if (4 + *thread_count * sizeof(MinidumpThread) < orig_size)
+    data = data.drop_front(4);
+
   return llvm::ArrayRef<MinidumpThread>(
       reinterpret_cast<const MinidumpThread *>(data.data()), *thread_count);
 }
 
 // MinidumpSystemInfo
 const MinidumpSystemInfo *
 MinidumpSystemInfo::Parse(llvm::ArrayRef<uint8_t> &data) {
   const MinidumpSystemInfo *system_info;
const MinidumpModule *MinidumpModule::Parse(llvm::ArrayRef<uint8_t> &data) {
   if (error.Fail())
     return nullptr;
 
   return module;
 }
 
 llvm::ArrayRef<MinidumpModule>
 MinidumpModule::ParseModuleList(llvm::ArrayRef<uint8_t> &data) {
-
+  const auto orig_size = data.size();
   const llvm::support::ulittle32_t *modules_count;
   Status error = consumeObject(data, modules_count);
   if (error.Fail() || *modules_count * sizeof(MinidumpModule) > data.size())
     return {};
   
+  // Compilers might end up padding an extra 4 bytes depending on how the
+  // structure is padded by the compiler and the #pragma pack settings.
+  if (4 + *modules_count * sizeof(MinidumpModule) < orig_size)
+    data = data.drop_front(4);
+  
   return llvm::ArrayRef<MinidumpModule>(
       reinterpret_cast<const MinidumpModule *>(data.data()), *modules_count);
 }
 
 // Exception stuff
 const MinidumpExceptionStream *
 MinidumpExceptionStream::Parse(llvm::ArrayRef<uint8_t> &data) {
   const MinidumpExceptionStream *exception_stream = nullptr;
   Status error = consumeObject(data, exception_stream);
   if (error.Fail())
     return nullptr;
 
   return exception_stream;
 }
 
 llvm::ArrayRef<MinidumpMemoryDescriptor>
 MinidumpMemoryDescriptor::ParseMemoryList(llvm::ArrayRef<uint8_t> &data) {
+  const auto orig_size = data.size();
   const llvm::support::ulittle32_t *mem_ranges_count;
   Status error = consumeObject(data, mem_ranges_count);
   if (error.Fail() ||
       *mem_ranges_count * sizeof(MinidumpMemoryDescriptor) > data.size())
     return {};
   
+  // Compilers might end up padding an extra 4 bytes depending on how the
+  // structure is padded by the compiler and the #pragma pack settings.
+  if (4 + *mem_ranges_count * sizeof(MinidumpMemoryDescriptor) < orig_size)
+    data = data.drop_front(4);
+
   return llvm::makeArrayRef(
       reinterpret_cast<const MinidumpMemoryDescriptor *>(data.data()),
       *mem_ranges_count);
 }
 
 std::pair<llvm::ArrayRef<MinidumpMemoryDescriptor64>, uint64_t>
 MinidumpMemoryDescriptor64::ParseMemory64List(llvm::ArrayRef<uint8_t> &data) {
   const llvm::support::ulittle64_t *mem_ranges_count;

lldb/trunk/unittests/Process/minidump/CMakeLists.txt

 
 set(test_inputs
    linux-i386.dmp
    linux-x86_64.dmp
    linux-x86_64_not_crashed.dmp
    fizzbuzz_no_heap.dmp
    fizzbuzz_wow64.dmp
    bad_duplicate_streams.dmp
-   bad_overlapping_streams.dmp)
+   bad_overlapping_streams.dmp
+   thread-list-padded.dmp
+   thread-list-not-padded.dmp
+   module-list-padded.dmp
+   module-list-not-padded.dmp
+   memory-list-padded.dmp
+   memory-list-not-padded.dmp)
 
 add_unittest_inputs(LLDBMinidumpTests "${test_inputs}")

lldb/trunk/unittests/Process/minidump/MinidumpParserTest.cpp

TEST_F(MinidumpParserTest, GetThreadsAndGetThreadContext) {
   const MinidumpThread thread = thread_list[0];
 
   EXPECT_EQ(16001UL, thread.thread_id);
 
   llvm::ArrayRef<uint8_t> context = parser->GetThreadContext(thread);
   EXPECT_EQ(1232UL, context.size());
 }
 
+TEST_F(MinidumpParserTest, GetThreadListNotPadded) {
+  // Verify that we can load a thread list that doesn't have 4 bytes of padding
+  // after the thread count.
+  SetUpData("thread-list-not-padded.dmp");
+  llvm::ArrayRef<MinidumpThread> thread_list;
+  
+  thread_list = parser->GetThreads();
+  ASSERT_EQ(2UL, thread_list.size());
+  EXPECT_EQ(0x11223344UL, thread_list[0].thread_id);
+  EXPECT_EQ(0x55667788UL, thread_list[1].thread_id);
+}
+
+TEST_F(MinidumpParserTest, GetThreadListPadded) {
+  // Verify that we can load a thread list that has 4 bytes of padding
+  // after the thread count as found in breakpad minidump files.
+  SetUpData("thread-list-padded.dmp");
+  auto thread_list = parser->GetThreads();
+  ASSERT_EQ(2UL, thread_list.size());
+  EXPECT_EQ(0x11223344UL, thread_list[0].thread_id);
+  EXPECT_EQ(0x55667788UL, thread_list[1].thread_id);
+}
+
+TEST_F(MinidumpParserTest, GetModuleListNotPadded) {
+  // Verify that we can load a module list that doesn't have 4 bytes of padding
+  // after the module count.
+  SetUpData("module-list-not-padded.dmp");
+  auto module_list = parser->GetModuleList();
+  ASSERT_EQ(2UL, module_list.size());
+  EXPECT_EQ(0x1000UL, module_list[0].base_of_image);
+  EXPECT_EQ(0x2000UL, module_list[0].size_of_image);
+  EXPECT_EQ(0x5000UL, module_list[1].base_of_image);
+  EXPECT_EQ(0x3000UL, module_list[1].size_of_image);
+}
+
+TEST_F(MinidumpParserTest, GetModuleListPadded) {
+  // Verify that we can load a module list that has 4 bytes of padding
+  // after the module count as found in breakpad minidump files.
+  SetUpData("module-list-padded.dmp");
+  auto module_list = parser->GetModuleList();
+  ASSERT_EQ(2UL, module_list.size());
+  EXPECT_EQ(0x1000UL, module_list[0].base_of_image);
+  EXPECT_EQ(0x2000UL, module_list[0].size_of_image);
+  EXPECT_EQ(0x5000UL, module_list[1].base_of_image);
+  EXPECT_EQ(0x3000UL, module_list[1].size_of_image);
+}
+
+TEST_F(MinidumpParserTest, GetMemoryListNotPadded) {
+  // Verify that we can load a memory list that doesn't have 4 bytes of padding
+  // after the memory range count.
+  SetUpData("memory-list-not-padded.dmp");
+  auto mem = parser->FindMemoryRange(0x8000);
+  ASSERT_TRUE(mem.hasValue());
+  EXPECT_EQ((lldb::addr_t)0x8000, mem->start);
+  mem = parser->FindMemoryRange(0x8010);
+  ASSERT_TRUE(mem.hasValue());
+  EXPECT_EQ((lldb::addr_t)0x8010, mem->start);
+}
+
+TEST_F(MinidumpParserTest, GetMemoryListPadded) {
+  // Verify that we can load a memory list that has 4 bytes of padding
+  // after the memory range count as found in breakpad minidump files.
+  SetUpData("memory-list-padded.dmp");
+  auto mem = parser->FindMemoryRange(0x8000);
+  ASSERT_TRUE(mem.hasValue());
+  EXPECT_EQ((lldb::addr_t)0x8000, mem->start);
+  mem = parser->FindMemoryRange(0x8010);
+  ASSERT_TRUE(mem.hasValue());
+  EXPECT_EQ((lldb::addr_t)0x8010, mem->start);
+}
+
 TEST_F(MinidumpParserTest, TruncatedMinidumps) {
   InvalidMinidump("linux-x86_64.dmp", 32);
   InvalidMinidump("linux-x86_64.dmp", 100);
   InvalidMinidump("linux-x86_64.dmp", 20 * 1024);
 }
 
 TEST_F(MinidumpParserTest, IllFormedMinidumps) {
   InvalidMinidump("bad_duplicate_streams.dmp", -1);