[scudo] Always zero on linux even if the memory cannot be released.

[cferris1000]

If a caller has locked memory, then the madvise call will fail. In that case, zero the memory so that we don't return non-zeroed memory for calloc calls since we thought the memory had been released.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Christopher Ferris (cferris1000)

Changes

If a caller has locked memory, then the madvise call will fail. In that case, zero the memory so that we don't return non-zeroed memory for calloc calls since we thought the memory had been released.

---

Full diff: https://github.com/llvm/llvm-project/pull/167788.diff

2 Files Affected:

• (modified) compiler-rt/lib/scudo/standalone/mem_map_linux.cpp (+6-1)
• (modified) compiler-rt/lib/scudo/standalone/tests/map_test.cpp (+41)

diff --git a/compiler-rt/lib/scudo/standalone/mem_map_linux.cpp b/compiler-rt/lib/scudo/standalone/mem_map_linux.cpp
index 783c4f0d9ab0f..df3e54cab6695 100644
--- a/compiler-rt/lib/scudo/standalone/mem_map_linux.cpp
+++ b/compiler-rt/lib/scudo/standalone/mem_map_linux.cpp
@@ -122,7 +122,12 @@ void MemMapLinux::setMemoryPermissionImpl(uptr Addr, uptr Size, uptr Flags) {
 void MemMapLinux::releaseAndZeroPagesToOSImpl(uptr From, uptr Size) {
   void *Addr = reinterpret_cast<void *>(From);
 
-  while (madvise(Addr, Size, MADV_DONTNEED) == -1 && errno == EAGAIN) {
+  int rc;
+  while ((rc = madvise(Addr, Size, MADV_DONTNEED)) == -1 && errno == EAGAIN) {
+  }
+  if (rc == -1) {
+    // If we can't madvies the memory, then we still need to zero it.
+    memset(Addr, 0, Size);
   }
 }
 
diff --git a/compiler-rt/lib/scudo/standalone/tests/map_test.cpp b/compiler-rt/lib/scudo/standalone/tests/map_test.cpp
index cc7d3ee4dc6c2..fadc0e00c7844 100644
--- a/compiler-rt/lib/scudo/standalone/tests/map_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/map_test.cpp
@@ -14,6 +14,10 @@
 #include <string.h>
 #include <unistd.h>
 
+#if SCUDO_LINUX
+#include <sys/mman.h>
+#endif
+
 static const char *MappingName = "scudo:test";
 
 TEST(ScudoMapTest, PageSize) {
@@ -89,3 +93,40 @@ TEST(ScudoMapTest, MapGrowUnmap) {
   memset(reinterpret_cast<void *>(Q), 0xbb, PageSize);
   MemMap.unmap();
 }
+
+// Verify that zeroing works properly.
+TEST(ScudoMapTest, Zeroing) {
+  scudo::ReservedMemoryT ReservedMemory;
+  const scudo::uptr PageSize = scudo::getPageSizeCached();
+  const scudo::uptr Size = 3 * PageSize;
+  ReservedMemory.create(/*Addr=*/0U, Size, MappingName);
+  ASSERT_TRUE(ReservedMemory.isCreated());
+
+  scudo::MemMapT MemMap = ReservedMemory.dispatch(ReservedMemory.getBase(), ReservedMemory.getCapacity());
+  EXPECT_TRUE(MemMap.remap(MemMap.getBase(), MemMap.getCapacity(), MappingName));
+  unsigned char *Data = reinterpret_cast<unsigned char*>(MemMap.getBase());
+  memset(Data, 1U, MemMap.getCapacity());
+  // Spot check some values.
+  EXPECT_EQ(1U, Data[0]);
+  EXPECT_EQ(1U, Data[PageSize]);
+  EXPECT_EQ(1U, Data[PageSize * 2]);
+  MemMap.releaseAndZeroPagesToOS(MemMap.getBase(), MemMap.getCapacity());
+  EXPECT_EQ(0U, Data[0]);
+  EXPECT_EQ(0U, Data[PageSize]);
+  EXPECT_EQ(0U, Data[PageSize * 2]);
+
+#if SCUDO_LINUX
+  // Now verify that if madvise fails, the data is still zeroed.
+  memset(Data, 1U, MemMap.getCapacity());
+  EXPECT_NE(-1, mlock(Data, MemMap.getCapacity()));
+  EXPECT_EQ(1U, Data[0]);
+  EXPECT_EQ(1U, Data[PageSize]);
+  EXPECT_EQ(1U, Data[PageSize * 2]);
+  MemMap.releaseAndZeroPagesToOS(MemMap.getBase(), MemMap.getCapacity());
+  EXPECT_EQ(0U, Data[0]);
+  EXPECT_EQ(0U, Data[PageSize]);
+  EXPECT_EQ(0U, Data[PageSize * 2]);
+#endif
+
+  MemMap.unmap();
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ChiaHungDuan]

I was thinking if we want to check something like "errno == EPERM". It seems not making any difference to the problem but maybe it helps with some other cases. I guess not...

[cferris1000]

Yeah, when I checked this the errno was EINVAL, which is not what I was expecting. It seems safer to just memset when it fails for whatever reason.