[scudo] Fix missing pushing 1 block to BatchClassId

[Chia-hungDuan]

Summary

This may happen when thread is teared down and it only has 1 block of BatchClass left and the freelist of BatchClass is empty. The side effect is that it leaks 1 block of BatchClass

[Chia-hungDuan]

Please ignore this revision, uploaded to the wrong revision number

[Chia-hungDuan]

Revise the logic

[cferris]

Are there already tests for this corner case?

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

217–218

Is this comment not true any more? Otherwise, you are holding the lock while getStats could be called.

[Chia-hungDuan]

Release lock before calling getStats().

Oops, uploaded wrong version

[Chia-hungDuan]

In D149141#4297278, @Chia-hungDuan wrote:

Release lock before calling getStats().

Oops, uploaded wrong version

One thing to add, the problem is not in primary32.

[cferris]

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

210

Shouldn't this be:

PrintStats = Region->Exhausted;

Since you only print the stats when it wasn't previously exhausted and the region is now exhausted?

[Chia-hungDuan]

Rebase and amend missing code which was supposed to be included in last patch

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

210

Per discussion, even though only region exhaustion will return false from populateFreeList(), any failure from populateFreeList() should be viewed as critical in BatchClass region.

BTW, I thought I added a DCHECK(!Region->Exhausted) but it seems that I didn't include it...

[cferris]

LGTM

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

if (ClassId == SizeClassMap::BatchClassId) {
       {
         ScopedLock L(Region->Mutex);
         // Constructing a batch group in the free list will use two blocks in
         // BatchClassId. If we are pushing BatchClassId blocks, we will use the
         // blocks in the array directly (can't delegate local cache which will
         // cause a recursive allocation). However, The number of free blocks may
         // be less than two. Therefore, populate the free list before inserting
         // the blocks.
-        if (Size >= 2U) {
-          pushBlocksImpl(C, SizeClassMap::BatchClassId, Region, Array, Size);
-          Region->Stats.PushedBlocks += Size;
-        } else {
-          const bool RegionIsExhausted = Region->Exhausted;
-          if (UNLIKELY(
-                  RegionIsExhausted ||
-                  !populateFreeList(C, SizeClassMap::BatchClassId, Region))) {
-            PrintStats = !RegionIsExhausted && Region->Exhausted;
-          }
+        const bool NeedToRefill = Size == 1U && Region->FreeList.empty();
+        // If BatchClass has been exhausted, the program should have been
+        // aborted.
+        DCHECK(!Region->Exhausted);
+
+                NeedToRefill &&
+          PrintStats = true;
+        } else {
+          pushBlocksImpl(C, SizeClassMap::BatchClassId, Region, Array, Size);
+          Region->Stats.PushedBlocks += Size;
         }
       }
 
       // Note that `getStats()` requires the lock of each region so we can't
       // call it while locking the Region->Mutex in the above.
       if (UNLIKELY(PrintStats)) {
         ScopedString Str;
         getStats(&Str);
         Str.append(
             "Scudo OOM: The process has exhausted %zuM for size class %zu.\n",
             RegionSize >> 20, getSizeByClassId(ClassId));
         Str.output();
         // Theoretically, BatchClass shouldn't be used up. Abort immediately
         // when it happens.
         reportOutOfBatchClass();
       }
+
       return;
     }
 
     // TODO(chiahungduan): Consider not doing grouping if the group size is not
     // greater than the block size with a certain scale.
 
     // Sort the blocks so that blocks belonging to the same group can be pushed
     // together.