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.
Details
Details
- Reviewers
labath zturner markmentovai - Commits
- rG2dd7e5e22251: Add support for parsing Breakpad minidump files that can have extra padding in…
rLLDB337694: Add support for parsing Breakpad minidump files that can have extra padding in…
rL337694: Add support for parsing Breakpad minidump files that can have extra padding in…
Diff Detail
Diff Detail
Event Timeline
Comment Actions
@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 | You'll also need to add these files to unittests/Process/minidump/CMakeLists.txt. Otherwise the tests won't work from cmake. | |
| 151–152 | 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. | |
Comment Actions
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.
Comment Actions
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?
Comment Actions
We have years of minidumps that already have this issue so it must work unfortunately.
Comment Actions
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
Comment Actions
BTW: The padding fix was inspired from the breakpad sources as they do this exact same thing.
Comment Actions
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.
Comment Actions
Thanks for the info. Pavel? This good to go? I have another patch that adds the ARM and ARM64 support that will quickly follow.
Comment Actions
The extra padding is unfortunate, but I guess we have to live with it now.
Looks good. Thanks.
You'll also need to add these files to unittests/Process/minidump/CMakeLists.txt. Otherwise the tests won't work from cmake.