[llvm-branch-commits] [lldb] 85dfcaa - [LLDB] MinidumpParser: Prefer executable module even at higher address

[Joseph Tremoulet via llvm-branch-commits]

Author: Joseph Tremoulet
Date: 2021-01-14T13:17:57-05:00
New Revision: 85dfcaadc5f0920dc8ecbece6c786701b8f45ab4

URL: https://github.com/llvm/llvm-project/commit/85dfcaadc5f0920dc8ecbece6c786701b8f45ab4
DIFF: https://github.com/llvm/llvm-project/commit/85dfcaadc5f0920dc8ecbece6c786701b8f45ab4.diff

LOG: [LLDB] MinidumpParser: Prefer executable module even at higher address

When a program maps one of its own modules for reading, and then
crashes, breakpad can emit two entries for that module in the
ModuleList.  We have logic to identify this case by checking permissions
on mapped memory regions and report just the module with an executable
region.  As currently written, though, the check is asymmetric -- the
entry with the executable region must be the second one encountered for
the preference to kick in.

This change makes the logic symmetric, so that the first-encountered
module will similarly be preferred if it has an executable region but
the second-encountered module does not.  This happens for example when
the module in question is the executable itself, which breakpad likes to
report first -- we need to ignore the other entry for that module when
we see it later, even though it may be mapped at a lower virtual
address.

Reviewed By: clayborg

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

Added: 
    

Modified: 
    lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
    lldb/unittests/Process/minidump/MinidumpParserTest.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
index e16f86cca1c2..61106ebcc430 100644
--- a/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
+++ b/lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -391,19 +391,23 @@ std::vector<const minidump::Module *> MinidumpParser::GetFilteredModuleList() {
       filtered_modules.push_back(&module);
     } else {
       // We have a duplicate module entry. Check the linux regions to see if
-      // the module we already have is not really a mapped executable. If it
-      // isn't check to see if the current duplicate module entry is a real
-      // mapped executable, and if so, replace it. This can happen when a
-      // process mmap's in the file for an executable in order to read bytes
-      // from the executable file. A memory region mapping will exist for the
-      // mmap'ed version and for the loaded executable, but only one will have
-      // a consecutive region that is executable in the memory regions.
+      // either module is not really a mapped executable. If one but not the
+      // other is a real mapped executable, prefer the executable one. This
+      // can happen when a process mmap's in the file for an executable in
+      // order to read bytes from the executable file. A memory region mapping
+      // will exist for the mmap'ed version and for the loaded executable, but
+      // only one will have a consecutive region that is executable in the
+      // memory regions.
       auto dup_module = filtered_modules[iter->second];
       ConstString name(*ExpectedName);
-      if (!CheckForLinuxExecutable(name, linux_regions,
-                                   dup_module->BaseOfImage) &&
-          CheckForLinuxExecutable(name, linux_regions, module.BaseOfImage)) {
-        filtered_modules[iter->second] = &module;
+      bool is_executable =
+          CheckForLinuxExecutable(name, linux_regions, module.BaseOfImage);
+      bool dup_is_executable =
+          CheckForLinuxExecutable(name, linux_regions, dup_module->BaseOfImage);
+
+      if (is_executable != dup_is_executable) {
+        if (is_executable)
+          filtered_modules[iter->second] = &module;
         continue;
       }
       // This module has been seen. Modules are sometimes mentioned multiple

diff  --git a/lldb/unittests/Process/minidump/MinidumpParserTest.cpp b/lldb/unittests/Process/minidump/MinidumpParserTest.cpp
index 69046af283eb..e3f23c5fe33a 100644
--- a/lldb/unittests/Process/minidump/MinidumpParserTest.cpp
+++ b/lldb/unittests/Process/minidump/MinidumpParserTest.cpp
@@ -792,6 +792,47 @@ TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedSecond) {
   EXPECT_EQ(0x400d0000u, filtered_modules[0]->BaseOfImage);
 }
 
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedSecondHigh) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:            ModuleList
+    Modules:
+      - Base of Image:   0x400d3000
+        Size of Image:   0x00002000
+        Module Name:     '/usr/lib/libc.so'
+        CodeView Record: ''
+      - Base of Image:   0x400d0000
+        Size of Image:   0x00001000
+        Module Name:     '/usr/lib/libc.so'
+        CodeView Record: ''
+  - Type:            LinuxMaps
+    Text:             |
+      400d0000-400d2000 r--p 00000000 b3:04 227        /usr/lib/libc.so
+      400d2000-400d3000 rw-p 00000000 00:00 0
+      400d3000-400d4000 r-xp 00010000 b3:04 227        /usr/lib/libc.so
+      400d4000-400d5000 rwxp 00001000 b3:04 227        /usr/lib/libc.so
+...
+)"),
+                    llvm::Succeeded());
+  // If we have a module mentioned twice in the module list, and we have full
+  // linux maps for all of the memory regions, make sure we pick the one that
+  // has a consecutive region with a matching path that has executable
+  // permissions. If clients open an object file with mmap, breakpad can create
+  // multiple mappings for a library errnoneously and the lowest address isn't
+  // always the right address. In this case we check the consective memory
+  // regions whose path matches starting at the base of image address and make
+  // sure one of the regions is executable and prefer that one.
+  //
+  // This test will make sure that if the executable is first in the module
+  // list, that it will remain the correctly selected module in the filtered
+  // list, even if the non-executable module was loaded at a lower base address.
+  std::vector<const minidump::Module *> filtered_modules =
+      parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400d3000u, filtered_modules[0]->BaseOfImage);
+}
+
 TEST_F(MinidumpParserTest, MinidumpDuplicateModuleSeparateCode) {
   ASSERT_THAT_ERROR(SetUpFromYaml(R"(
 --- !minidump