Page MenuHomePhabricator

Support parsing minidump files that are created by Breakpad.
ClosedPublic

Authored by clayborg on Jul 19 2018, 5:09 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

clayborg created this revision.Jul 19 2018, 5:09 PM
clayborg added a reviewer: markmentovai.
labath added a subscriber: lemo.Jul 20 2018, 5:46 AM

@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.

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.

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?

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 updated this revision to Diff 156544.Jul 20 2018, 10:56 AM

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 updated this revision to Diff 156545.Jul 20 2018, 10:57 AM

Remove xcode project changes.

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

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.

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 accepted this revision.Jul 23 2018, 2:31 AM

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

Looks good. Thanks.

This revision is now accepted and ready to land.Jul 23 2018, 2:31 AM
This revision was automatically updated to reflect the committed changes.