[compiler-rt] c7bc3db - [scudo][standalone] Fix Secondary bug w/ freelist

[Kostya Kortchinsky via llvm-commits]

Author: Kostya Kortchinsky
Date: 2019-10-31T14:38:30-07:00
New Revision: c7bc3db23cafee2b51c43bfbe2c02f61cf115721

URL: https://github.com/llvm/llvm-project/commit/c7bc3db23cafee2b51c43bfbe2c02f61cf115721
DIFF: https://github.com/llvm/llvm-project/commit/c7bc3db23cafee2b51c43bfbe2c02f61cf115721.diff

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

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.

Reviewers: morehouse, hctim, cferris, pcc, eugenis, vitalybuka

Subscribers: #sanitizers, llvm-commits

Tags: #sanitizers, #llvm

Differential Revision: https://reviews.llvm.org/D69675

Added: 
    

Modified: 
    compiler-rt/lib/scudo/standalone/combined.h
    compiler-rt/lib/scudo/standalone/secondary.h
    compiler-rt/lib/scudo/standalone/tests/combined_test.cpp

Removed: 
    


################################################################################
diff  --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index dc9c8be342a5..2f8d82b58db3 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -161,6 +161,7 @@ template <class Params> class Allocator {
                           uptr Alignment = MinAlignment,
                           bool ZeroContents = false) {
     initThreadMaybe();
+    ZeroContents = ZeroContents || Options.ZeroContents;
 
     if (UNLIKELY(Alignment > MaxAlignment)) {
       if (Options.MayReturnNull)
@@ -200,7 +201,8 @@ template <class Params> class Allocator {
         TSD->unlock();
     } else {
       ClassId = 0;
-      Block = Secondary.allocate(NeededSize, Alignment, &BlockEnd);
+      Block =
+          Secondary.allocate(NeededSize, Alignment, &BlockEnd, ZeroContents);
     }
 
     if (UNLIKELY(!Block)) {
@@ -212,7 +214,7 @@ template <class Params> class Allocator {
     // 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 = {};

diff  --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index 38e2d958dddc..bca783a3b602 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -60,7 +60,8 @@ template <uptr MaxFreeListSize = 32U> class MapAllocator {
     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);
 
@@ -111,7 +112,8 @@ template <uptr MaxFreeListSize = 32U> class MapAllocator {
 // (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 =
@@ -133,8 +135,11 @@ void *MapAllocator<MaxFreeListSize>::allocate(uptr Size, uptr AlignmentHint,
       Stats.add(StatAllocated, FreeBlockSize);
       if (BlockEnd)
         *BlockEnd = H.BlockEnd;
-      return reinterpret_cast<void *>(reinterpret_cast<uptr>(&H) +
-                                      LargeBlock::getHeaderSize());
+      void *Ptr = reinterpret_cast<void *>(reinterpret_cast<uptr>(&H) +
+                                           LargeBlock::getHeaderSize());
+      if (ZeroContents)
+        memset(Ptr, 0, H.BlockEnd - reinterpret_cast<uptr>(Ptr));
+      return Ptr;
     }
   }
 

diff  --git a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
index e12d58392520..d32ea89e0ea3 100644
--- a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
@@ -65,6 +65,20 @@ template <class Config> static void testAllocator() {
   }
   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);