[scudo][standalone] Fix Secondary bug w/ freelist

[cryptoad]

Summary

cferris@ found an issue due to the new Secondary free list behavior
and unfortunately it's completely my fault. The issue is twofold:

• I lost track of the (major) fact that the Combined assumes that all chunks returned by the Secondary are zero'd out apprioriately when dealing with ZeroContents. With the introduction of the freelist, it's no longer the case as there can be a small portion of memory between the header and the next page boundary that is left untouched (the rest is zero'd via release). So the next time that block is returned, it's not fully zero'd out.
• There was no test that would exercise that behavior :(

There are several ways to fix this, the one I chose makes the most
sense to me: we pass ZeroContents to the Secondary's allocate
and it zero's out the block if requested and it's coming from the
freelist. The prevents an extraneous memset in case the block
comes from map. Another possbility could have been to memset
in deallocate, but it's probably overzealous as all secondary
blocks don't need to be zero'd out.

Add a test that would have found the issue prior to fix.

[cryptoad]

Minor changes.

[morehouse]

compiler-rt/lib/scudo/standalone/secondary.h

141

We currently release unconditionally in deallocate. Does this zero-out the memory?

If so, we might only need to zero the first page here, not the whole allocation.

[cryptoad]

compiler-rt/lib/scudo/standalone/secondary.h

141

You are correct, we could technically memset only the portion before the first page boundary.
As you also said it's the current as I want to move the release to abide by release_to_os_interval_ms
This would mean that eventually it could not have been released prior to reallocation.
At this point I'd rather not take a shortcut because I feel it's going to come back to me later on.

[morehouse]

compiler-rt/lib/scudo/standalone/secondary.h

141

Sounds good as long as the memset doesn't negate the perf improvemnt from using a free list.

compiler-rt/lib/scudo/standalone/combined.h

Quarantine.drain(&TSD->QuarantineCache,
                      QuarantineCallback(*this, TSD->Cache));
     TSD->Cache.destroy(&Stats);
   }
 
   NOINLINE void *allocate(uptr Size, Chunk::Origin Origin,
                           uptr Alignment = MinAlignment,
                           bool ZeroContents = false) {
     initThreadMaybe();
+    ZeroContents = ZeroContents || Options.ZeroContents;
 
     if (UNLIKELY(Alignment > MaxAlignment)) {
       if (Options.MayReturnNull)
         return nullptr;
       reportAlignmentTooBig(Alignment, MaxAlignment);
     }
     if (Alignment < MinAlignment)
       Alignment = MinAlignment;
if (LIKELY(PrimaryT::canAllocate(NeededSize))) {
       DCHECK_NE(ClassId, 0U);
       bool UnlockRequired;
       auto *TSD = TSDRegistry.getTSDAndLock(&UnlockRequired);
       Block = TSD->Cache.allocate(ClassId);
       if (UnlockRequired)
         TSD->unlock();
     } else {
       ClassId = 0;
-      Block = Secondary.allocate(NeededSize, Alignment, &BlockEnd);
+      Block =
+          Secondary.allocate(NeededSize, Alignment, &BlockEnd, ZeroContents);
     }
 
     if (UNLIKELY(!Block)) {
       if (Options.MayReturnNull)
         return nullptr;
       reportOutOfMemory(NeededSize);
     }
 
     // We only need to zero the contents for Primary backed allocations. This
     // condition is not necessarily unlikely, but since memset is costly, we
     // might as well mark it as such.
-    if (UNLIKELY((ZeroContents || Options.ZeroContents) && ClassId))
+    if (UNLIKELY(ZeroContents && ClassId))
       memset(Block, 0, PrimaryT::getSizeByClassId(ClassId));
 
     Chunk::UnpackedHeader Header = {};
     uptr UserPtr = reinterpret_cast<uptr>(Block) + Chunk::getHeaderSize();
     if (UNLIKELY(!isAligned(UserPtr, Alignment))) {
       const uptr AlignedUserPtr = roundUpTo(UserPtr, Alignment);
       const uptr Offset = AlignedUserPtr - UserPtr;
       DCHECK_GT(Offset, 2 * sizeof(u32));

compiler-rt/lib/scudo/standalone/secondary.h

void initLinkerInitialized(GlobalStats *S) {
     if (LIKELY(S))
       S->link(&Stats);
   }
   void init(GlobalStats *S) {
     memset(this, 0, sizeof(*this));
     initLinkerInitialized(S);
   }
 
-  void *allocate(uptr Size, uptr AlignmentHint = 0, uptr *BlockEnd = nullptr);
+  void *allocate(uptr Size, uptr AlignmentHint = 0, uptr *BlockEnd = nullptr,
+                 bool ZeroContents = false);
 
   void deallocate(void *Ptr);
 
   static uptr getBlockEnd(void *Ptr) {
     return LargeBlock::getHeader(Ptr)->BlockEnd;
   }
 
   static uptr getBlockSize(void *Ptr) {

 // an allocation from the Secondary with a large alignment would end up wasting
 // VA space (even though we are not committing the whole thing), hence the need
 // to trim off some of the reserved space.
 // For allocations requested with an alignment greater than or equal to a page,
 // the committed memory will amount to something close to Size - AlignmentHint
 // (pending rounding and headers).
 template <uptr MaxFreeListSize>
 void *MapAllocator<MaxFreeListSize>::allocate(uptr Size, uptr AlignmentHint,
-                                              uptr *BlockEnd) {
+                                              uptr *BlockEnd,
+                                              bool ZeroContents) {
   DCHECK_GT(Size, AlignmentHint);
   const uptr PageSize = getPageSizeCached();
   const uptr RoundedSize =
       roundUpTo(Size + LargeBlock::getHeaderSize(), PageSize);
 
   if (MaxFreeListSize && AlignmentHint < PageSize) {
     ScopedLock L(Mutex);
     for (auto &H : FreeBlocks) {
       const uptr FreeBlockSize = H.BlockEnd - reinterpret_cast<uptr>(&H);
       if (FreeBlockSize < RoundedSize)
         continue;
       // Candidate free block should only be at most 4 pages larger.
       if (FreeBlockSize > RoundedSize + 4 * PageSize)
         break;
       FreeBlocks.remove(&H);
       InUseBlocks.push_back(&H);
       AllocatedBytes += FreeBlockSize;
       NumberOfAllocs++;
       Stats.add(StatAllocated, FreeBlockSize);
       if (BlockEnd)
         *BlockEnd = H.BlockEnd;
-      return reinterpret_cast<void *>(reinterpret_cast<uptr>(&H) +
+      void *Ptr = reinterpret_cast<void *>(reinterpret_cast<uptr>(&H) +
                                            LargeBlock::getHeaderSize());
+      if (ZeroContents)
+        memset(Ptr, 0, H.BlockEnd - reinterpret_cast<uptr>(Ptr));
+      return Ptr;
     }
   }
 
   MapPlatformData Data = {};
   const uptr MapSize = RoundedSize + 2 * PageSize;
   uptr MapBase =
       reinterpret_cast<uptr>(map(nullptr, MapSize, "scudo:secondary",
                                  MAP_NOACCESS | MAP_ALLOWNOMEM, &Data));

compiler-rt/lib/scudo/standalone/tests/combined_test.cpp

for (scudo::uptr AlignLog = MinAlignLog; AlignLog <= 16U; AlignLog++) {
         EXPECT_LE(Size, Allocator->getUsableSize(P));
         memset(P, 0xaa, Size);
         Allocator->deallocate(P, Origin, Size);
       }
     }
   }
   Allocator->releaseToOS();
 
+  // Ensure that specifying ZeroContents returns a zero'd out block.
+  for (scudo::uptr SizeLog = 0U; SizeLog <= 20U; SizeLog++) {
+    for (scudo::uptr Delta = 0U; Delta <= 4U; Delta++) {
+      const scudo::uptr Size = (1U << SizeLog) + Delta * 128U;
+      void *P = Allocator->allocate(Size, Origin, 1U << MinAlignLog, true);
+      EXPECT_NE(P, nullptr);
+      for (scudo::uptr I = 0; I < Size; I++)
+        EXPECT_EQ((reinterpret_cast<char *>(P))[I], 0);
+      memset(P, 0xaa, Size);
+      Allocator->deallocate(P, Origin, Size);
+    }
+  }
+  Allocator->releaseToOS();
+
   // Verify that a chunk will end up being reused, at some point.
   const scudo::uptr NeedleSize = 1024U;
   void *NeedleP = Allocator->allocate(NeedleSize, Origin);
   Allocator->deallocate(NeedleP, Origin);
   bool Found = false;
   for (scudo::uptr I = 0; I < 1024U && !Found; I++) {
     void *P = Allocator->allocate(NeedleSize, Origin);
     if (P == NeedleP)