Fix a crasher due to an assert when two files have the same UUID but different paths.

Summary: The PlaceholderObjectFile has an assert in SetLoadAddress that fires if "m_base == value" is not true. To avoid this, we create check that the base address matches, and if it doesn't we clear the module that was found using the UUID so that we create a new PlaceholderObjectFile. Added a test to cover this issue.

Reviewers: labath, aadsm, dvlahovski

Subscribers: lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D68106

llvm-svn: 374242

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

@@ -42,7 +42,7 @@ class MiniDumpUUIDTestCase(TestBase):
     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 
+            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
@@ -56,7 +56,7 @@ class MiniDumpUUIDTestCase(TestBase):
     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 
+            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.
         """
         modules = self.get_minidump_modules("linux-arm-uuids-no-age.yaml")
@@ -68,7 +68,7 @@ class MiniDumpUUIDTestCase(TestBase):
     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 
+            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
@@ -83,7 +83,7 @@ class MiniDumpUUIDTestCase(TestBase):
     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 
+            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.
         """
         modules = self.get_minidump_modules("linux-arm-uuids-with-age.yaml")
@@ -121,13 +121,31 @@ class MiniDumpUUIDTestCase(TestBase):
         self.verify_module(modules[0], "/not/exist/a", None)
         self.verify_module(modules[1], "/not/exist/b", None)
 
+    def test_uuid_modules_elf_build_id_same(self):
+        """
+            Test multiple modules having a MINIDUMP_MODULE.CvRecord that is
+            valid, and contains a ELF build ID whose value is the same. There
+            is an assert in the PlaceholderObjectFile that was firing when we
+            encountered this which was crashing the process that was checking
+            if PlaceholderObjectFile.m_base was the same as the address this
+            fake module was being loaded at. We need to ensure we don't crash
+            in such cases and that we add both modules even though they have
+            the same UUID.
+        """
+        modules = self.get_minidump_modules("linux-arm-same-uuids.yaml")
+        self.assertEqual(2, len(modules))
+        self.verify_module(modules[0], "/file/does/not/exist/a",
+                           '11223344-1122-3344-1122-334411223344-11223344')
+        self.verify_module(modules[1], "/file/does/not/exist/b",
+                           '11223344-1122-3344-1122-334411223344-11223344')
+
     @expectedFailureAll(oslist=["windows"])
     def test_partial_uuid_match(self):
         """
             Breakpad has been known to create minidump files using CvRecord in each
             module whose signature is set to PDB70 where the UUID only contains the
-            first 16 bytes of a 20 byte ELF build ID. Code was added to 
+            first 16 bytes of a 20 byte ELF build ID. Code was added to
-            ProcessMinidump.cpp to deal with this and allows partial UUID matching. 
+            ProcessMinidump.cpp to deal with this and allows partial UUID matching.
 
             This test verifies that if we have a minidump with a 16 byte UUID, that
             we are able to associate a symbol file with a 20 byte UUID only if the
@@ -141,16 +159,16 @@ class MiniDumpUUIDTestCase(TestBase):
         self.dbg.HandleCommand(cmd)
         modules = self.get_minidump_modules("linux-arm-partial-uuids-match.yaml")
         self.assertEqual(1, len(modules))
-        self.verify_module(modules[0], so_path, 
+        self.verify_module(modules[0], so_path,
                            "7295E17C-6668-9E05-CBB5-DEE5003865D5-5267C116")
 
     def test_partial_uuid_mismatch(self):
         """
             Breakpad has been known to create minidump files using CvRecord in each
             module whose signature is set to PDB70 where the UUID only contains the
-            first 16 bytes of a 20 byte ELF build ID. Code was added to 
+            first 16 bytes of a 20 byte ELF build ID. Code was added to
-            ProcessMinidump.cpp to deal with this and allows partial UUID matching. 
+            ProcessMinidump.cpp to deal with this and allows partial UUID matching.
-            
+
             This test verifies that if we have a minidump with a 16 byte UUID, that
             we are not able to associate a symbol file with a 20 byte UUID only if
             any of the first 16 bytes do not match. In this case we will see the UUID
@@ -163,7 +181,7 @@ class MiniDumpUUIDTestCase(TestBase):
         modules = self.get_minidump_modules("linux-arm-partial-uuids-mismatch.yaml")
         self.assertEqual(1, len(modules))
         self.verify_module(modules[0],
-                           "/invalid/path/on/current/system/libuuidmismatch.so", 
+                           "/invalid/path/on/current/system/libuuidmismatch.so",
                            "7295E17C-6668-9E05-CBB5-DEE5003865D5")
 
     def test_relative_module_name(self):

lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-arm-same-uuids.yaml

@@ -0,0 +1,21 @@
+--- !minidump
+Streams:
+  - Type:            SystemInfo
+    Processor Arch:  AMD64
+    Platform ID:     Linux
+    CSD Version:     '15E216'
+    CPU:
+      Vendor ID:       GenuineIntel
+      Version Info:    0x00000000
+      Feature Info:    0x00000000
+  - Type:            ModuleList
+    Modules:
+      - Base of Image:   0x0000000000001000
+        Size of Image:   0x00001000
+        Module Name:     '/file/does/not/exist/a'
+        CodeView Record: '52534453112233441122334411223344112233441122334411'
+      - Base of Image:   0x0000000000003000
+        Size of Image:   0x00001000
+        Module Name:     '/file/does/not/exist/b'
+        CodeView Record: '52534453112233441122334411223344112233441122334411'
+...

lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp

@@ -49,8 +49,8 @@ namespace {
 class PlaceholderObjectFile : public ObjectFile {
 public:
   PlaceholderObjectFile(const lldb::ModuleSP &module_sp,
-                        const ModuleSpec &module_spec, lldb::offset_t base,
+                        const ModuleSpec &module_spec, lldb::addr_t base,
-                        lldb::offset_t size)
+                        lldb::addr_t size)
       : ObjectFile(module_sp, &module_spec.GetFileSpec(), /*file_offset*/ 0,
                    /*length*/ 0, /*data_sp*/ nullptr, /*data_offset*/ 0),
         m_arch(module_spec.GetArchitecture()), m_uuid(module_spec.GetUUID()),
@@ -58,7 +58,10 @@ public:
     m_symtab_up = std::make_unique<Symtab>(this);
   }
 
-  ConstString GetPluginName() override { return ConstString("placeholder"); }
+  static ConstString GetStaticPluginName() {
+    return ConstString("placeholder");
+  }
+  ConstString GetPluginName() override { return GetStaticPluginName(); }
   uint32_t GetPluginVersion() override { return 1; }
   bool ParseHeader() override { return true; }
   Type CalculateType() override { return eTypeUnknown; }
@@ -109,11 +112,12 @@ public:
               GetFileSpec(), m_base, m_base + m_size);
   }
 
+  lldb::addr_t GetBaseImageAddress() const { return m_base; }
 private:
   ArchSpec m_arch;
   UUID m_uuid;
-  lldb::offset_t m_base;
+  lldb::addr_t m_base;
-  lldb::offset_t m_size;
+  lldb::addr_t m_size;
 };
 } // namespace
 
@@ -351,14 +355,15 @@ void ProcessMinidump::ReadModuleList() {
   std::vector<const minidump::Module *> filtered_modules =
       m_minidump_parser->GetFilteredModuleList();
 
-  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES));
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
 
   for (auto module : filtered_modules) {
     std::string name = cantFail(m_minidump_parser->GetMinidumpFile().getString(
         module->ModuleNameRVA));
+    const uint64_t load_addr = module->BaseOfImage;
+    const uint64_t load_size = module->SizeOfImage;
     LLDB_LOG(log, "found module: name: {0} {1:x10}-{2:x10} size: {3}", name,
-             module->BaseOfImage, module->BaseOfImage + module->SizeOfImage,
+             load_addr, load_addr + load_size, load_size);
-             module->SizeOfImage);
 
     // check if the process is wow64 - a 32 bit windows process running on a
     // 64 bit windows
@@ -373,7 +378,7 @@ void ProcessMinidump::ReadModuleList() {
     Status error;
     // Try and find a module with a full UUID that matches. This function will
     // add the module to the target if it finds one.
-    lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec, 
+    lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec,
                                                      true /* notify */, &error);
     if (!module_sp) {
       // Try and find a module without specifying the UUID and only looking for
@@ -386,8 +391,8 @@ void ProcessMinidump::ReadModuleList() {
       ModuleSpec basename_module_spec(module_spec);
       basename_module_spec.GetUUID().Clear();
       basename_module_spec.GetFileSpec().GetDirectory().Clear();
-      module_sp = GetTarget().GetOrCreateModule(basename_module_spec, 
+      module_sp = GetTarget().GetOrCreateModule(basename_module_spec,
-                                                     true /* notify */, &error);
+                                                true /* notify */, &error);
       if (module_sp) {
         // We consider the module to be a match if the minidump UUID is a
         // prefix of the actual UUID, or if either of the UUIDs are empty.
@@ -401,6 +406,19 @@ void ProcessMinidump::ReadModuleList() {
         }
       }
     }
+    if (module_sp) {
+      // Watch out for place holder modules that have different paths, but the
+      // same UUID. If the base address is different, create a new module. If
+      // we don't then we will end up setting the load address of a different
+      // PlaceholderObjectFile and an assertion will fire.
+      auto *objfile = module_sp->GetObjectFile();
+      if (objfile && objfile->GetPluginName() ==
+          PlaceholderObjectFile::GetStaticPluginName()) {
+        if (((PlaceholderObjectFile *)objfile)->GetBaseImageAddress() !=
+            load_addr)
+          module_sp.reset();
+      }
+    }
     if (!module_sp) {
       // We failed to locate a matching local object file. Fortunately, the
       // minidump format encodes enough information about each module's memory
@@ -415,12 +433,12 @@ void ProcessMinidump::ReadModuleList() {
                name);
 
       module_sp = Module::CreateModuleFromObjectFile<PlaceholderObjectFile>(
-          module_spec, module->BaseOfImage, module->SizeOfImage);
+          module_spec, load_addr, load_size);
       GetTarget().GetImages().Append(module_sp, true /* notify */);
     }
 
     bool load_addr_changed = false;
-    module_sp->SetLoadAddress(GetTarget(), module->BaseOfImage, false,
+    module_sp->SetLoadAddress(GetTarget(), load_addr, false,
                               load_addr_changed);
   }
 }