[sanitizer-common] [Darwin] Fix overlapping dyld segment addresses

[ndrewh]

This fixes two problems:

• dyld itself resides within the shared cache. MemoryMappingLayout incorrectly computes the slide for dyld's segments, causing them to (appear to) overlap with other modules. This can cause symbolication issues.
• The MemoryMappingLayout ranges on Darwin are not disjoint due to the fact that the LINKEDIT segments overlap for each module. We now ignore these segments to ensure the mapping is disjoint.

This adds a check for disjointness, and a runtime warning if this is ever violated (as that suggests issues in the sanitizer memory mapping). There is now a test to ensure that these problems do not recur.

rdar://163149325

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Andrew Haberlandt (ndrewh)

Changes

This fixes two problems:

• dyld itself resides within the shared cache. MemoryMappingLayout incorrectly computes the slide for dyld's segments, causing them to (appear to) overlap with other modules.
• The MemoryMappingLayout ranges on Darwin are not disjoint due to the fact that the LINKEDIT segments overlap for each module. We now ignore these segments to ensure the mapping is disjoint.

This adds a check for disjointness, and a runtime warning if this is ever violated (as that suggests issues in the sanitizer memory mapping). There is now a test to ensure that these problems do not recur.

rdar://163149325

---

Full diff: https://github.com/llvm/llvm-project/pull/166005.diff

3 Files Affected:

• (modified) compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h (+6)
• (modified) compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp (+60-8)
• (added) compiler-rt/test/asan/TestCases/Darwin/asan-verify-module-map.cpp (+20)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h
index d713ddf847dfb..8af0ad70f313f 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps.h
@@ -104,6 +104,12 @@ class MemoryMappingLayout : public MemoryMappingLayoutBase {
   // Adds all mapped objects into a vector.
   void DumpListOfModules(InternalMmapVectorNoCtor<LoadedModule> *modules);
 
+#  if SANITIZER_APPLE
+  // Verify that the memory mapping is well-formed. (e.g. mappings do not
+  // overlap)
+  bool Verify();
+#  endif
+
  protected:
 #if SANITIZER_APPLE
   virtual const ImageHeader *CurrentImageHeader();
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
index a9533d6fc04ca..b8a8ef7df2c39 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
@@ -82,6 +82,7 @@ void MemoryMappedSegment::AddAddressRanges(LoadedModule *module) {
 
 MemoryMappingLayout::MemoryMappingLayout(bool cache_enabled) {
   Reset();
+  Verify();
 }
 
 MemoryMappingLayout::~MemoryMappingLayout() {
@@ -187,6 +188,7 @@ typedef struct dyld_shared_cache_dylib_text_info
 
 extern bool _dyld_get_shared_cache_uuid(uuid_t uuid);
 extern const void *_dyld_get_shared_cache_range(size_t *length);
+extern intptr_t _dyld_get_image_slide(const struct mach_header* mh);
 extern int dyld_shared_cache_iterate_text(
     const uuid_t cacheUuid,
     void (^callback)(const dyld_shared_cache_dylib_text_info *info));
@@ -255,16 +257,16 @@ static bool NextSegmentLoad(MemoryMappedSegment *segment,
   layout_data->current_load_cmd_count--;
   if (((const load_command *)lc)->cmd == kLCSegment) {
     const SegmentCommand* sc = (const SegmentCommand *)lc;
+    if (strncmp(sc->segname, "__LINKEDIT", sizeof("__LINKEDIT")) == 0) {
+      // The LINKEDIT sections alias, so we ignore these sections to
+      // ensure our mappings are disjoint.
+      return false;
+    }
+
     uptr base_virt_addr, addr_mask;
     if (layout_data->current_image == kDyldImageIdx) {
-      base_virt_addr = (uptr)get_dyld_hdr();
-      // vmaddr is masked with 0xfffff because on macOS versions < 10.12,
-      // it contains an absolute address rather than an offset for dyld.
-      // To make matters even more complicated, this absolute address
-      // isn't actually the absolute segment address, but the offset portion
-      // of the address is accurate when combined with the dyld base address,
-      // and the mask will give just this offset.
-      addr_mask = 0xfffff;
+      base_virt_addr = (uptr)_dyld_get_image_slide(get_dyld_hdr());
+      addr_mask = ~0;
     } else {
       base_virt_addr =
           (uptr)_dyld_get_image_vmaddr_slide(layout_data->current_image);
@@ -445,6 +447,56 @@ bool MemoryMappingLayout::Next(MemoryMappedSegment *segment) {
   return false;
 }
 
+// NOTE: Verify expects to be called immediately after Reset(), since otherwise
+// it may miss some mappings. Verify will Reset() the layout after verification.
+bool MemoryMappingLayout::Verify() {
+  InternalMmapVector<char> module_name(kMaxPathLength);
+  MemoryMappedSegment segment(module_name.data(), module_name.size());
+  MemoryMappedSegmentData data;
+  segment.data_ = &data;
+
+  InternalMmapVector<MemoryMappedSegment> segments;
+  while (Next(&segment)) {
+    // skip the __PAGEZERO segment, its vmsize is 0
+    if (segment.filename[0] == '\0' || (segment.start == segment.end))
+      continue;
+
+    segments.push_back(segment);
+  }
+
+  // Verify that none of the segments overlap:
+  // 1. Sort the segments by the start address
+  // 2. Check that every segment starts after the previous one ends.
+  Sort(segments.data(), segments.size(),
+       [](MemoryMappedSegment& a, MemoryMappedSegment& b) {
+         return a.start < b.start;
+       });
+
+  // To avoid spam, we only print the report message once-per-process.
+  static bool invalid_module_map_reported = false;
+  bool well_formed = true;
+
+  for (size_t i = 1; i < segments.size(); i++) {
+    uptr cur_start = segments[i].start;
+    uptr prev_end = segments[i - 1].end;
+    if (cur_start < prev_end) {
+      well_formed = false;
+      VReport(2, "Overlapping mappings: cur_start = %p prev_end = %p\n",
+              (void*)cur_start, (void*)prev_end);
+      if (!invalid_module_map_reported) {
+        Report(
+            "WARN: Invalid dyld module map detected. This is most likely a bug "
+            "in the sanitizer.\n");
+        Report("WARN: Backtraces may be unreliable.\n");
+        invalid_module_map_reported = true;
+      }
+    }
+  }
+
+  Reset();
+  return well_formed;
+}
+
 void MemoryMappingLayout::DumpListOfModules(
     InternalMmapVectorNoCtor<LoadedModule> *modules) {
   Reset();
diff --git a/compiler-rt/test/asan/TestCases/Darwin/asan-verify-module-map.cpp b/compiler-rt/test/asan/TestCases/Darwin/asan-verify-module-map.cpp
new file mode 100644
index 0000000000000..e48b2fd8cb982
--- /dev/null
+++ b/compiler-rt/test/asan/TestCases/Darwin/asan-verify-module-map.cpp
@@ -0,0 +1,20 @@
+// This test simply checks that the "Invalid module map" warning is not printed
+// in the output of a backtrace.
+
+// RUN: %clangxx_asan -O0 -g %s -o %t.executable
+// RUN: %env_asan_opts="print_module_map=2" not %run %t.executable 2>&1 | FileCheck %s
+
+// CHECK-NOT: WARN: Invalid module map
+
+#include <cstdlib>
+
+extern "C" void foo(int *a) { *a = 5; }
+
+int main() {
+  int *a = (int *)malloc(sizeof(int));
+  if (!a)
+    return 0;
+  free(a);
+  foo(a);
+  return 0;
+}
\ No newline at end of file

[DanBlackwell]

Can you see about strengthening the test? Happy for this to be merged once that has been considered.

[DanBlackwell]

I'm happy for this to be merged, thanks!

[vitalybuka]

Can you make it a static function, with parameters of what is needed
and avoid Verify in the header?

Unless there is a plan to extend it to other platforms?

[ndrewh]

I had imagined it would eventually be extended to other platforms, but I didn't personally do that here since I wasn't sure other platforms guaranteed non-overlapping regions (and didn't have the ability to easily test this change on other platforms).

[padriff]

We can make it a static function in this file. If other platforms need to introduce something similar then it can be promoted to MemoryMappingLayout

[ndrewh]

This is not straightforward as a static function because data_ is a private member of MemoryMappedSegment which is only accessible inside MemoryMappingLayout because it is a friend class.

[padriff]

Looks good with Vitaly's suggestion

[ndrewh]

I re-did the verification on top of DumpListOfModules() (rather than Next()) so that this could be a static function. This gets around the issue with accessing the private data_ member of MemoryMappedSegment.

[padriff]

update looks good