Update sanitizer_allocator to use new API.

[flowerhack]

Summary

Update sanitizer_allocator to use new API.

Second patch in a series. First patch https://reviews.llvm.org/D39072

Updates MmapNoAccess / MmapFixed call sites in the saniziter_allocator
to use the new Init/Map APIs instead.

[mcgrathr]

Mostly LGTM

compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h

78 ↗

(On Diff #117864)

This one should pass the name too.

541 ↗

(On Diff #117864)

This should be static const char kAllocatorName[], or just drop it and use a string constant at the call site.

[alekseyshl]

lib/sanitizer_common/sanitizer_allocator_primary64.h

630

It should be:

uptr mapped = address_range.Map(beg, size, true /*tolerate_enomem*/);

which got me thinking, to avoid errors like this, maybe it's not that a bad idea to have two members instead of a default parameter:

uptr Map(uptr fixed_addr, uptr size);
uptr MapOrDie(uptr fixed_addr, uptr size);

indeed, we should rename Unmap to UnmapOrDie in this case.

On the other hand, this is the only use of tolerate_enomem, if we get it right right now, I think we'll be fine with the current API.

645

address_range.Unmap(beg, size)

[flowerhack]

lib/sanitizer_common/sanitizer_allocator_primary64.h

630

fixed

[alekseyshl]

lib/sanitizer_common/sanitizer_fuchsia.cc

274

Move it above Unmap

lib/sanitizer_common/sanitizer_win.cc

246

Why do we need MapImpl, why not just inline MapFixed... calls? Same question for other platforms.

[flowerhack]

lib/sanitizer_common/sanitizer_win.cc

246

good point, done

[flowerhack]

Thanks @alekseyshl !

However, https://reviews.llvm.org/D39072 needs to land before this one can be merged. Have you had a chance to look at that one yet? Thanks!

[cryptoad]

I have some local tests failure with this patch. Looking into it, do not submit.

lib/sanitizer_common/sanitizer_fuchsia.cc

268

Remove the 2 extra empty lines.

[cryptoad]

SanitizerCommon-tsan-x86_64-Linux :: Linux/decorate_proc_maps.cc appears to consistently fail with that patch.
With error message: ==14973==FATAL: ThreadSanitizer: failed to intercept open

[cryptoad]

This is due to decorate_proc_maps.
From Initialize, tsan's Init calls InitializeAllocator, which ends up calling MmapFixedNoAccess.
name is not null, so it continues to GetNamedMappingFd, which itself calls shm_open, which calls the open interceptor, and it all dies.

[cryptoad]

So this could go away with AllocatorName returning null, but it's not an actual fix, we would just be pretending the issue is not there.
Adding @dvyukov for input.

[cryptoad]

So to be clearer, this is a problem for every sanitizer intercepting open and using a map with a name and decorate_proc_maps=1 during the init.
esan seems to be the only other (than tsan) that does that. So when running the libc-intercept.c test case with decorate_proc_maps=1, it also segfaults.

[dvyukov]

In D38592#912999, @cryptoad wrote:

So this could go away with AllocatorName returning null, but it's not an actual fix, we would just be pretending the issue is not there.
Adding @dvyukov for input.

I don't fully understand what's the problem. Aleksey, can you please help with this?

[flowerhack]

The root bug is that, if you have decorate_proc_maps enabled, *and* actually use a named mapping, *and* are using a sanitizer intercepting open (just TSAN and ESAN), then we'll get an explosion during initialization.

I've updated this CL with what seems like a reasonable fix to me: change ReservedAddressRange::Init to not use name on POSIX, since that's where the problem happens (Fuchsia is fine with names).

I can file a bug for allowing named mappings can be used everywhere more easily (the fix may involve e.g. porting shm_open s.t. it's not invoking open, which is what's causing the problem), if that's desired?

Please let me know if this minor change is satisfactory so I can go ahead with it, thanks~

[alekseyshl]

In D38592#915616, @flowerhack wrote:

The root bug is that, if you have decorate_proc_maps enabled, *and* actually use a named mapping, *and* are using a sanitizer intercepting open (just TSAN and ESAN), then we'll get an explosion during initialization.

I've updated this CL with what seems like a reasonable fix to me: change ReservedAddressRange::Init to not use name on POSIX, since that's where the problem happens (Fuchsia is fine with names).

I can file a bug for allowing named mappings can be used everywhere more easily (the fix may involve e.g. porting shm_open s.t. it's not invoking open, which is what's causing the problem), if that's desired?

Please let me know if this minor change is satisfactory so I can go ahead with it, thanks~

Looks like a perfectly acceptable fix to me, we never named these mappings. Please add a comment with TODO there explaining why we're not passing name down (pretty much what cryptoad@ said).

[cryptoad]

lib/sanitizer_common/sanitizer_posix_libcdep.cc

346

I think you can skip the possible solution part.
If not, at least s/ccomplain/complain/ :)

lib/sanitizer_common/sanitizer_allocator_primary64.h

public:
   }
   uptr CompactPtrToPointer(uptr base, CompactPtrT ptr32) const {
     return base + (static_cast<uptr>(ptr32) << kCompactPtrScale);
   }
 
   void Init(s32 release_to_os_interval_ms) {
     uptr TotalSpaceSize = kSpaceSize + AdditionalSize();
     if (kUsingConstantSpaceBeg) {
-      CHECK_EQ(kSpaceBeg, reinterpret_cast<uptr>(
-                              MmapFixedNoAccess(kSpaceBeg, TotalSpaceSize)));
+      CHECK_EQ(kSpaceBeg, address_range.Init(TotalSpaceSize, AllocatorName(),
+                                             kSpaceBeg));
     } else {
-      NonConstSpaceBeg =
-          reinterpret_cast<uptr>(MmapNoAccess(TotalSpaceSize));
+      NonConstSpaceBeg = address_range.Init(TotalSpaceSize, AllocatorName());
       CHECK_NE(NonConstSpaceBeg, ~(uptr)0);
     }
     SetReleaseToOSIntervalMs(release_to_os_interval_ms);
     MapWithCallbackOrDie(SpaceEnd(), AdditionalSize());
   }
 
   s32 ReleaseToOSIntervalMs() const {
     return atomic_load(&release_to_os_interval_ms_, memory_order_relaxed);
if (same_chunk_count_per_page) {
       }
     }
     range_tracker.Done();
   }
 
  private:
   friend class MemoryMapper;
 
