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

This one should pass the name too.

541

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

629 ↗

(On Diff #119986)

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.

644 ↗

(On Diff #119986)

address_range.Unmap(beg, size)

[flowerhack]

lib/sanitizer_common/sanitizer_allocator_primary64.h

629 ↗

(On Diff #119986)

fixed

[alekseyshl]

lib/sanitizer_common/sanitizer_fuchsia.cc

272 ↗

(On Diff #120323)

Move it above Unmap

lib/sanitizer_common/sanitizer_win.cc

246 ↗

(On Diff #120323)

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

[flowerhack]

lib/sanitizer_common/sanitizer_win.cc

246 ↗

(On Diff #120323)

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 ↗

(On Diff #121069)

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 ↗

(On Diff #121790)

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

compiler-rt/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, kAllocatorName, kSpaceBeg));
     } else {
-      NonConstSpaceBeg =
-          reinterpret_cast<uptr>(MmapNoAccess(TotalSpaceSize));
+      NonConstSpaceBeg = address_range.Init(TotalSpaceSize);
       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;
+  const char kAllocatorName[20] = "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.Map(beg, size));
     MapUnmapCallback().OnMap(beg, size);
   }
 
   void UnmapWithCallbackOrDie(uptr beg, uptr size) {
     MapUnmapCallback().OnUnmap(beg, size);
     UnmapOrDie(reinterpret_cast<void *>(beg), size);
   }