Load correct module for linux and android when duplicates exist in minidump.

[clayborg]

Summary

Breakpad creates minidump files that can a module loaded multiple times. We found that when a process mmap's the object file for a library, this can confuse breakpad into creating multiple modules in the module list. This patch fixes the GetFilteredModules() to check the linux maps for permissions and use the one that has execute permissions. Typically when people mmap a file into memory they don't map it as executable. This helps people to correctly load minidump files for post mortem analysis.

[labath]

If I correctly understand what this is doing, then I don't think it's a good idea. The base of an (elf) shared library does not have to be mapped executable. These are the mappings I get for a trivial hello world program (no mmapping of libraries or anything) on my linux machine:

00400000-00401000 r--p 00000000 fd:01 2838574                            /tmp/l/a.out
00401000-00402000 r-xp 00001000 fd:01 2838574                            /tmp/l/a.out
00402000-00403000 r--p 00002000 fd:01 2838574                            /tmp/l/a.out
00403000-00404000 r--p 00002000 fd:01 2838574                            /tmp/l/a.out
00404000-00405000 rw-p 00003000 fd:01 2838574                            /tmp/l/a.out
020fb000-0211c000 rw-p 00000000 00:00 0                                  [heap]
7fe4f5d87000-7fe4f5da9000 r--p 00000000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5da9000-7fe4f5ef3000 r-xp 00022000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5ef3000-7fe4f5f3d000 r--p 0016c000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5f3d000-7fe4f5f41000 r--p 001b5000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5f41000-7fe4f5f43000 rw-p 001b9000 fd:01 2738932                    /lib64/libc-2.31.so
...

Here, the correct base of a.out is 0x00400000 and the libc base is 0x7fe4f5d87000. But this patch would make them be detected as 0x00401000 and 0x7fe4f5da9000, respectively.

This behavior is controlled by the -z (no)separate-code. My machine has separate-code as default, but that setting may not be universal, so you may need to pass this flag explicitly to reproduce this. For reference, these are the mappings I get when compiling a.out with -z noseparate-code (libc mappings remain unchanged, of course):

00400000-00401000 r-xp 00000000 fd:01 2838574                            /tmp/l/a.out
00401000-00402000 r--p 00000000 fd:01 2838574                            /tmp/l/a.out
00402000-00403000 rw-p 00001000 fd:01 2838574                            /tmp/l/a.out

It sounds like we need a better heuristic. How about "the number of consecutive mappings with the same name"? User mmapping code is likely going to map the library in a single chunk, but the dynamic linker will typically create multiple mappings (even a trivial executable can have five), so it seems like picking the longest sequence could work...

[clayborg]

In D86375#2233054, @labath wrote:

If I correctly understand what this is doing, then I don't think it's a good idea. The base of an (elf) shared library does not have to be mapped executable. These are the mappings I get for a trivial hello world program (no mmapping of libraries or anything) on my linux machine:

00400000-00401000 r--p 00000000 fd:01 2838574                            /tmp/l/a.out
00401000-00402000 r-xp 00001000 fd:01 2838574                            /tmp/l/a.out
00402000-00403000 r--p 00002000 fd:01 2838574                            /tmp/l/a.out
00403000-00404000 r--p 00002000 fd:01 2838574                            /tmp/l/a.out
00404000-00405000 rw-p 00003000 fd:01 2838574                            /tmp/l/a.out
020fb000-0211c000 rw-p 00000000 00:00 0                                  [heap]
7fe4f5d87000-7fe4f5da9000 r--p 00000000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5da9000-7fe4f5ef3000 r-xp 00022000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5ef3000-7fe4f5f3d000 r--p 0016c000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5f3d000-7fe4f5f41000 r--p 001b5000 fd:01 2738932                    /lib64/libc-2.31.so
7fe4f5f41000-7fe4f5f43000 rw-p 001b9000 fd:01 2738932                    /lib64/libc-2.31.so
...

Here, the correct base of a.out is 0x00400000 and the libc base is 0x7fe4f5d87000. But this patch would make them be detected as 0x00401000 and 0x7fe4f5da9000, respectively.

It actually will do the right thing here. We only prefer the executable mapping for multiple entries in the minidump modules list. The minidump will have only one load address per module and that address will always be the first mapping in the linux proc maps file.

