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

[ndrewh]

Revert #166005 due to breaking x86 iOS sims

We're sometimes hitting a allocator assert when running x86 iOS sim tests. I don't believe this PR is at fault, but there's probably a memory safety / allocator issue somewhere which the allocation pattern here is exposing.

[llvmbot]

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

Author: Andrew Haberlandt (ndrewh)

Changes

Revert #166005 due to breaking x86 iOS sims

We're sometimes hitting a allocator assert when running x86 iOS sim tests. I don't believe this PR is at fault, but there's probably a memory safety / allocator issue somewhere which the allocation pattern here is exposing.

---

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

2 Files Affected:

• (modified) compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp (+17-61)
• (removed) compiler-rt/test/asan/TestCases/Darwin/asan-verify-module-map.cpp (-25)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
index 72f4bbf212f9a..a5ec85ae16460 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
@@ -45,6 +45,7 @@ struct MemoryMappedSegmentData {
   const char *current_load_cmd_addr;
   u32 lc_type;
   uptr base_virt_addr;
+  uptr addr_mask;
 };
 
 template <typename Section>
@@ -53,58 +54,12 @@ static void NextSectionLoad(LoadedModule *module, MemoryMappedSegmentData *data,
   const Section *sc = (const Section *)data->current_load_cmd_addr;
   data->current_load_cmd_addr += sizeof(Section);
 
-  uptr sec_start = sc->addr + data->base_virt_addr;
+  uptr sec_start = (sc->addr & data->addr_mask) + data->base_virt_addr;
   uptr sec_end = sec_start + sc->size;
   module->addAddressRange(sec_start, sec_end, /*executable=*/false, isWritable,
                           sc->sectname);
 }
 
-static bool VerifyMemoryMapping(MemoryMappingLayout* mapping) {
-  InternalMmapVector<LoadedModule> modules;
-  modules.reserve(128);  // matches DumpProcessMap
-  mapping->DumpListOfModules(&modules);
-
-  InternalMmapVector<LoadedModule::AddressRange> segments;
-  for (uptr i = 0; i < modules.size(); ++i) {
-    for (auto& range : modules[i].ranges()) {
-      segments.push_back(range);
-    }
-  }
-
-  // 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(),
-       [](LoadedModule::AddressRange& a, LoadedModule::AddressRange& b) {
-         return a.beg < b.beg;
-       });
-
-  // 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].beg;
-    uptr prev_end = segments[i - 1].end;
-    if (cur_start < prev_end) {
-      well_formed = false;
-      VReport(2, "Overlapping mappings: %s start = %p, %s end = %p\n",
-              segments[i].name, (void*)cur_start, segments[i - 1].name,
-              (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;
-      }
-    }
-  }
-
-  mapping->Reset();
-  return well_formed;
-}
-
 void MemoryMappedSegment::AddAddressRanges(LoadedModule *module) {
   // Don't iterate over sections when the caller hasn't set up the
   // data pointer, when there are no sections, or when the segment
@@ -130,7 +85,6 @@ void MemoryMappedSegment::AddAddressRanges(LoadedModule *module) {
 
 MemoryMappingLayout::MemoryMappingLayout(bool cache_enabled) {
   Reset();
-  VerifyMemoryMapping(this);
 }
 
 MemoryMappingLayout::~MemoryMappingLayout() {
@@ -236,7 +190,6 @@ 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));
@@ -305,21 +258,23 @@ 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 are for internal linker use, and may alias
-      // with the LINKEDIT section for other modules. (If we included them,
-      // our memory map would contain overlappping sections.)
-      return false;
-    }
-
-    uptr base_virt_addr;
-    if (layout_data->current_image == kDyldImageIdx)
-      base_virt_addr = (uptr)_dyld_get_image_slide(get_dyld_hdr());
-    else
+    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;
+    } else {
       base_virt_addr =
           (uptr)_dyld_get_image_vmaddr_slide(layout_data->current_image);
+      addr_mask = ~0;
+    }
 
-    segment->start = sc->vmaddr + base_virt_addr;
+    segment->start = (sc->vmaddr & addr_mask) + base_virt_addr;
     segment->end = segment->start + sc->vmsize;
     // Most callers don't need section information, so only fill this struct
     // when required.
@@ -329,6 +284,7 @@ static bool NextSegmentLoad(MemoryMappedSegment *segment,
           (const char *)lc + sizeof(SegmentCommand);
       seg_data->lc_type = kLCSegment;
       seg_data->base_virt_addr = base_virt_addr;
+      seg_data->addr_mask = addr_mask;
       internal_strncpy(seg_data->name, sc->segname,
                        ARRAY_SIZE(seg_data->name));
     }
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
deleted file mode 100644
index 7660841c72877..0000000000000
--- a/compiler-rt/test/asan/TestCases/Darwin/asan-verify-module-map.cpp
+++ /dev/null
@@ -1,25 +0,0 @@
-// This test simply checks that the "Invalid dyld module map" warning is not printed
-// in the output of a backtrace.
-
-// RUN: %clangxx_asan -DSHARED_LIB -g %s -dynamiclib -o %t.dylib
-// RUN: %clangxx_asan -O0 -g %s %t.dylib -o %t.executable
-// RUN: %env_asan_opts="print_module_map=2" not %run %t.executable 2>&1 | FileCheck %s -DDYLIB=%t.dylib
-
-// CHECK-NOT: WARN: Invalid dyld module map
-// CHECK-DAG: 0x{{.*}}-0x{{.*}} [[DYLIB]]
-// CHECK-DAG: 0x{{.*}}-0x{{.*}} {{.*}}libsystem
-
-#ifdef SHARED_LIB
-extern "C" void foo(int *a) { *a = 5; }
-#else
-#  include <cstdlib>
-
-extern "C" void foo(int *a);
-
-int main() {
-  int *a = (int *)malloc(sizeof(int));
-  free(a);
-  foo(a);
-  return 0;
-}
-#endif
\ No newline at end of file

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff origin/main HEAD --extensions cpp -- compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp --diff_from_common_commit

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
index a5ec85ae1..78ec4cb22 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_procmaps_mac.cpp
@@ -83,9 +83,7 @@ void MemoryMappedSegment::AddAddressRanges(LoadedModule *module) {
   } while (--data_->nsects);
 }
 
-MemoryMappingLayout::MemoryMappingLayout(bool cache_enabled) {
-  Reset();
-}
+MemoryMappingLayout::MemoryMappingLayout(bool cache_enabled) { Reset(); }
 
 MemoryMappingLayout::~MemoryMappingLayout() {
 }
@@ -189,7 +187,7 @@ typedef struct dyld_shared_cache_dylib_text_info
     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 const void* _dyld_get_shared_cache_range(size_t* length);
 extern int dyld_shared_cache_iterate_text(
     const uuid_t cacheUuid,
     void (^callback)(const dyld_shared_cache_dylib_text_info *info));