Make sure to scan mmap'd memory regions for root pointers on OS X

[fjricci]

Summary

In the general case, we only need to check for root regions inside
the memory map returned by procmaps. However, on Darwin,
we also need to check inside mmap'd regions, which aren't returned
in the list of modules we get from procmaps.

This patch refactors memory region scanning on darwin to reduce
code duplication with the kernel alloc once page scan.

[kubamracek]

lib/lsan/lsan_common_mac.cc

150–156 ↗

(On Diff #95630)

How many times is this called? This should really be called only once per the whole scan, and it's hard to see if that's the case or not.

The naming is weird: ProcessMemoryRegion suggests that this processes some specific region, but in fact, this function *scans* for a new region. I don't follow why this is called from ProcessPlatformSpecificRootRegion.

[fjricci]

lib/lsan/lsan_common_mac.cc

150–156 ↗

(On Diff #95630)

This would be called once when checking for the kernel alloc once page, and once when processing the root regions. I could probably change it to do both in one loop, if that's preferable, but it would require changing the ProcessPlatformSpecificAllocations API to take root region pointers (probably not a big deal, since only mac uses that API currently).

It needs to be called from ProcessPlatformSpecificRootRegion to find root regions contained within mmap'd memory regions.

[fjricci]

lib/lsan/lsan_common_mac.cc

150–156 ↗

(On Diff #95630)

Actually that refactor would probably be even more complicated than this version, as we'd need to encapsulate the functionality of ProcessRootRegions (which iterates over the root regions, scanning each one). Currently we avoid duplicating this behavior by doing the platform-specific scan inside the call tree of that function.

[kubamracek]

lib/lsan/lsan_common_mac.cc

150–156 ↗

(On Diff #95630)

Can you try doing something else then: Create a GetKernelAllocOnceRegion, which will return the start and size of the VM_MEMORY_OS_ALLOC_ONCE region, and then it will cache the result; 2nd calls to this will just return the cached values. Then you can call this function as many times as you need.

[fjricci]

lib/lsan/lsan_common_mac.cc

150–156 ↗

(On Diff #95630)

I don't think that will change the performance at all, but perhaps the design here isn't super clear.

ProcessMemoryRegion is called (1 + num_root_regions) times, but it's only called with AllocOncePageCb one time. So there should be no duplication or caching benefit there.

I do see now how this could become slow if a large number of root regions was registered, since we'll iterate through all the pages for each root region. Perhaps I should refactor this such that ProcessMemoryRegion is only called once, and for each block does the VM_MEMORY_OS_ALLOC_ONCE check, in addition to iterating through the set of registered root regions to check if any are a match.

I think it comes down to whether we'd rather:

1. Iterate through the root regions once, and on each iteration, traverse the list of memory regions
2. Iterate through the list of memory regions once, and on each iteration run through all of the root regions.

My assumption is that 1) is simpler, but slower (and is what this patch does). 2) should be quite a bit more performant, but will also be a bit messier.

I'll put up a diff for 2) but save a copy of this one in case that turns out to be too ugly.

[kubamracek]

lib/lsan/lsan_common_mac.cc

150–156 ↗

(On Diff #95630)

Right, my suggestion is not to address performance, but to improve readability, and to make it clear what each function does. Currently, the name ProcessMemoryRegion doesn't suggest why we should be iterating over memory chunks with vm_region_recurse_64.

In any case, if the purpose of this loop is to find the alloc once region, we should have a separate function that does just that and nothing else.

[fjricci]

Rework to only recurse memory regions once

This actually turns out to be relatively clean, I think.

[fjricci]

Remove extra function definition

[kubamracek]

lib/lsan/lsan_common_mac.cc

149–156 ↗

(On Diff #95775)

Why is this needed again? I thought that we only need to account for the VM_MEMORY_OS_ALLOC_ONCE region.

Are we doing something like that on Linux? Why do we need to scan all mmap'd regions on Darwin only?

[fjricci]

lib/lsan/lsan_common_mac.cc

149–156 ↗

(On Diff #95775)

It appears that the Linux implementation of MemoryMappingLayout from sanitizer_procmaps includes mmap'd regions, while the Darwin implementation does not. I'm not sure whether that's because mmap-ing works differently on Darwin, or if it's a result of the differences between how we generate the module listing.

[fjricci]

Yeah, it appears that sanitizer_procmaps_mac iterates using _dyld_get_image_vmaddr_slide, which will iterate over only images, as opposed to the vm_region_recurse_64 which iterates over all memory regions. The linux implementation just reads straight out of /proc/self/maps.

[kubamracek]

lib/lsan/lsan_common_mac.cc

149–156 ↗

(On Diff #95775)

I see. MemoryMappingLayout on Linux parses /proc/self/maps, which includes mmap regions, but Darwin uses dyld APIs to only iterate through modules.

For the moment, I think this is fine, but we should make a comment about the difference here. Ultimately, it would be better to converge the implementations of MemoryMappingLayout to do the same.

[fjricci]

lib/lsan/lsan_common_mac.cc

149–156 ↗

(On Diff #95775)

Ok. I'll make a comment and put looking into unifying the behavior on my list of things to do.

[fjricci]

Update comment and add TODO

[kubamracek]

lib/lsan/lsan_common.cc

86 ↗

(On Diff #95789)

Can this be static since we now have GetRootRegions?

291 ↗

(On Diff #95789)

Can this be const RootRegion & instead? The rest of compiler-rt seems to heavily prefer const X & rather than X const &. Same below.

[fjricci]

Done in r300765

[kubamracek]

Thanks!

compiler-rt/trunk/lib/lsan/lsan_common.h

 };
 
 typedef InternalMmapVector<uptr> Frontier;
 
 // Platform-specific functions.
 void InitializePlatformSpecificModules();
 void ProcessGlobalRegions(Frontier *frontier);
 void ProcessPlatformSpecificAllocations(Frontier *frontier);
+
+struct RootRegion {
+  uptr begin;
+  uptr size;
+};
+
+InternalMmapVector<RootRegion> const *GetRootRegions();
+void ScanRootRegion(Frontier *frontier, RootRegion const &region,
+                    uptr region_begin, uptr region_end, uptr prot);
 // Run stoptheworld while holding any platform-specific locks.
 void DoStopTheWorld(StopTheWorldCallback callback, void* argument);
 
 void ScanRangeForPointers(uptr begin, uptr end,
                           Frontier *frontier,
                           const char *region_type, ChunkTag tag);
 void ScanGlobalRange(uptr begin, uptr end, Frontier *frontier);
 

compiler-rt/trunk/lib/lsan/lsan_common.cc

if (&__lsan_default_suppressions)
     suppression_ctx->Parse(__lsan_default_suppressions());
 }
 
 static SuppressionContext *GetSuppressionContext() {
   CHECK(suppression_ctx);
   return suppression_ctx;
 }
 
-struct RootRegion {
-  const void *begin;
-  uptr size;
-};
-
 InternalMmapVector<RootRegion> *root_regions;
 
+InternalMmapVector<RootRegion> const *GetRootRegions() { return root_regions; }
+
 void InitializeRootRegions() {
   CHECK(!root_regions);
   ALIGNED(64) static char placeholder[sizeof(InternalMmapVector<RootRegion>)];
   root_regions = new(placeholder) InternalMmapVector<RootRegion>(1);
 }
 
 void InitCommonLsan() {
   InitializeRootRegions();
if (flags()->use_tls) {
         // We are handling a thread with DTLS under destruction. Log about
         // this and continue.
         LOG_THREADS("Thread %d has DTLS under destruction.\n", os_id);
       }
     }
   }
 }
 
-static void ProcessRootRegion(Frontier *frontier, uptr root_begin,
-                              uptr root_end) {
-  MemoryMappingLayout proc_maps(/*cache_enabled*/true);
+void ScanRootRegion(Frontier *frontier, RootRegion const &root_region,
+                    uptr region_begin, uptr region_end, uptr prot) {
+  uptr intersection_begin = Max(root_region.begin, region_begin);
+  uptr intersection_end = Min(region_end, root_region.begin + root_region.size);
+  if (intersection_begin >= intersection_end) return;
+  bool is_readable = prot & MemoryMappingLayout::kProtectionRead;
+  LOG_POINTERS("Root region %p-%p intersects with mapped region %p-%p (%s)\n",
+               root_region.begin, root_region.begin + root_region.size,
+               region_begin, region_end,
+               is_readable ? "readable" : "unreadable");
+  if (is_readable)
+    ScanRangeForPointers(intersection_begin, intersection_end, frontier, "ROOT",
+                         kReachable);
+}
+
+static void ProcessRootRegion(Frontier *frontier,
+                              RootRegion const &root_region) {
+  MemoryMappingLayout proc_maps(/*cache_enabled*/ true);
   uptr begin, end, prot;
   while (proc_maps.Next(&begin, &end,
                         /*offset*/ nullptr, /*filename*/ nullptr,
                         /*filename_size*/ 0, &prot)) {
-    uptr intersection_begin = Max(root_begin, begin);
-    uptr intersection_end = Min(end, root_end);
-    if (intersection_begin >= intersection_end) continue;
-    bool is_readable = prot & MemoryMappingLayout::kProtectionRead;
-    LOG_POINTERS("Root region %p-%p intersects with mapped region %p-%p (%s)\n",
-                 root_begin, root_end, begin, end,
-                 is_readable ? "readable" : "unreadable");
-    if (is_readable)
-      ScanRangeForPointers(intersection_begin, intersection_end, frontier,
-                           "ROOT", kReachable);
+    ScanRootRegion(frontier, root_region, begin, end, prot);
   }
 }
 
 // Scans root regions for heap pointers.
 static void ProcessRootRegions(Frontier *frontier) {
   if (!flags()->use_root_regions) return;
   CHECK(root_regions);
   for (uptr i = 0; i < root_regions->size(); i++) {
-    RootRegion region = (*root_regions)[i];
-    uptr begin_addr = reinterpret_cast<uptr>(region.begin);
-    ProcessRootRegion(frontier, begin_addr, begin_addr + region.size);
+    ProcessRootRegion(frontier, (*root_regions)[i]);
   }
 }
 
 static void FloodFillTag(Frontier *frontier, ChunkTag tag) {
   while (frontier->size()) {
     uptr next_chunk = frontier->back();
     frontier->pop_back();
     LsanMetadata m(next_chunk);

 #endif // CAN_SANITIZE_LEAKS
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __lsan_register_root_region(const void *begin, uptr size) {
 #if CAN_SANITIZE_LEAKS
   BlockingMutexLock l(&global_mutex);
   CHECK(root_regions);
-  RootRegion region = {begin, size};
+  RootRegion region = {reinterpret_cast<uptr>(begin), size};
   root_regions->push_back(region);
   VReport(1, "Registered root region at %p of size %llu\n", begin, size);
 #endif // CAN_SANITIZE_LEAKS
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __lsan_unregister_root_region(const void *begin, uptr size) {
 #if CAN_SANITIZE_LEAKS
   BlockingMutexLock l(&global_mutex);
   CHECK(root_regions);
   bool removed = false;
   for (uptr i = 0; i < root_regions->size(); i++) {
     RootRegion region = (*root_regions)[i];
-    if (region.begin == begin && region.size == size) {
+    if (region.begin == reinterpret_cast<uptr>(begin) && region.size == size) {
       removed = true;
       uptr last_index = root_regions->size() - 1;
       (*root_regions)[i] = (*root_regions)[last_index];
       root_regions->pop_back();
       VReport(1, "Unregistered root region at %p of size %llu\n", begin, size);
       break;
     }
   }

compiler-rt/trunk/lib/lsan/lsan_common_mac.cc

for (const __sanitizer::LoadedModule::AddressRange &range :
          modules[i].ranges()) {
       if (range.executable || !range.readable) continue;
 
       ScanGlobalRange(range.beg, range.end, frontier);
     }
   }
 }
 
-// libxpc stashes some pointers in the Kernel Alloc Once page,
-// make sure not to report those as leaks.
 void ProcessPlatformSpecificAllocations(Frontier *frontier) {
   mach_port_name_t port;
   if (task_for_pid(mach_task_self(), internal_getpid(), &port)
       != KERN_SUCCESS) {
     return;
   }
 
   unsigned depth = 1;
   vm_size_t size = 0;
   vm_address_t address = 0;
   kern_return_t err = KERN_SUCCESS;
   mach_msg_type_number_t count = VM_REGION_SUBMAP_INFO_COUNT_64;
 
+  InternalMmapVector<RootRegion> const *root_regions = GetRootRegions();
+
   while (err == KERN_SUCCESS) {
     struct vm_region_submap_info_64 info;
     err = vm_region_recurse_64(port, &address, &size, &depth,
                                (vm_region_info_t)&info, &count);
+
+    uptr end_address = address + size;
+
+    // libxpc stashes some pointers in the Kernel Alloc Once page,
+    // make sure not to report those as leaks.
     if (info.user_tag == VM_MEMORY_OS_ALLOC_ONCE) {
-      ScanRangeForPointers(address, address + size, frontier,
-                           "GLOBAL", kReachable);
-      return;
+      ScanRangeForPointers(address, end_address, frontier, "GLOBAL",
+                           kReachable);
+    }
+
+    // This additional root region scan is required on Darwin in order to
+    // detect root regions contained within mmap'd memory regions, because
+    // the Darwin implementation of sanitizer_procmaps traverses images
+    // as loaded by dyld, and not the complete set of all memory regions.
+    //
+    // TODO(fjricci) - remove this once sanitizer_procmaps_mac has the same
+    // behavior as sanitizer_procmaps_linux and traverses all memory regions
+    if (flags()->use_root_regions) {
+      for (uptr i = 0; i < root_regions->size(); i++) {
+        ScanRootRegion(frontier, (*root_regions)[i], address, end_address,
+                       info.protection);
+      }
     }
-    address += size;
+
+    address = end_address;
   }
 }
 
 void DoStopTheWorld(StopTheWorldCallback callback, void *argument) {
   StopTheWorld(callback, argument);
 }
 
 } // namespace __lsan
 
 #endif // CAN_SANITIZE_LEAKS && SANITIZER_MAC