So the only entry for "/tmp/l/a.out" in the minidump would be the one for 00400000, and the only mapping for /lib64/libc-2.31.so would be 7fe4f5d87000.

If we have multiple module entries _and_ if you have separate code, the old behavior will continue and it will prefer the mapping with the lowest address because neither of the binaries would have the base address marked as executable. If we have multiple entries for the exact same path, but with different addresses, we will prefer the one that _is_ marked executable correctly (which happens on Android for us all the time, can could easily happen on Linux with "-z no-separate-code".

This behavior is controlled by the -z (no)separate-code. My machine has separate-code as default, but that setting may not be universal, so you may need to pass this flag explicitly to reproduce this. For reference, these are the mappings I get when compiling a.out with -z noseparate-code (libc mappings remain unchanged, of course):

00400000-00401000 r-xp 00000000 fd:01 2838574                            /tmp/l/a.out
00401000-00402000 r--p 00000000 fd:01 2838574                            /tmp/l/a.out
00402000-00403000 rw-p 00001000 fd:01 2838574                            /tmp/l/a.out

It sounds like we need a better heuristic. How about "the number of consecutive mappings with the same name"? User mmapping code is likely going to map the library in a single chunk, but the dynamic linker will typically create multiple mappings (even a trivial executable can have five), so it seems like picking the longest sequence could work...

So this heuristic will work like it did before for "-z separate-code", and will fix multiple mappings if "-z no-separate-code" is used. So this is only a win in my book since we only check the start address for a shared library _and_ we only check for the execute part if we have multiple module entries for the same file.

[clayborg]

Added a better algorithm for detecting if an address from the module list for linux processes with valid /proc/<pid>/maps file is executable. We now search consective addresses that match the path to the module for any that are marked as executable. Added tests to test if the mmap'ed file comes first or second, and also a test for "-z separate-code" when the first mapping for an executable isn't executable, but a subsequent mapping with a matching path is.

[labath]

Thanks for the clarification. The new version looks great.

lldb/source/Plugins/Process/minidump/MinidumpParser.h

public:
   /// Returns a list of memory regions and a flag indicating whether the list is
   /// complete (includes all regions mapped into the process memory).
   std::pair<MemoryRegionInfos, bool> BuildMemoryRegions();
 
   static llvm::StringRef GetStreamTypeAsString(StreamType stream_type);
 
   llvm::object::MinidumpFile &GetMinidumpFile() { return *m_file; }
 
+  static MemoryRegionInfo GetMemoryRegionInfo(const MemoryRegionInfos &regions,
+                                              lldb::addr_t load_addr);
+
 private:
   MinidumpParser(lldb::DataBufferSP data_sp,
                  std::unique_ptr<llvm::object::MinidumpFile> file);
 
   lldb::DataBufferSP m_data_sp;
   std::unique_ptr<llvm::object::MinidumpFile> m_file;
   ArchSpec m_arch;
 };
 
 } // end namespace minidump
 } // end namespace lldb_private
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_MINIDUMP_MINIDUMPPARSER_H

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

if (ExpectedModules)
     return *ExpectedModules;
 
   LLDB_LOG_ERROR(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_MODULES),
                  ExpectedModules.takeError(),
                  "Failed to read module list: {0}");
   return {};
 }
 
