[compiler-rt] a9d5f89 - [scudo][standalone] Fix a race in the secondary release

[Kostya Kortchinsky via llvm-commits]

Author: Kostya Kortchinsky
Date: 2020-02-05T11:02:51-08:00
New Revision: a9d5f8989d83dee1ed01b7f8eaaab89ad190116a

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

LOG: [scudo][standalone] Fix a race in the secondary release

Summary:
I tried to move the `madvise` calls outside of one of the secondary
mutexes, but this backfired. There is situation when a low release
interval is set combined with secondary pressure that leads to a race:
a thread can get a block from the cache, while another thread is
`madvise`'ing that block, resulting in a null header.

I changed the secondary race test so that this situation would be
triggered, and moved the release into the cache mutex scope.

Reviewers: cferris, pcc, eugenis, hctim, morehouse

Subscribers: jfb, #sanitizers, llvm-commits

Tags: #sanitizers, #llvm

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

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index feb43e78e44a..deba7a930d98 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -97,6 +97,7 @@ class MapAllocatorCache {
           Entries[0].BlockEnd = H->BlockEnd;
           Entries[0].MapBase = H->MapBase;
           Entries[0].MapSize = H->MapSize;
+          Entries[0].Data = H->Data;
           Entries[0].Time = Time;
           EntriesCount++;
           EntryCached = true;
@@ -130,6 +131,7 @@ class MapAllocatorCache {
       (*H)->BlockEnd = Entries[I].BlockEnd;
       (*H)->MapBase = Entries[I].MapBase;
       (*H)->MapSize = Entries[I].MapSize;
+      (*H)->Data = Entries[I].Data;
       EntriesCount--;
       return true;
     }
@@ -174,31 +176,17 @@ class MapAllocatorCache {
   }
 
   void releaseOlderThan(u64 Time) {
-    struct {
-      uptr Block;
-      uptr BlockSize;
-      MapPlatformData Data;
-    } BlockInfo[MaxEntriesCount];
-    uptr N = 0;
-    {
-      ScopedLock L(Mutex);
-      if (!EntriesCount)
-        return;
-      for (uptr I = 0; I < MaxEntriesCount; I++) {
-        if (!Entries[I].Block || !Entries[I].Time)
-          continue;
-        if (Entries[I].Time > Time)
-          continue;
-        BlockInfo[N].Block = Entries[I].Block;
-        BlockInfo[N].BlockSize = Entries[I].BlockEnd - Entries[I].Block;
-        BlockInfo[N].Data = Entries[I].Data;
-        Entries[I].Time = 0;
-        N++;
-      }
+    ScopedLock L(Mutex);
+    if (!EntriesCount)
+      return;
+    for (uptr I = 0; I < MaxEntriesCount; I++) {
+      if (!Entries[I].Block || !Entries[I].Time || Entries[I].Time > Time)
+        continue;
+      releasePagesToOS(Entries[I].Block, 0,
+                       Entries[I].BlockEnd - Entries[I].Block,
+                       &Entries[I].Data);
+      Entries[I].Time = 0;
     }
-    for (uptr I = 0; I < N; I++)
-      releasePagesToOS(BlockInfo[I].Block, 0, BlockInfo[I].BlockSize,
-                       &BlockInfo[I].Data);
   }
 
   struct CachedBlock {

diff  --git a/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp b/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
index c2826e3ddefe..d2260b9c15b0 100644
--- a/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
@@ -137,8 +137,15 @@ static void performAllocations(LargeAllocator *L) {
     while (!Ready)
       Cv.wait(Lock);
   }
-  for (scudo::uptr I = 0; I < 32U; I++)
-    V.push_back(L->allocate((std::rand() % 16) * PageSize));
+  for (scudo::uptr I = 0; I < 128U; I++) {
+    // Deallocate 75% of the blocks.
+    const bool Deallocate = (rand() & 3) != 0;
+    void *P = L->allocate((std::rand() % 16) * PageSize);
+    if (Deallocate)
+      L->deallocate(P);
+    else
+      V.push_back(P);
+  }
   while (!V.empty()) {
     L->deallocate(V.back());
     V.pop_back();
@@ -147,9 +154,9 @@ static void performAllocations(LargeAllocator *L) {
 
 TEST(ScudoSecondaryTest, SecondaryThreadsRace) {
   LargeAllocator *L = new LargeAllocator;
-  L->init(nullptr);
-  std::thread Threads[10];
-  for (scudo::uptr I = 0; I < 10U; I++)
+  L->init(nullptr, /*ReleaseToOsInterval=*/0);
+  std::thread Threads[16];
+  for (scudo::uptr I = 0; I < ARRAY_SIZE(Threads); I++)
     Threads[I] = std::thread(performAllocations, L);
   {
     std::unique_lock<std::mutex> Lock(Mutex);