Page MenuHomePhabricator

Add hashing of the .text section to ProcessMinidump.
ClosedPublic

Authored by clayborg on Aug 19 2020, 8:56 PM.

Details

Summary

Breakpad will always have a UUID for binaries when it creates minidump files. If an ELF files has a GNU build ID, it will use that. If it doesn't, it will create one by hashing up to the first 4096 bytes of the .text section. LLDB was not able to load these binaries even when we had the right binary because the UUID didn't match. LLDB will use the GNU build ID first as the main UUID for a binary and fallback onto a 8 byte CRC if a binary doesn't have one. With this fix, we will check for the Breakpad hash or the Facebook hash (a modified version of the breakpad hash that collides a bit less) and accept binaries when these hashes match.

Diff Detail

Event Timeline

clayborg created this revision.Aug 19 2020, 8:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2020, 8:56 PM
clayborg requested review of this revision.Aug 19 2020, 8:56 PM
jhenderson resigned from this revision.Aug 20 2020, 12:04 AM

I'm not an LLDB developer, so probably not the right person to review this, sorry.

wallace accepted this revision.Aug 20 2020, 12:10 AM
wallace added inline comments.
lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
563–564

I don't understand well the first sentence

This revision is now accepted and ready to land.Aug 20 2020, 12:10 AM

This has been a feature we've been missing for a while. Thanks for implementing it.

Just two quick requests on the implementation.

lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
171–174

Running off of the end of data buffer in this way seems dangerous. I get that we actually want to replicate running off of the end of the section, but we should do it in a way that won't light up on any sanitizer. It seems like it should be possible to fetch the desired data via something like this:

size_t size_to_read = std::min(llvm::alignTo(sect_sp->GetFileSize(), kMDGUIDSize), kBreakpadPageSize) /*or something similar*/;
sect_sp->GetObjectFile()->GetData(sect_sp->GetFileOffset(), size_to_read, data);
lldb/test/API/functionalities/postmortem/minidump-new/libbreakpad.yaml
16

I guess this should include a custom Fill pseudo-section so that we can guarantee the contents of whatever comes after it. Otherwise, yaml2obj might decide to place anything (or nothing) there. Something like this ought to do it:

- Type: Fill
  Pattern: "DEADBEEF"
  Size: 0xsomething

Also, +@markmentovai, in case he has any thoughts on this.

clayborg added inline comments.Aug 20 2020, 2:10 PM
lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
174

I will try and make sure the data is there. The main issue is I don't think you can ask for more bytes than a section has, I believe it will cap the data. But I will check into this.

563–564

I will try to make this clearer and rephrase a bit.

lldb/test/API/functionalities/postmortem/minidump-new/libbreakpad.yaml
16

We don't need to because I selected a multiple of 16 for the contents of the text section! If I added one more byte, then we would need to.

clayborg added inline comments.Aug 20 2020, 2:12 PM
lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
174

Never mind, you are reading from the object file's data directly (not the section contents) so this should work just fine.

clayborg updated this revision to Diff 286901.Aug 20 2020, 2:28 PM

Fixed:

  • Use a safer method to read the data we need for the .text section in case we need and extra 15 bytes.
  • Rephrase confusing comment
clayborg marked 2 inline comments as done.Aug 20 2020, 2:29 PM

Hopefully this should be good to go, let me know if anyone has any issues.

labath accepted this revision.Aug 21 2020, 1:43 AM

Hopefully this should be good to go, let me know if anyone has any issues.

Looks good, just please also add a test case which runs off the end of the text section. If you have problems generating this test with yaml2obj, I would also be fine with a checked-in binary for a special case like this.

lldb/test/API/functionalities/postmortem/minidump-new/libbreakpad.yaml
16

I see. In that case, it would be good to have an test case which actually exercises the overflow path. :)

clayborg updated this revision to Diff 287078.Aug 21 2020, 12:05 PM

Added a test case with a 1 byte .text section that will overflow into the 15 bytes of the .data section that follows. This will test the overflow case that was requested.

clayborg marked an inline comment as done.Aug 21 2020, 12:05 PM
labath accepted this revision.Aug 24 2020, 12:35 AM

thanks

This revision was landed with ongoing or failed builds.Aug 24 2020, 11:44 AM
This revision was automatically updated to reflect the committed changes.