[sanitizer] Update scudo to use new API

[flowerhack]

Summary

The ScudoAllocator uses a SecondaryHeader to keep track of the size and base address of each mmap'd chunk.

This aligns well with what the ReservedAddressRange is trying to do. This changeset converts the scudo allocator from using the MmapNoAccess/MmapFixed APIs to the ReservedAddressRange::Init and ::Map APIs. In doing so, it replaces the SecondayHeader struct with the ReservedAddressRange object.

This is part 3 of a 4 part changeset; part 1 https://reviews.llvm.org/D39072 and part 2 https://reviews.llvm.org/D38592

[cryptoad]

Some comments, even though it's not done yet.

compiler-rt/lib/sanitizer_common/sanitizer_common.h

87

I think you can just pad the structure in the secondary or just declare it with ALIGNED there maybe.
Haven't tried either.

compiler-rt/lib/sanitizer_common/sanitizer_posix.cc

156

That would be great to have that in D38437 in one form or another.

compiler-rt/lib/scudo/scudo_allocator_secondary.h

87

I would avoid a placement new here and use a bit_cast or memcpy there.

126

Might want to rename that API as to getReservedAddressRange.

[flowerhack]

compiler-rt/lib/sanitizer_common/sanitizer_posix.cc

156

moved to D38437

[flowerhack]

rebased against patch 2 of this series

feedback much appreciated!

[flowerhack]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

87

i switched this around at some point, but it turns out getting a specific alignment for an instance of a class, outside of the declaration of that class, is pretty much Not A Thing.

there's a way to do it with placement new, but, seemed more straightforward to subclass ReservedAddressRange, s.t. ScudoReservedAddressRange is properly-aligned but otherwise identical. lmk if this is a bad idea!

[cryptoad]

On the form:

• some lines are have more 80 chars
• variable names should be compliant with https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly (eg: s/address_range/AddressRange and so on).

On the content, I think we can get rid of ScudoReservedAddressRange and only work with a ReservedAddressRange.
We don't have to make the structure a multiple of MinAlignment itself, but just account for it in space computations. So Introduce something like:
uptr AlignedReservedAddressRangeSize = RoundUpTo(sizeof(ReservedAddressRange), MinAlignment);
Then compute the mapping size using that.
You can just store and load the ReservedAddressRange at Ptr-sizeof(sizeof(ReservedAddressRange)) , this will just result in potential empty space before the structure.
Also instead of using the Swap construct you should be able to memcpy the structure (or assign it, it should be POD), it will be inlined.

[cryptoad]

Some style comments for the Scudo part.
And a few other changes.

lib/scudo/scudo_allocator_secondary.h

84 ↗

