Fix UUID decoding from minidump files.

[clayborg]

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.

[zturner]

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

[clayborg]

In D59433#1431488, @zturner wrote:

"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]

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

In D59433#1431602, @clayborg wrote:

In D59433#1431488, @zturner wrote:

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

[serge-sans-paille]

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]

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]

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

[shafik]

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

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py

+"""
+Test basics of Minidump debugging.
+"""
+
+from __future__ import print_function
+from six import iteritems
+
+import shutil
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class MiniDumpUUIDTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def setUp(self):
+        super(MiniDumpUUIDTestCase, self).setUp()
+        self._initial_platform = lldb.DBG.GetSelectedPlatform()
+
+    def tearDown(self):
+        lldb.DBG.SetSelectedPlatform(self._initial_platform)
+        super(MiniDumpUUIDTestCase, self).tearDown()
+
+    def verify_module(self, module, verify_path, verify_uuid):
+        uuid = module.GetUUIDString()
+        self.assertEqual(verify_path, module.GetFileSpec().fullpath)
+        self.assertEqual(verify_uuid, uuid)
+
+    def test_zero_uuid_modules(self):
+        """
+            Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
+            but contains a PDB70 value whose age is zero and whose UUID values are 
+            all zero. Prior to a fix all such modules would be duplicated to the
+            first one since the UUIDs claimed to be valid and all zeroes. Now we
+            ensure that the UUID is not valid for each module and that we have
+            each of the modules in the target after loading the core
+        """
+        self.dbg.CreateTarget(None)
+        self.target = self.dbg.GetSelectedTarget()
+        self.process = self.target.LoadCore("linux-arm-zero-uuids.dmp")
+        modules = self.target.modules
+        self.assertEqual(2, len(modules))
+        self.verify_module(modules[0], "/file/does/not/exist/a", None)
+        self.verify_module(modules[1], "/file/does/not/exist/b", None)
+
+    def test_uuid_modules_no_age(self):
+        """
+            Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
+            and contains a PDB70 value whose age is zero and whose UUID values are 
+            valid. Ensure we decode the UUID and don't include the age field in the UUID.
+        """
+        self.dbg.CreateTarget(None)
+        self.target = self.dbg.GetSelectedTarget()
+        self.process = self.target.LoadCore("linux-arm-uuids-no-age.dmp")
+        modules = self.target.modules
+        self.assertEqual(2, len(modules))
+        self.verify_module(modules[0], "/tmp/a", "01020304-0506-0708-090A-0B0C0D0E0F10")
+        self.verify_module(modules[1], "/tmp/b", "0A141E28-323C-4650-5A64-6E78828C96A0")
+
+    def test_uuid_modules_no_age_apple(self):
+        """
+            Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
+            and contains a PDB70 value whose age is zero and whose UUID values are 
+            valid. Ensure we decode the UUID and don't include the age field in the UUID.
+            Also ensure that the first uint32_t is byte swapped, along with the next
+            two uint16_t values. Breakpad incorrectly byte swaps these values when it
+            saves Darwin minidump files.
+        """
+        self.dbg.CreateTarget(None)
+        self.target = self.dbg.GetSelectedTarget()
+        self.process = self.target.LoadCore("macos-arm-uuids-no-age.dmp")
+        modules = self.target.modules
+        self.assertEqual(2, len(modules))
+        self.verify_module(modules[0], "/tmp/a", "04030201-0605-0807-090A-0B0C0D0E0F10")
+        self.verify_module(modules[1], "/tmp/b", "281E140A-3C32-5046-5A64-6E78828C96A0")
+
+    def test_uuid_modules_with_age(self):
+        """
+            Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
+            and contains a PDB70 value whose age is valid and whose UUID values are 
+            valid. Ensure we decode the UUID and include the age field in the UUID.
+        """
+        self.dbg.CreateTarget(None)
+        self.target = self.dbg.GetSelectedTarget()
+        self.process = self.target.LoadCore("linux-arm-uuids-with-age.dmp")
+        modules = self.target.modules
+        self.assertEqual(2, len(modules))
+        self.verify_module(modules[0], "/tmp/a", "01020304-0506-0708-090A-0B0C0D0E0F10-10101010")
+        self.verify_module(modules[1], "/tmp/b", "0A141E28-323C-4650-5A64-6E78828C96A0-20202020")
+
+    def test_uuid_modules_elf_build_id_16(self):
+        """
+            Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
+            and contains a ELF build ID whose value is valid and is 16 bytes long.
+        """
+        self.dbg.CreateTarget(None)
+        self.target = self.dbg.GetSelectedTarget()
+        self.process = self.target.LoadCore("linux-arm-uuids-elf-build-id-16.dmp")
+        modules = self.target.modules
+        self.assertEqual(2, len(modules))
+        self.verify_module(modules[0], "/tmp/a", "01020304-0506-0708-090A-0B0C0D0E0F10")
+        self.verify_module(modules[1], "/tmp/b", "0A141E28-323C-4650-5A64-6E78828C96A0")
+
+    def test_uuid_modules_elf_build_id_20(self):
+        """
+            Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
+            and contains a ELF build ID whose value is valid and is 20 bytes long.
+        """
+        self.dbg.CreateTarget(None)
+        self.target = self.dbg.GetSelectedTarget()
+        self.process = self.target.LoadCore("linux-arm-uuids-elf-build-id-20.dmp")
+        modules = self.target.modules
+        self.assertEqual(2, len(modules))
+        self.verify_module(modules[0], "/tmp/a", "01020304-0506-0708-090A-0B0C0D0E0F10-11121314")
+        self.verify_module(modules[1], "/tmp/b", "0A141E28-323C-4650-5A64-6E78828C96A0-AAB4BEC8")

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

