[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 ↗

(On Diff #117866)

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 ↗

(On Diff #117866)

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

compiler-rt/lib/scudo/scudo_allocator_secondary.h

87 ↗

(On Diff #117866)

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

126 ↗

(On Diff #117866)

Might want to rename that API as to getReservedAddressRange.

[flowerhack]

compiler-rt/lib/sanitizer_common/sanitizer_posix.cc

156 ↗

(On Diff #117866)

moved to D38437

[flowerhack]

rebased against patch 2 of this series

feedback much appreciated!

[flowerhack]

compiler-rt/lib/sanitizer_common/sanitizer_common.h

87 ↗

(On Diff #117866)

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

83–85

ALIGNED(MinAlignment) shouldn't be necessary?

108

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

111–112

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

113

Same alignment comment as above.

121–123

This can go away.

129–133

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

107–108

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

[cryptoad]

lib/scudo/scudo_allocator_secondary.h

99–101

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.

lib/sanitizer_common/sanitizer_fuchsia.cc

 
 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);
+  if (addr_as_void == base_) {
+    base_ = reinterpret_cast<void*>(reinterpret_cast<uptr>(addr) + size);
+  }
+  size_ = size_ - 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;

lib/sanitizer_common/sanitizer_posix_libcdep.cc

 
 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);
+  if (addr_as_void == base_) {
+    base_ = reinterpret_cast<void*>(reinterpret_cast<uptr>(addr) + size);
+  }
+  size_ = size_ - size;
 }
 
 void *MmapFixedNoAccess(uptr fixed_addr, uptr size, const char *name) {
   int fd = name ? GetNamedMappingFd(name, size) : -1;
   unsigned flags = MAP_PRIVATE | MAP_FIXED | MAP_NORESERVE;
   if (fd == -1) flags |= MAP_ANON;
 
   return (void *)internal_mmap((void *)fixed_addr, size, PROT_NONE, flags, fd,

lib/sanitizer_common/sanitizer_win.cc

 }
 
 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);
+  if (addr_as_void == base_) {
+    base_ = reinterpret_cast<void*>(reinterpret_cast<uptr>(addr) + size);
+  }
+  size_ = size_ - size;
 }
 
 void *MmapFixedOrDieOnFatalError(uptr fixed_addr, uptr size) {
   void *p = VirtualAlloc((LPVOID)fixed_addr, size,
       MEM_COMMIT, PAGE_READWRITE);
   if (p == 0) {
     char mem_type[30];
     internal_snprintf(mem_type, sizeof(mem_type), "memory at address 0x%zx",

lib/scudo/scudo_allocator_secondary.h

 
 class ScudoLargeMmapAllocator {
  public:
   void Init() {
     PageSize = GetPageSizeCached();
   }
 
   void *Allocate(AllocatorStats *Stats, uptr Size, uptr Alignment) {
+    ReservedAddressRange AddressRange;
     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 + AlignedReservedAddressRangeSize;
     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 = AddressRange.Init(MapSize);
     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);
+          AddressRange.Unmap(MapBeg, NewMapBeg - MapBeg);
           MapBeg = NewMapBeg;
         }
         UserEnd = UserBeg + UserSize;
       }
       uptr NewMapEnd = RoundUpTo(UserEnd, PageSize) + PageSize;
       if (NewMapEnd != MapEnd) {
-        UnmapOrDie(reinterpret_cast<void *>(NewMapEnd), MapEnd - NewMapEnd);
+        AddressRange.Unmap(NewMapEnd, MapEnd - NewMapEnd);
         MapEnd = NewMapEnd;
       }
       MapSize = MapEnd - MapBeg;
     }
 
     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)));
+    // Actually mmap the memory, preserving the guard pages on either side
+    CHECK_EQ(MapBeg + PageSize, AddressRange.Map(MapBeg + PageSize,
+                                                 MapSize - 2 * PageSize));
     uptr Ptr = UserBeg - AlignedChunkHeaderSize;
-    SecondaryHeader *Header = getHeader(Ptr);
-    Header->MapBeg = MapBeg;
-    Header->MapSize = MapSize;
+    ReservedAddressRange *StoredRange = getReservedAddressRange(Ptr);
+    *StoredRange = AddressRange;
+
     // 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);
+    // Since we're unmapping the entirety of where StoredRange actually is,
+    // copy onto the stack, so we don't get AVs while updating base and size.
+    ReservedAddressRange AddressRange = *getReservedAddressRange(Ptr);
     {
       SpinMutexLock l(&StatsMutex);
-      Stats->Sub(AllocatorStatAllocated, Header->MapSize - 2 * PageSize);
-      Stats->Sub(AllocatorStatMapped, Header->MapSize - 2 * PageSize);
+      Stats->Sub(AllocatorStatAllocated, AddressRange.size() - 2 * PageSize);
+      Stats->Sub(AllocatorStatMapped, AddressRange.size() - 2 * PageSize);
     }
-    UnmapOrDie(reinterpret_cast<void *>(Header->MapBeg), Header->MapSize);
+    AddressRange.Unmap(reinterpret_cast<uptr>(AddressRange.base()),
+                       AddressRange.size());
   }
 
   uptr GetActuallyAllocatedSize(void *Ptr) {
-    SecondaryHeader *Header = getHeader(Ptr);
-    // Deduct PageSize as MapSize includes the trailing guard page.
-    uptr MapEnd = Header->MapBeg + Header->MapSize - PageSize;
+    ReservedAddressRange *StoredRange = getReservedAddressRange(Ptr);
+    // Deduct PageSize as ReservedAddressRange size includes the trailing guard
+    // page.
+    uptr MapEnd = reinterpret_cast<uptr>(StoredRange->base()) +
+                  StoredRange->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);
-
-  SecondaryHeader *getHeader(uptr Ptr) {
-    return reinterpret_cast<SecondaryHeader*>(Ptr - sizeof(SecondaryHeader));
-  }
-  SecondaryHeader *getHeader(const void *Ptr) {
-    return getHeader(reinterpret_cast<uptr>(Ptr));
+  ReservedAddressRange *getReservedAddressRange(uptr Ptr) {
+    return reinterpret_cast<ReservedAddressRange*>(Ptr - sizeof(
+                                                   ReservedAddressRange));
+  }
+  ReservedAddressRange *getReservedAddressRange(const void *Ptr) {
+    return getReservedAddressRange(reinterpret_cast<uptr>(Ptr));
   }
 
-  const uptr SecondaryHeaderSize = sizeof(SecondaryHeader);
-  const uptr HeadersSize = SecondaryHeaderSize + AlignedChunkHeaderSize;
+  const uptr AlignedReservedAddressRangeSize =
+    RoundUpTo(sizeof(ReservedAddressRange), MinAlignment);
+  const uptr HeadersSize = AlignedReservedAddressRangeSize +
+                           AlignedChunkHeaderSize;
+
   uptr PageSize;
   SpinMutex StatsMutex;
 };
 
 #endif  // SCUDO_ALLOCATOR_SECONDARY_H_