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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad created this revision.
cryptoad added reviewers: cferris, pcc, eugenis, hctim, morehouse.
Herald added subscribers: Sanitizers, jfb.
Herald added projects: Sanitizers, LLVM.
cryptoad marked an inline comment as done.
cryptoad added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/secondary.h:100
           Entries[0].MapSize = H->MapSize;
+          Entries[0].Data = H->Data;
           Entries[0].Time = Time;
----------------
And I forgot to say that the `Data` field was not copied.
In practice this didn't matter since only Fuchsia is using it and it doesn't support caching.


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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74072

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


Index: compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
===================================================================
--- compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
+++ compiler-rt/lib/scudo/standalone/tests/secondary_test.cpp
@@ -137,8 +137,15 @@
     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 @@
 
 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);
Index: compiler-rt/lib/scudo/standalone/secondary.h
===================================================================
--- compiler-rt/lib/scudo/standalone/secondary.h
+++ compiler-rt/lib/scudo/standalone/secondary.h
@@ -97,6 +97,7 @@
           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 @@
       (*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 @@
   }
 
   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 {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D74072.242675.patch
Type: text/x-patch
Size: 3220 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200205/134364f9/attachment-0001.bin>