UUID MinidumpParser::GetModuleUUID(const MinidumpModule *module) {
   Status error = consumeObject(cv_record, signature);
   if (error.Fail())
     return UUID();
 
   const CvSignature cv_signature =
       static_cast<CvSignature>(static_cast<uint32_t>(*signature));
 
   if (cv_signature == CvSignature::Pdb70) {
-    // PDB70 record
     const CvRecordPdb70 *pdb70_uuid = nullptr;
     Status error = consumeObject(cv_record, pdb70_uuid);
-    if (!error.Fail()) {
-      auto arch = GetArchitecture();
-      // For Apple targets we only need a 16 byte UUID so that we can match
-      // the UUID in the Module to actual UUIDs from the built binaries. The
-      // "Age" field is zero in breakpad minidump files for Apple targets, so
-      // we restrict the UUID to the "Uuid" field so we have a UUID we can use
-      // to match.
-      if (arch.GetTriple().getVendor() == llvm::Triple::Apple)
-        return UUID::fromData(pdb70_uuid->Uuid, sizeof(pdb70_uuid->Uuid));
-      else
-        return UUID::fromData(pdb70_uuid, sizeof(*pdb70_uuid));
+    if (error.Fail())
+      return UUID();
+    // If the age field is not zero, then include the entire pdb70_uuid struct
+    if (pdb70_uuid->Age != 0)
+
+    // Many times UUIDs are all zeroes. This can cause more than one module
+    // to claim it has a valid UUID of all zeroes and causes the files to all
+    // merge into the first module that claims this valid zero UUID.
+    bool all_zeroes = true;
+    for (size_t i = 0; all_zeroes && i < sizeof(pdb70_uuid->Uuid); ++i)
+      all_zeroes = pdb70_uuid->Uuid[i] == 0;
+    if (all_zeroes)
+      return UUID();
+    
+    if (GetArchitecture().GetTriple().getVendor() == llvm::Triple::Apple) {
+      // Breakpad incorrectly byte swaps the first 32 bit and next 2 16 bit
+      // values in the UUID field. Undo this so we can match things up
+      // with our symbol files
+      uint8_t apple_uuid[16];
+      // Byte swap the first 32 bits
+      apple_uuid[0] = pdb70_uuid->Uuid[3];
+      apple_uuid[1] = pdb70_uuid->Uuid[2];
+      apple_uuid[2] = pdb70_uuid->Uuid[1];
+      apple_uuid[3] = pdb70_uuid->Uuid[0];
+      // Byte swap the next 16 bit value
+      apple_uuid[4] = pdb70_uuid->Uuid[5];
+      apple_uuid[5] = pdb70_uuid->Uuid[4];
+      // Byte swap the next 16 bit value
+      apple_uuid[6] = pdb70_uuid->Uuid[7];
+      apple_uuid[7] = pdb70_uuid->Uuid[6];
+      for (size_t i = 8; i < sizeof(pdb70_uuid->Uuid); ++i)
+        apple_uuid[i] = pdb70_uuid->Uuid[i];
+      return UUID::fromData(apple_uuid, sizeof(apple_uuid));
     }
+    return UUID::fromData(pdb70_uuid->Uuid, sizeof(pdb70_uuid->Uuid));
   } else if (cv_signature == CvSignature::ElfBuildId)
     return UUID::fromData(cv_record);
 
   return UUID();
 }
 
 llvm::ArrayRef<MinidumpThread> MinidumpParser::GetThreads() {
   llvm::ArrayRef<uint8_t> data = GetStream(MinidumpStreamType::ThreadList);