+  ReservedAddressRange address_range;
+  static const char *AllocatorName() { return "sanitizer_allocator"; }
+
   static const uptr kRegionSize = kSpaceSize / kNumClassesRounded;
   // FreeArray is the array of free-d chunks (stored as 4-byte offsets).
   // In the worst case it may reguire kRegionSize/SizeClassMap::kMinSize
   // elements, but in reality this will not happen. For simplicity we
   // dedicate 1/8 of the region's virtual space to FreeArray.
   static const uptr kFreeArraySize = kRegionSize / 8;
 
   static const bool kUsingConstantSpaceBeg = kSpaceBeg != ~(uptr)0;
uptr GetChunkIdx(uptr chunk, uptr size) const {
     return (u32)offset / (u32)size;
   }
 
   CompactPtrT *GetFreeArray(uptr region_beg) const {
     return reinterpret_cast<CompactPtrT *>(GetMetadataEnd(region_beg));
   }
 
   bool MapWithCallback(uptr beg, uptr size) {
-    uptr mapped = reinterpret_cast<uptr>(MmapFixedOrDieOnFatalError(beg, size));
+    uptr mapped = address_range.Map(beg, size);
     if (UNLIKELY(!mapped))
       return false;
     CHECK_EQ(beg, mapped);
     MapUnmapCallback().OnMap(beg, size);
     return true;
   }
 
   void MapWithCallbackOrDie(uptr beg, uptr size) {
-    CHECK_EQ(beg, reinterpret_cast<uptr>(MmapFixedOrDie(beg, size)));
+    CHECK_EQ(beg, address_range.MapOrDie(beg, size));
     MapUnmapCallback().OnMap(beg, size);
   }
 
   void UnmapWithCallbackOrDie(uptr beg, uptr size) {
     MapUnmapCallback().OnUnmap(beg, size);
-    UnmapOrDie(reinterpret_cast<void *>(beg), size);
+    address_range.Unmap(beg, size);
   }
 
   bool EnsureFreeArraySpace(RegionInfo *region, uptr region_beg,
                             uptr num_freed_chunks) {
     uptr needed_space = num_freed_chunks * sizeof(CompactPtrT);
     if (region->mapped_free_array < needed_space) {
       uptr new_mapped_free_array = RoundUpTo(needed_space, kFreeArrayMapSize);
       CHECK_LE(new_mapped_free_array, kFreeArraySize);

lib/sanitizer_common/sanitizer_common.h

 // Check if the built VMA size matches the runtime one.
 void CheckVMASize();
 void RunMallocHooks(const void *ptr, uptr size);
 void RunFreeHooks(const void *ptr);
 
 class ReservedAddressRange {
  public:
   uptr Init(uptr size, const char *name = nullptr, uptr fixed_addr = 0);
-  uptr Map(uptr fixed_addr, uptr size, bool tolerate_enomem = false);
+  uptr Map(uptr fixed_addr, uptr size);
+  uptr MapOrDie(uptr fixed_addr, uptr size);
   void Unmap(uptr addr, uptr size);
   void *base() const { return base_; }
   uptr size() const { return size_; }
 
  private:
   void* base_;
   uptr size_;
   const char* name_;

lib/sanitizer_common/sanitizer_fuchsia.cc

uptr ReservedAddressRange::Init(uptr init_size, const char* name,
   base_ = MmapNoAccess(init_size);
   size_ = init_size;
   name_ = name;
   return reinterpret_cast<uptr>(base_);
 }
 
 // Uses fixed_addr for now.
 // Will use offset instead once we've implemented this function for real.
-uptr ReservedAddressRange::Map(uptr fixed_addr, uptr map_size,
-                               bool tolerate_enomem) {
+uptr ReservedAddressRange::Map(uptr fixed_addr, uptr map_size) {
+  return reinterpret_cast<uptr>(MmapFixedOrDieOnFatalError(fixed_addr,
+                                                           map_size));
+}
+
+uptr ReservedAddressRange::MapOrDie(uptr fixed_addr, uptr map_size) {
   return reinterpret_cast<uptr>(MmapFixedOrDie(fixed_addr, map_size));
 }
 
 void ReservedAddressRange::Unmap(uptr addr, uptr size) {
   void* addr_as_void = reinterpret_cast<void*>(addr);
   uptr base_as_uptr = reinterpret_cast<uptr>(base_);
   // Only unmap at the beginning or end of the range.
   CHECK((addr_as_void == base_) || (addr + size == base_as_uptr + size_));
   CHECK_LE(size, size_);
   UnmapOrDie(reinterpret_cast<void*>(addr), size);
 }
 
+
+
 // MmapNoAccess and MmapFixedOrDie are used only by sanitizer_allocator.
 // Instead of doing exactly what they say, we make MmapNoAccess actually
 // just allocate a VMAR to reserve the address space.  Then MmapFixedOrDie
 // uses that VMAR instead of the root.
 
 zx_handle_t allocator_vmar = ZX_HANDLE_INVALID;
 uintptr_t allocator_vmar_base;
 size_t allocator_vmar_size;
 
 void *MmapNoAccess(uptr size) {
   size = RoundUpTo(size, PAGE_SIZE);
   CHECK_EQ(allocator_vmar, ZX_HANDLE_INVALID);
   uintptr_t base;
   zx_status_t status =

lib/sanitizer_common/sanitizer_posix_libcdep.cc

Report("ERROR: %s failed to "
            "allocate 0x%zx (%zd) bytes at address %zx (errno: %d)\n",
            SanitizerToolName, size, size, fixed_addr, reserrno);
   IncreaseTotalMmap(size);
   return (void *)p;
 }
 
 uptr ReservedAddressRange::Init(uptr size, const char *name, uptr fixed_addr) {
   if (fixed_addr) {
-    base_ = MmapFixedNoAccess(fixed_addr, size, name);
+    base_ = MmapFixedNoAccess(fixed_addr, size);
   } else {
     base_ = MmapNoAccess(size);
   }
   size_ = size;
   name_ = name;
   return reinterpret_cast<uptr>(base_);
 }
 
 // Uses fixed_addr for now.
 // Will use offset instead once we've implemented this function for real.
-uptr ReservedAddressRange::Map(uptr fixed_addr, uptr size,
-                               bool tolerate_enomem) {
-  if (tolerate_enomem) {
-    return reinterpret_cast<uptr>(MmapFixedOrDieOnFatalError(fixed_addr, size));
-  }
+uptr ReservedAddressRange::Map(uptr fixed_addr, uptr size) {
+
+uptr ReservedAddressRange::MapOrDie(uptr fixed_addr, uptr size) {
   return reinterpret_cast<uptr>(MmapFixedOrDie(fixed_addr, size));
 }
 
 void ReservedAddressRange::Unmap(uptr addr, uptr size) {
   void* addr_as_void = reinterpret_cast<void*>(addr);
   uptr base_as_uptr = reinterpret_cast<uptr>(base_);
   // Only unmap at the beginning or end of the range.
   CHECK((addr_as_void == base_) || (addr + size == base_as_uptr + size_));

lib/sanitizer_common/sanitizer_win.cc

internal_snprintf(mem_type, sizeof(mem_type), "memory at address 0x%zx",
                       fixed_addr);
     ReportMmapFailureAndDie(size, mem_type, "allocate", GetLastError());
   }
   return p;
 }
 
 // Uses fixed_addr for now.
 // Will use offset instead once we've implemented this function for real.
-uptr ReservedAddressRange::Map(uptr fixed_addr, uptr size,
-                               bool tolerate_enomem) {
-  if (tolerate_enomem) {
-    return reinterpret_cast<uptr>(MmapFixedOrDieOnFatalError(fixed_addr, size));
-  }
+uptr ReservedAddressRange::Map(uptr fixed_addr, uptr size) {
+
+uptr ReservedAddressRange::MapOrDie(uptr fixed_addr, uptr size) {
   return reinterpret_cast<uptr>(MmapFixedOrDie(fixed_addr, size));
 }
 
 void ReservedAddressRange::Unmap(uptr addr, uptr size) {
   void* addr_as_void = reinterpret_cast<void*>(addr);
   uptr base_as_uptr = reinterpret_cast<uptr>(base_);
   // Only unmap if it covers the entire range.
   CHECK((addr == base_as_uptr) && (size == size_));
   UnmapOrDie(addr_as_void, size);
 }