+static bool
+CreateRegionsCacheFromLinuxMaps(MinidumpParser &parser,
+                                std::vector<MemoryRegionInfo> &regions) {
+  auto data = parser.GetStream(StreamType::LinuxMaps);
+  if (data.empty())
+    return false;
+  ParseLinuxMapRegions(llvm::toStringRef(data),
+                       [&](const lldb_private::MemoryRegionInfo &region,
+                           const lldb_private::Status &status) -> bool {
+                         if (status.Success())
+                           regions.push_back(region);
+                         return true;
+                       });
+  return !regions.empty();
+}
+
+/// Check for the memory regions starting at \a load_addr for a contiguous
+/// section that has execute permissions that matches the module path.
+///
+/// When we load a breakpad generated minidump file, we might have the
+/// /proc/<pid>/maps text for a process that details the memory map of the
+/// process that the minidump is describing. This checks the sorted memory
+/// regions for a section that has execute permissions. A sample maps files
+/// might look like:
+///
+/// 00400000-00401000 r--p 00000000 fd:01 2838574           /tmp/a.out
+/// 00401000-00402000 r-xp 00001000 fd:01 2838574           /tmp/a.out
+/// 00402000-00403000 r--p 00002000 fd:01 2838574           /tmp/a.out
+/// 00403000-00404000 r--p 00002000 fd:01 2838574           /tmp/a.out
+/// 00404000-00405000 rw-p 00003000 fd:01 2838574           /tmp/a.out
+/// ...
+///
+/// This function should return true when given 0x00400000 and "/tmp/a.out"
+/// is passed in as the path since it has a consecutive memory region for
+/// "/tmp/a.out" that has execute permissions at 0x00401000. This will help us
+/// differentiate if a file has been memory mapped into a process for reading
+/// and breakpad ends up saving a minidump file that has two module entries for
+/// a given file: one that is read only for the entire file, and then one that
+/// is the real executable that is loaded into memory for execution. For memory
+/// mapped files they will typically show up and r--p permissions and a range
+/// matcning the entire range of the file on disk:
+///
+/// 00800000-00805000 r--p 00000000 fd:01 2838574           /tmp/a.out
+/// 00805000-00806000 r-xp 00001000 fd:01 1234567           /usr/lib/libc.so
+///
+/// This function should return false when asked about 0x00800000 with
+/// "/tmp/a.out" as the path.
+///
+/// \param[in] path
+///   The path to the module to check for in the memory regions. Only sequential
+///   memory regions whose paths match this path will be considered when looking
+///   for execute permissions.
+///
+/// \param[in] regions
+///   A sorted list of memory regions obtained from a call to
+///   CreateRegionsCacheFromLinuxMaps.
+///
+/// \param[in] base_of_image
+///   The load address of this module from BaseOfImage in the modules list.
+///
+/// \return
+///   True if a contiguous region of memory belonging to the module with a
+///   matching path exists that has executable permissions. Returns false if
+///   \a regions is empty or if there are no regions with execute permissions
+///   that match \a path.
+
+static bool CheckForLinuxExecutable(ConstString path,
+                                    const MemoryRegionInfos &regions,
+                                    lldb::addr_t base_of_image) {
+  if (regions.empty())
+    return false;
+  lldb::addr_t addr = base_of_image;
+  MemoryRegionInfo region = MinidumpParser::GetMemoryRegionInfo(regions, addr);
+  while (region.GetName() == path) {
+    if (region.GetExecutable() == MemoryRegionInfo::eYes)
+      return true;
+    addr += region.GetRange().GetByteSize();
+    region = MinidumpParser::GetMemoryRegionInfo(regions, addr);
+  }
+  return false;
+}
+
 std::vector<const minidump::Module *> MinidumpParser::GetFilteredModuleList() {
   Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_MODULES);
   auto ExpectedModules = GetMinidumpFile().getModuleList();
   if (!ExpectedModules) {
     LLDB_LOG_ERROR(log, ExpectedModules.takeError(),
                    "Failed to read module list: {0}");
     return {};
   }
 
