Page MenuHomePhabricator

Fix UUID decoding from minidump files.
ClosedPublic

Authored by clayborg on Mar 15 2019, 2:27 PM.

Details

Summary

This patch fixes:

  • UUIDs now don't include the age field from a PDB70 when the age is zero. Prior to this they would incorrectly contain the zero age which stopped us from being able to match up the UUID with real files.
  • UUIDs for Apple targets get the first 32 bit value and next two 16 bit values swapped. Breakpad incorrectly swaps these values when it creates darwin minidump files, so this must be undone so we can match up symbol files with the minidump modules.
  • UUIDs that are all zeroes are treated as invalid UUIDs. Breakpad will always save out a UUID, even if one wasn't available. This caused all files that have UUID values of zero to be uniqued to the first module that had a zero UUID. We now don't fill in the UUID if it is all zeroes.

Added tests for PDB70 and ELF build ID based CvRecords.

Diff Detail

Repository
rL LLVM

Event Timeline

clayborg created this revision.Mar 15 2019, 2:27 PM

"MinidumpNew" is a little bit vague. What's "new" about it? Is there a way we could come up with a better name?

As an aside, since there's no interactivity in these tests, they seem like a good candidate for lit/Minidump, with 1 file for each test. Something like:

; zero-uuid-modules.test
; RUN: lldb -core %S/Inputs/linux-arm-zero-uuids.dmp -S %p | FileCheck %s

target modules list

; CHECK: [  0] 00000000-0000-0000-0000-000000000000-00000000                    /tmp/a
; CHECK: [  1] 00000000-0000-0000-0000-000000000000-00000000                    /tmp/b

Let's see what Pavel thinks though.

"MinidumpNew" is a little bit vague. What's "new" about it? Is there a way we could come up with a better name?

This was an existing file that I just added new tests to, and it seemed to be where most of the minidump tests were.

As an aside, since there's no interactivity in these tests, they seem like a good candidate for lit/Minidump, with 1 file for each test. Something like:

; zero-uuid-modules.test
; RUN: lldb -core %S/Inputs/linux-arm-zero-uuids.dmp -S %p | FileCheck %s

target modules list

; CHECK: [  0] 00000000-0000-0000-0000-000000000000-00000000                    /tmp/a
; CHECK: [  1] 00000000-0000-0000-0000-000000000000-00000000                    /tmp/b

Let's see what Pavel thinks though.

I wish the lit tests worked with the Xcode build

labath accepted this revision.Mar 18 2019, 1:37 AM

Looks good to me. I have some comments inline and below, but none of them is really substantial.

"MinidumpNew" is a little bit vague. What's "new" about it? Is there a way we could come up with a better name?

This was an existing file that I just added new tests to, and it seemed to be where most of the minidump tests were.

The name is a relict from when we were implementing our own minidump reader. In that context, the existing implementation which relied on the windows APIs was the "old" thing, and the one which read the files directly was "new". But this distinction doesn't really make sense anymore, so maybe we could start getting rid of it by putting the new tests into a fresh file (TestMinidumpUUIDs.py ?).

As an aside, since there's no interactivity in these tests, they seem like a good candidate for lit/Minidump, with 1 file for each test. Something like:

; zero-uuid-modules.test
; RUN: lldb -core %S/Inputs/linux-arm-zero-uuids.dmp -S %p | FileCheck %s

target modules list

; CHECK: [  0] 00000000-0000-0000-0000-000000000000-00000000                    /tmp/a
; CHECK: [  1] 00000000-0000-0000-0000-000000000000-00000000                    /tmp/b

Let's see what Pavel thinks though.

I think turning these into lit tests would be reasonable, but I also don't have a problem with them staying like they are.

packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
533 ↗(On Diff #190901)

Am I right in thinking that if I have an object file called /tmp/a on my system, then lldb will pick it up (and it's build-id), causing this test to fail ? If that's the case then it would be better to use some path which is less likely to exist.

source/Plugins/Process/minidump/MinidumpParser.cpp
175–183 ↗(On Diff #190901)

Instead of all of this, I believe you should be able to just replace the UUID::fromData with UUID::fromOptionalData in the calls below.

This revision is now accepted and ready to land.Mar 18 2019, 1:37 AM
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
533 ↗(On Diff #190901)

I second this. https://docs.python.org/3/library/tempfile.html provides ways to create such temporary names.

clayborg marked an inline comment as done.Wed, Mar 20, 8:09 AM
clayborg added inline comments.
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
533 ↗(On Diff #190901)

These have strict UUID values that must match and the value is hard coded into the minidump files. So even if there is such a file, the UUID won't match.

labath added inline comments.Wed, Mar 20, 8:17 AM
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
533 ↗(On Diff #190901)

That is true for other files, but is it true about this one? I specifically highlighted this instance one because it tests the uuid-not-present case (in which case I believe lldb will interpret that as matching any object file).

Closed by commit rL356573: Fix UUID decoding from minidump files (authored by gclayton, committed by ). · Explain WhyWed, Mar 20, 9:49 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Mar 20, 9:49 AM
shafik added a subscriber: shafik.EditedThu, Mar 21, 10:22 AM

It looks like this commit introduced undefined behavior via a misaligned load see this log from lldb sanitized bot on green dragon

http://lab.llvm.org:8080/green/view/LLDB/job/lldb-sanitized/2050/testReport/junit/lldb-Suite/functionalities_postmortem_minidump-new/TestMiniDumpUUID_py/

Summary:

/Users/buildslave/jenkins/workspace/lldb-sanitized/llvm/lib/Support/ConvertUTF.cpp:259:14: runtime error: load of misaligned address 0x61700018f671 for type 'const llvm::UTF16' (aka 'const unsigned short'), which requires 2 byte alignment
0x61700018f671: note: pointer points here

0c 00 00  00 2f 00 74 00 6d 00 70  00 2f 00 62 00 52 53 44  53 0a 14 1e 28 32 3c 46  50 5a 64 6e 78
         ^

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Users/buildslave/jenkins/workspace/lldb-sanitized/llvm/lib/Support/ConvertUTF.cpp:259:14 in

ormris removed a subscriber: ormris.Thu, Mar 21, 10:25 AM