(On Diff #121998)

ALIGNED(MinAlignment) shouldn't be necessary?

108 ↗

(On Diff #121998)

This is a somewhat awkward new line, since it's aligned on the content of the cast as opposed to the parameter itself.

113 ↗

(On Diff #121998)

s/MapSize/ReservedAddressRange size. Or something like that.

115 ↗

(On Diff #121998)

Same alignment comment as above.

120 ↗

(On Diff #121998)

This can go away.

129 ↗

(On Diff #121998)

You can do something like:

const uptr AlignedReservedAddressRangeSize =
    RoundUpTo(sizeof(ReservedAddressRange), MinAlignment);

Same a line and the arithmetic expression is on the same line.

[cryptoad]

LGTM pending tests. Could you rebase it please?

[flowerhack]

rebased!

[flowerhack]

& tests pass + builds and works on Linux and Fuchsia

[cryptoad]

lib/scudo/scudo_allocator_secondary.h

105 ↗

(On Diff #122128)

Didn't catch that one before: but shouldn't that be StoredRange->Unmap()?

[cryptoad]

lib/scudo/scudo_allocator_secondary.h

99 ↗

(On Diff #122463)

You should be able to do directly ReservedAddressRange AddressRange = *getReservedAddressRange(Ptr); and then you don't have to use the StoredRange intermediate variable. Keep the comment though, but move it here.

[cryptoad]

Tested Android & Linux locally, it's passing. LGTM.

[cryptoad]

Rebase & small style changes.

[cryptoad]

Removing unneeded reinterpret_cast.

compiler-rt/lib/sanitizer_common/sanitizer_common.h

 uptr GetThreadSelf();
 void GetThreadStackTopAndBottom(bool at_initialization, uptr *stack_top,
                                 uptr *stack_bottom);
 void GetThreadStackAndTls(bool main, uptr *stk_addr, uptr *stk_size,
                           uptr *tls_addr, uptr *tls_size);
 
 // Memory management
 // This class relies on zero-initialization.
-class ReservedAddressRange {
+// TODO(flowerhack) this is a hack; move MinAlignment into sanitizer_common.h
+class alignas(1 << FIRST_32_SECOND_64(3, 4)) ReservedAddressRange {
  public:
   uptr Init(uptr size, const char *name = nullptr, uptr fixed_addr = 0);
   uptr Map(uptr offset, uptr size, bool tolerate_enomem = false);
+  void Unmap(void *addr, uptr size);
   void *base() { return base_; }
+  uptr size() { return size_; }
  private:
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wunused-private-field"
   uptr os_cookie_;
   void* base_;
   uptr size_;
 };
 

compiler-rt/lib/sanitizer_common/sanitizer_posix.cc

 uptr ReservedAddressRange::Map(uptr fixed_addr, uptr size,
                                bool tolerate_enomem) {
   if (tolerate_enomem) {
     return reinterpret_cast<uptr>(MmapFixedOrDieOnFatalError(fixed_addr, size));
   }
   return reinterpret_cast<uptr>(MmapFixedOrDie(fixed_addr, size));
 }
 
+// TODO(flowerhack): add sanity checks to make sure addr is actually
+// within address_range
+void ReservedAddressRange::Unmap(void *addr, uptr size) {
+  UnmapOrDie(addr, size);
+  if (addr == base_) {
+    base_ = reinterpret_cast<void*>(reinterpret_cast<uptr>(addr) + size);
+  }
+  size_ = size_ - size;
+}
+
 void *MmapFixedOrDieOnFatalError(uptr fixed_addr, uptr size) {
   return MmapFixedImpl(fixed_addr, size, true /*tolerate_enomem*/);
 }
 
 bool MprotectNoAccess(uptr addr, uptr size) {
   return 0 == internal_mprotect((void*)addr, size, PROT_NONE);
 }
 

compiler-rt/lib/scudo/scudo_allocator_secondary.h

 class ScudoLargeMmapAllocator {
  public:
 
   void Init() {
     PageSize = GetPageSizeCached();
   }
 
   void *Allocate(AllocatorStats *Stats, uptr Size, uptr Alignment) {
+    ReservedAddressRange address_range;
     uptr UserSize = Size - AlignedChunkHeaderSize;
     // The Scudo frontend prevents us from allocating more than
     // MaxAllowedMallocSize, so integer overflow checks would be superfluous.
-    uptr MapSize = Size + SecondaryHeaderSize;
+    uptr MapSize = Size + ReservedAddressRangeSize;
     if (Alignment > MinAlignment)
       MapSize += Alignment;
     MapSize = RoundUpTo(MapSize, PageSize);
     // Account for 2 guard pages, one before and one after the chunk.
     MapSize += 2 * PageSize;
 
-    uptr MapBeg = reinterpret_cast<uptr>(MmapNoAccess(MapSize));
+    uptr MapBeg = address_range.Init(MapSize);
+    CHECK_EQ(MapBeg, address_range.base());
+    CHECK_EQ(MapSize, address_range.size());
     if (MapBeg == ~static_cast<uptr>(0))
       return ReturnNullOrDieOnFailure::OnOOM();
     // A page-aligned pointer is assumed after that, so check it now.
     CHECK(IsAligned(MapBeg, PageSize));
     uptr MapEnd = MapBeg + MapSize;
     // The beginning of the user area for that allocation comes after the
     // initial guard page, and both headers. This is the pointer that has to
     // abide by alignment requirements.
     uptr UserBeg = MapBeg + PageSize + HeadersSize;
     uptr UserEnd = UserBeg + UserSize;
 
     // In the rare event of larger alignments, we will attempt to fit the mmap
     // area better and unmap extraneous memory. This will also ensure that the
     // offset and unused bytes field of the header stay small.
     if (Alignment > MinAlignment) {
       if (!IsAligned(UserBeg, Alignment)) {
         UserBeg = RoundUpTo(UserBeg, Alignment);
         CHECK_GE(UserBeg, MapBeg);
         uptr NewMapBeg = RoundDownTo(UserBeg - HeadersSize, PageSize) -
             PageSize;
         CHECK_GE(NewMapBeg, MapBeg);
         if (NewMapBeg != MapBeg) {
-          UnmapOrDie(reinterpret_cast<void *>(MapBeg), NewMapBeg - MapBeg);
+          address_range.Unmap(reinterpret_cast<void *>(MapBeg), NewMapBeg - MapBeg);
           MapBeg = NewMapBeg;
+          CHECK_EQ(MapBeg, address_range.base());
         }
         UserEnd = UserBeg + UserSize;
       }
       uptr NewMapEnd = RoundUpTo(UserEnd, PageSize) + PageSize;
       if (NewMapEnd != MapEnd) {
-        UnmapOrDie(reinterpret_cast<void *>(NewMapEnd), MapEnd - NewMapEnd);
+        address_range.Unmap(reinterpret_cast<void *>(NewMapEnd), MapEnd - NewMapEnd);
         MapEnd = NewMapEnd;
       }
       MapSize = MapEnd - MapBeg;
+      CHECK_EQ(MapSize, address_range.size());
     }
 
     CHECK_LE(UserEnd, MapEnd - PageSize);
     // Actually mmap the memory, preserving the guard pages on either side.
-    CHECK_EQ(MapBeg + PageSize, reinterpret_cast<uptr>(
-        MmapFixedOrDie(MapBeg + PageSize, MapSize - 2 * PageSize)));
+    CHECK_EQ(MapBeg + PageSize, address_range.Map(MapBeg + PageSize, MapSize - 2 * PageSize));
     uptr Ptr = UserBeg - AlignedChunkHeaderSize;
-    SecondaryHeader *Header = getHeader(Ptr);
-    Header->MapBeg = MapBeg;
-    Header->MapSize = MapSize;
+    ReservedAddressRange *stored_range = new(reinterpret_cast<void*>(Ptr)) ReservedAddressRange();
+    // TODO should probably be a move
+    Swap(address_range, *stored_range);
+    CHECK_EQ(MapBeg, stored_range->base());
+    CHECK_EQ(MapSize, stored_range->size());
+
     // The primary adds the whole class size to the stats when allocating a
     // chunk, so we will do something similar here. But we will not account for
     // the guard pages.
     {
       SpinMutexLock l(&StatsMutex);
       Stats->Add(AllocatorStatAllocated, MapSize - 2 * PageSize);
       Stats->Add(AllocatorStatMapped, MapSize - 2 * PageSize);
     }
 
     return reinterpret_cast<void *>(Ptr);
   }
 
   void Deallocate(AllocatorStats *Stats, void *Ptr) {
-    SecondaryHeader *Header = getHeader(Ptr);
+    ReservedAddressRange *stored_range = getHeader(Ptr);
     {
       SpinMutexLock l(&StatsMutex);
-      Stats->Sub(AllocatorStatAllocated, Header->MapSize - 2 * PageSize);
-      Stats->Sub(AllocatorStatMapped, Header->MapSize - 2 * PageSize);
+      Stats->Sub(AllocatorStatAllocated, stored_range->size() - 2 * PageSize);
+      Stats->Sub(AllocatorStatMapped, stored_range->size() - 2 * PageSize);
     }
-    UnmapOrDie(reinterpret_cast<void *>(Header->MapBeg), Header->MapSize);
+    UnmapOrDie(reinterpret_cast<void *>(stored_range->base()), stored_range->size());
   }
 
   uptr GetActuallyAllocatedSize(void *Ptr) {
-    SecondaryHeader *Header = getHeader(Ptr);
+    ReservedAddressRange *stored_range = getHeader(Ptr);
     // Deduct PageSize as MapSize includes the trailing guard page.
-    uptr MapEnd = Header->MapBeg + Header->MapSize - PageSize;
+    uptr MapEnd = reinterpret_cast<uptr>(stored_range->base()) + stored_range->size() - PageSize;
     return MapEnd - reinterpret_cast<uptr>(Ptr);
   }
 
  private:
-  // A Secondary allocated chunk header contains the base of the mapping and
-  // its size, which comprises the guard pages.
-  struct SecondaryHeader {
-    uptr MapBeg;
-    uptr MapSize;
-  };
-  // Check that sizeof(SecondaryHeader) is a multiple of MinAlignment.
-  COMPILER_CHECK((sizeof(SecondaryHeader) & (MinAlignment - 1)) == 0);
+  // Check that sizeof(ReservedAddressRange) is a multiple of MinAlignment.
+  COMPILER_CHECK((sizeof(ReservedAddressRange) & (MinAlignment - 1)) == 0);
 
-  SecondaryHeader *getHeader(uptr Ptr) {
-    return reinterpret_cast<SecondaryHeader*>(Ptr - sizeof(SecondaryHeader));
+  ReservedAddressRange *getHeader(uptr Ptr) {
+    return reinterpret_cast<ReservedAddressRange*>(Ptr - sizeof(ReservedAddressRange));
   }
-  SecondaryHeader *getHeader(const void *Ptr) {
+  ReservedAddressRange *getHeader(const void *Ptr) {
     return getHeader(reinterpret_cast<uptr>(Ptr));
   }
 
-  const uptr SecondaryHeaderSize = sizeof(SecondaryHeader);
-  const uptr HeadersSize = SecondaryHeaderSize + AlignedChunkHeaderSize;
+  const uptr ReservedAddressRangeSize = sizeof(ReservedAddressRange);
+  const uptr HeadersSize = ReservedAddressRangeSize + AlignedChunkHeaderSize;
   uptr PageSize;
   SpinMutex StatsMutex;
 };
 
 #endif  // SCUDO_ALLOCATOR_SECONDARY_H_

compiler-rt/lib/scudo/scudo_platform.h

 /// Scudo platform specific definitions.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef SCUDO_PLATFORM_H_
 #define SCUDO_PLATFORM_H_
 
 #include "sanitizer_common/sanitizer_allocator.h"
+#include "sanitizer_common/sanitizer_placement_new.h"
 
 #if !SANITIZER_LINUX && !SANITIZER_FUCHSIA
 # error "The Scudo hardened allocator is not supported on this platform."
 #endif
 
 #if SANITIZER_ANDROID || SANITIZER_FUCHSIA
 // Android and Fuchsia use a pool of TSDs shared between threads.
 # define SCUDO_TSD_EXCLUSIVE 0