+  // Create memory regions from the linux maps only. We do this to avoid issues
+  // with breakpad generated minidumps where if someone has mmap'ed a shared
+  // library into memory to accesss its data in the object file, we can get a
+  // minidump with two mappings for a binary: one whose base image points to a
+  // memory region that is read + execute and one that is read only.
+  MemoryRegionInfos linux_regions;
+  if (CreateRegionsCacheFromLinuxMaps(*this, linux_regions))
+    llvm::sort(linux_regions);
+
   // map module_name -> filtered_modules index
   typedef llvm::StringMap<size_t> MapType;
   MapType module_name_to_filtered_index;
 
   std::vector<const minidump::Module *> filtered_modules;
 
   for (const auto &module : *ExpectedModules) {
     auto ExpectedName = m_file->getString(module.ModuleNameRVA);
std::tie(iter, inserted) = module_name_to_filtered_index.try_emplace(
         *ExpectedName, filtered_modules.size());
 
     if (inserted) {
       // This module has not been seen yet, insert it into filtered_modules at
       // the index that was inserted into module_name_to_filtered_index using
       // "filtered_modules.size()" above.
       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.
+      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;
+        continue;
+      }
       // This module has been seen. Modules are sometimes mentioned multiple
       // times when they are mapped discontiguously, so find the module with
       // the lowest "base_of_image" and use that as the filtered module.
-      auto dup_module = filtered_modules[iter->second];
       if (module.BaseOfImage < dup_module->BaseOfImage)
         filtered_modules[iter->second] = &module;
     }
   }
   return filtered_modules;
 }
 
 const minidump::ExceptionStream *MinidumpParser::GetExceptionStream() {
llvm::ArrayRef<uint8_t> MinidumpParser::GetMemory(lldb::addr_t addr,
   if (addr < range->start || offset >= range->range_ref.size())
     return {};
 
   const size_t overlap = std::min(size, range->range_ref.size() - offset);
   return range->range_ref.slice(offset, overlap);
 }
 
 static bool
-CreateRegionsCacheFromLinuxMaps(MinidumpParser &parser,
-                                std::vector<MemoryRegionInfo> &regions) {
-  auto data = parser.GetStream(StreamType::LinuxMaps);
-  if (data.empty())
-    return false;
-  ParseLinuxMapRegions(llvm::toStringRef(data),
-                       [&](const lldb_private::MemoryRegionInfo &region,
-                           const lldb_private::Status &status) -> bool {
-    if (status.Success())
-      regions.push_back(region);
-    return true;
-  });
-  return !regions.empty();
-}
-
-static bool
 CreateRegionsCacheFromMemoryInfoList(MinidumpParser &parser,
                                      std::vector<MemoryRegionInfo> &regions) {
   Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_MODULES);
   auto ExpectedInfo = parser.GetMinidumpFile().getMemoryInfoList();
   if (!ExpectedInfo) {
     LLDB_LOG_ERROR(log, ExpectedInfo.takeError(),
                    "Failed to read memory info list: {0}");
     return false;
CreateRegionsCacheFromMemory64List(MinidumpParser &parser,
   llvm::ArrayRef<uint8_t> data =
       parser.GetStream(StreamType::Memory64List);
   if (data.empty())
     return false;
   llvm::ArrayRef<MinidumpMemoryDescriptor64> memory64_list;
   uint64_t base_rva;
   std::tie(memory64_list, base_rva) =
       MinidumpMemoryDescriptor64::ParseMemory64List(data);
-  
   if (memory64_list.empty())
     return false;
-    
   regions.reserve(memory64_list.size());
   for (const auto &memory_desc : memory64_list) {
     if (memory_desc.data_size == 0)
       continue;
     MemoryRegionInfo region;
     region.GetRange().SetRangeBase(memory_desc.start_of_memory_range);
     region.GetRange().SetByteSize(memory_desc.data_size);
     region.SetReadable(MemoryRegionInfo::eYes);
switch (stream_type) {
     ENUM_TO_CSTR(FacebookDumpErrorLog);
     ENUM_TO_CSTR(FacebookAppStateLog);
     ENUM_TO_CSTR(FacebookAbortReason);
     ENUM_TO_CSTR(FacebookThreadName);
     ENUM_TO_CSTR(FacebookLogcat);
   }
   return "unknown stream type";
 }
+
+MemoryRegionInfo
+MinidumpParser::GetMemoryRegionInfo(const MemoryRegionInfos &regions,
+                                    lldb::addr_t load_addr) {
+  MemoryRegionInfo region;
+  auto pos = llvm::upper_bound(regions, load_addr);
+  if (pos != regions.begin() &&
+      std::prev(pos)->GetRange().Contains(load_addr)) {
+    return *std::prev(pos);
+  }
+
+  if (pos == regions.begin())
+    region.GetRange().SetRangeBase(0);
+  else
+    region.GetRange().SetRangeBase(std::prev(pos)->GetRange().GetRangeEnd());
+
+  if (pos == regions.end())
+    region.GetRange().SetRangeEnd(UINT64_MAX);
+  else
+    region.GetRange().SetRangeEnd(pos->GetRange().GetRangeBase());
+
+  region.SetReadable(MemoryRegionInfo::eNo);
+  region.SetWritable(MemoryRegionInfo::eNo);
+  region.SetExecutable(MemoryRegionInfo::eNo);
+  region.SetMapped(MemoryRegionInfo::eNo);
+  return region;
+}

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

ArchSpec ProcessMinidump::GetArchitecture() {
 
   llvm::Triple triple;
   triple.setVendor(llvm::Triple::VendorType::UnknownVendor);
   triple.setArch(llvm::Triple::ArchType::x86);
   triple.setOS(llvm::Triple::OSType::Win32);
   return ArchSpec(triple);
 }
 
-static MemoryRegionInfo GetMemoryRegionInfo(const MemoryRegionInfos &regions,
-                                            lldb::addr_t load_addr) {
-  MemoryRegionInfo region;
-  auto pos = llvm::upper_bound(regions, load_addr);
-  if (pos != regions.begin() &&
-      std::prev(pos)->GetRange().Contains(load_addr)) {
-    return *std::prev(pos);
-  }
-
-  if (pos == regions.begin())
-    region.GetRange().SetRangeBase(0);
-  else
-    region.GetRange().SetRangeBase(std::prev(pos)->GetRange().GetRangeEnd());
-
-  if (pos == regions.end())
-    region.GetRange().SetRangeEnd(UINT64_MAX);
-  else
-    region.GetRange().SetRangeEnd(pos->GetRange().GetRangeBase());
-
-  region.SetReadable(MemoryRegionInfo::eNo);
-  region.SetWritable(MemoryRegionInfo::eNo);
-  region.SetExecutable(MemoryRegionInfo::eNo);
-  region.SetMapped(MemoryRegionInfo::eNo);
-  return region;
-}
-
 void ProcessMinidump::BuildMemoryRegions() {
   if (m_memory_regions)
     return;
   m_memory_regions.emplace();
   bool is_complete;
   std::tie(*m_memory_regions, is_complete) =
       m_minidump_parser->BuildMemoryRegions();
 
   if (is_complete)
     return;
 
   MemoryRegionInfos to_add;
   ModuleList &modules = GetTarget().GetImages();
   SectionLoadList &load_list = GetTarget().GetSectionLoadList();
   modules.ForEach([&](const ModuleSP &module_sp) {
     SectionList *sections = module_sp->GetSectionList();
     for (size_t i = 0; i < sections->GetSize(); ++i) {
       SectionSP section_sp = sections->GetSectionAtIndex(i);
       addr_t load_addr = load_list.GetSectionLoadAddress(section_sp);
       if (load_addr == LLDB_INVALID_ADDRESS)
         continue;
       MemoryRegionInfo::RangeType section_range(load_addr,
                                                 section_sp->GetByteSize());
       MemoryRegionInfo region =
-          ::GetMemoryRegionInfo(*m_memory_regions, load_addr);
+          MinidumpParser::GetMemoryRegionInfo(*m_memory_regions, load_addr);
       if (region.GetMapped() != MemoryRegionInfo::eYes &&
           region.GetRange().GetRangeBase() <= section_range.GetRangeBase() &&
           section_range.GetRangeEnd() <= region.GetRange().GetRangeEnd()) {
         to_add.emplace_back();
         to_add.back().GetRange() = section_range;
         to_add.back().SetLLDBPermissions(section_sp->GetPermissions());
         to_add.back().SetMapped(MemoryRegionInfo::eYes);
         to_add.back().SetName(module_sp->GetFileSpec().GetPath().c_str());
       }
     }
     return true;
   });
   m_memory_regions->insert(m_memory_regions->end(), to_add.begin(),
                            to_add.end());
   llvm::sort(*m_memory_regions);
 }
 
 Status ProcessMinidump::GetMemoryRegionInfo(lldb::addr_t load_addr,
                                             MemoryRegionInfo &region) {
   BuildMemoryRegions();
-  region = ::GetMemoryRegionInfo(*m_memory_regions, load_addr);
+  region = MinidumpParser::GetMemoryRegionInfo(*m_memory_regions, load_addr);
   return Status();
 }
 
 Status ProcessMinidump::GetMemoryRegions(MemoryRegionInfos &region_list) {
   BuildMemoryRegions();
   region_list = *m_memory_regions;
   return Status();
 }

lldb/unittests/Process/minidump/MinidumpParserTest.cpp

 //===-- MinidumpTypesTest.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Plugins/Process/minidump/MinidumpParser.h"
 #include "Plugins/Process/minidump/MinidumpTypes.h"
 #include "Plugins/Process/minidump/RegisterContextMinidump_x86_32.h"
 #include "Plugins/Process/minidump/RegisterContextMinidump_x86_64.h"
 #include "TestingSupport/SubsystemRAII.h"
 #include "TestingSupport/TestUtilities.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/ArchSpec.h"
)"),
   // If we have a module mentioned twice in the module list, the filtered
   // module list should contain the instance with the lowest BaseOfImage.
   std::vector<const minidump::Module *> filtered_modules =
       parser->GetFilteredModuleList();
   ASSERT_EQ(1u, filtered_modules.size());
   EXPECT_EQ(0x0000000000001000u, filtered_modules[0]->BaseOfImage);
 }
 
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleMappedFirst) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:            ModuleList
+    Modules:
+      - Base of Image:   0x400d0000
+        Size of Image:   0x00002000
+        Module Name:     '/usr/lib/libc.so'
+        CodeView Record: ''
+      - Base of Image:   0x400d3000
+        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 second in the module
+  // list, that it will become the selected module in the filtered list.
+  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, MinidumpDuplicateModuleMappedSecond) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:            ModuleList
+    Modules:
+      - Base of Image:   0x400d0000
+        Size of Image:   0x00002000
+        Module Name:     '/usr/lib/libc.so'
+        CodeView Record: ''
+      - Base of Image:   0x400d3000
+        Size of Image:   0x00001000
+        Module Name:     '/usr/lib/libc.so'
+        CodeView Record: ''
+  - Type:            LinuxMaps
+    Text:             |
+      400d0000-400d1000 r-xp 00010000 b3:04 227        /usr/lib/libc.so
+      400d1000-400d2000 rwxp 00001000 b3:04 227        /usr/lib/libc.so
+      400d2000-400d3000 rw-p 00000000 00:00 0
+      400d3000-400d5000 r--p 00000000 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.
+  std::vector<const minidump::Module *> filtered_modules =
+      parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400d0000u, filtered_modules[0]->BaseOfImage);
+}
+
+TEST_F(MinidumpParserTest, MinidumpDuplicateModuleSeparateCode) {
+  ASSERT_THAT_ERROR(SetUpFromYaml(R"(
+--- !minidump
+Streams:
+  - Type:            ModuleList
+    Modules:
+      - Base of Image:   0x400d0000
+        Size of Image:   0x00002000
+        Module Name:     '/usr/lib/libc.so'
+        CodeView Record: ''
+      - Base of Image:   0x400d5000
+        Size of Image:   0x00001000
+        Module Name:     '/usr/lib/libc.so'
+        CodeView Record: ''
+  - Type:            LinuxMaps
+    Text:             |
+      400d0000-400d3000 r--p 00000000 b3:04 227        /usr/lib/libc.so
+      400d3000-400d5000 rw-p 00000000 00:00 0
+      400d5000-400d6000 r--p 00000000 b3:04 227        /usr/lib/libc.so
+      400d6000-400d7000 r-xp 00010000 b3:04 227        /usr/lib/libc.so
+      400d7000-400d8000 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 if binaries are compiled with "-z separate-code",
+  // where the first region for a binary won't be marked as executable, that
+  // it gets selected by detecting the second consecutive mapping at 0x400d7000
+  // when asked about the a module mamed "/usr/lib/libc.so" at 0x400d5000.
+  std::vector<const minidump::Module *> filtered_modules =
+      parser->GetFilteredModuleList();
+  ASSERT_EQ(1u, filtered_modules.size());
+  EXPECT_EQ(0x400d5000u, filtered_modules[0]->BaseOfImage);
+}
+
 TEST_F(MinidumpParserTest, MinidumpModuleOrder) {
   ASSERT_THAT_ERROR(SetUpFromYaml(R"(
 --- !minidump
 Streams:
   - Type:            ModuleList
     Modules:
       - Base of Image:   0x0000000000002000
         Size of Image:   0x00001000
)"),
   EXPECT_THAT_EXPECTED(
       parser->GetMinidumpFile().getString(filtered_modules[0]->ModuleNameRVA),
       llvm::HasValue("/tmp/a"));
   EXPECT_EQ(0x0000000000001000u, filtered_modules[1]->BaseOfImage);
   EXPECT_THAT_EXPECTED(
       parser->GetMinidumpFile().getString(filtered_modules[1]->ModuleNameRVA),
       llvm::HasValue("/tmp/b"));
 }
-