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

[cryptoad]

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.

[cryptoad]

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

100

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.

[merge_guards_bot]

Unit tests: pass. 62503 tests passed, 0 failed and 844 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[pcc]

LGTM

[cryptoad]

Retrying clang-format.

[merge_guards_bot]

Unit tests: pass. 62503 tests passed, 0 failed and 844 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

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

const u64 Time = getMonotonicTime();
         for (uptr I = 0; I < MaxEntriesCount; I++) {
           if (Entries[I].Block)
             continue;
           if (I != 0)
             Entries[I] = Entries[0];
           Entries[0].Block = reinterpret_cast<uptr>(H);
           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;
           break;
         }
       }
     }
     if (EmptyCache)
for (uptr I = 0; I < MaxEntriesCount; I++) {
       const uptr BlockSize = Entries[I].BlockEnd - Entries[I].Block;
       if (Size > BlockSize)
         continue;
       if (Size < BlockSize - PageSize * 4U)
         continue;
       *H = reinterpret_cast<LargeBlock::Header *>(Entries[I].Block);
       Entries[I].Block = 0;
       (*H)->BlockEnd = Entries[I].BlockEnd;
       (*H)->MapBase = Entries[I].MapBase;
       (*H)->MapSize = Entries[I].MapSize;
+      (*H)->Data = Entries[I].Data;
       EntriesCount--;
       return true;
     }
     return false;
   }
 
   static bool canCache(uptr Size) {
     return MaxEntriesCount != 0U && Size <= MaxEntrySize;
uptr N = 0;
       }
       EntriesCount = 0;
       IsFullEvents = 0;
     }
     for (uptr I = 0; I < N; I++)
       unmap(MapInfo[I].MapBase, MapInfo[I].MapSize, UNMAP_ALL,
             &MapInfo[I].Data);
   }
 
   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++;
-      }
-    }
-    for (uptr I = 0; I < N; I++)
-      releasePagesToOS(BlockInfo[I].Block, 0, BlockInfo[I].BlockSize,
-                       &BlockInfo[I].Data);
-  }
+      if (!Entries[I].Block || !Entries[I].Time || Entries[I].Time > Time)
+      releasePagesToOS(Entries[I].Block, 0,
+                       Entries[I].BlockEnd - Entries[I].Block,
+                       &Entries[I].Data);
 
   struct CachedBlock {
     uptr Block;
     uptr BlockEnd;
     uptr MapBase;
     uptr MapSize;
     MapPlatformData Data;
     u64 Time;
   };
 
   HybridMutex Mutex;
   CachedBlock Entries[MaxEntriesCount];
   u32 EntriesCount;
   uptr LargestSize;
   u32 IsFullEvents;
   s32 ReleaseToOsIntervalMs;
 };

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

 static void performAllocations(LargeAllocator *L) {
   std::vector<void *> V;
   const scudo::uptr PageSize = scudo::getPageSizeCached();
   {
     std::unique_lock<std::mutex> Lock(Mutex);
     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();
   }
 }
 
 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);
     Ready = true;
     Cv.notify_all();
   }
   for (auto &T : Threads)
     T.join();
   scudo::ScopedString Str(1024);
   L->getStats(&Str);
   Str.output();
 }