[PATCH] D66231: [scudo][standalone] Fix malloc_iterate

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad created this revision.
cryptoad added reviewers: cferris, morehouse, hctim, vitalybuka, eugenis.
Herald added subscribers: Sanitizers, delcypher.
Herald added projects: LLVM, Sanitizers.

cferris's Bionic tests found an issue in Scudo's `malloc_iterate`.

We were inclusive of both boundaries, which resulted in a `Block` that
was located on said boundary to be possibly accounted for twice, or
just being accounted for while iterating on regions that are not ours
(usually the unmapped ones in between Primary regions).

The fix is to exclude the upper boundary in `iterateOverChunks`, and
add a regression test.

This additionally corrects a typo in a comment, and change the 64-bit
Primary iteration function to not assume that `BatchClassId` is 0.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D66231

Files:
  lib/scudo/standalone/combined.h
  lib/scudo/standalone/primary64.h
  lib/scudo/standalone/tests/wrappers_c_test.cpp


Index: lib/scudo/standalone/tests/wrappers_c_test.cpp
===================================================================
--- lib/scudo/standalone/tests/wrappers_c_test.cpp
+++ lib/scudo/standalone/tests/wrappers_c_test.cpp
@@ -14,6 +14,14 @@
 #include <malloc.h>
 #include <unistd.h>
 
+extern "C" {
+void malloc_enable(void);
+void malloc_disable(void);
+int malloc_iterate(uintptr_t base, size_t size,
+                   void (*callback)(uintptr_t base, size_t size, void *arg),
+                   void *arg);
+}
+
 // Note that every C allocation function in the test binary will be fulfilled
 // by Scudo (this includes the gtest APIs, etc.), which is a test by itself.
 // But this might also lead to unexpected side-effects, since the allocation and
@@ -239,3 +247,43 @@
   MI = mallinfo();
   EXPECT_GE(static_cast<size_t>(MI.fordblks), Free + BypassQuarantineSize);
 }
+
+static uintptr_t BoundaryP = 0U;
+static size_t Counter = 0U;
+
+static void callback(uintptr_t Base, size_t Size, void *Arg) {
+  if (Base == BoundaryP)
+    Counter++;
+}
+
+// Verify that a block located on an iteration boundary is not mis-accounted.
+TEST(ScudoWrappersCTest, MallocIterateBoundary) {
+  const size_t BlockDelta = FIRST_32_SECOND_64(8U, 16U);
+  const size_t SpecialSize = 128U - BlockDelta;
+  const size_t PageSize = sysconf(_SC_PAGESIZE);
+
+  std::vector<void *> V;
+  while (V.size() < 512U) {
+    void *P = malloc(SpecialSize);
+    const uintptr_t Block = reinterpret_cast<uintptr_t>(P) - BlockDelta;
+    if ((Block & (PageSize - 1)) == 0) {
+      BoundaryP = reinterpret_cast<uintptr_t>(P);
+      break;
+    }
+    V.push_back(P);
+  }
+  EXPECT_NE(BoundaryP, 0U);
+
+  const uintptr_t Boundary = BoundaryP - BlockDelta;
+  malloc_disable();
+  malloc_iterate(Boundary - PageSize, PageSize, callback, nullptr);
+  malloc_iterate(Boundary, PageSize, callback, nullptr);
+  malloc_enable();
+  EXPECT_EQ(Counter, 1U);
+
+  while (!V.empty()) {
+    free(V.back());
+    V.pop_back();
+  }
+  free(reinterpret_cast<void *>(BoundaryP));
+}
Index: lib/scudo/standalone/primary64.h
===================================================================
--- lib/scudo/standalone/primary64.h
+++ lib/scudo/standalone/primary64.h
@@ -36,7 +36,7 @@
 // freelist to the thread specific freelist, and back.
 //
 // The memory used by this allocator is never unmapped, but can be partially
-// released it the platform allows for it.
+// released if the platform allows for it.
 
 template <class SizeClassMapT, uptr RegionSizeLog> class SizeClassAllocator64 {
 public:
@@ -135,7 +135,9 @@
   }
 
   template <typename F> void iterateOverBlocks(F Callback) const {
-    for (uptr I = 1; I < NumClasses; I++) {
+    for (uptr I = 0; I < NumClasses; I++) {
+      if (I == SizeClassMap::BatchClassId)
+        continue;
       const RegionInfo *Region = getRegionInfo(I);
       const uptr BlockSize = getSizeByClassId(I);
       const uptr From = Region->RegionBeg;
Index: lib/scudo/standalone/combined.h
===================================================================
--- lib/scudo/standalone/combined.h
+++ lib/scudo/standalone/combined.h
@@ -375,7 +375,7 @@
     const uptr From = Base;
     const uptr To = Base + Size;
     auto Lambda = [this, From, To, Callback, Arg](uptr Block) {
-      if (Block < From || Block > To)
+      if (Block < From || Block >= To)
         return;
       uptr ChunkSize;
       const uptr ChunkBase = getChunkFromBlock(Block, &ChunkSize);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D66231.215161.patch
Type: text/x-patch
Size: 3479 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190814/9b905b4e/attachment.bin>