[scudo] Apply filling option when realloc grows a block in-place too

[fabio-d]

No description provided.

[llvmbot]

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

Author: Fabio D'Urso (fabio-d)

Changes

---

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

2 Files Affected:

• (modified) compiler-rt/lib/scudo/standalone/combined.h (+13)
• (modified) compiler-rt/lib/scudo/standalone/tests/combined_test.cpp (+21)

diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index 15a199ae0349b..60c7fae1d57d2 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -565,6 +565,19 @@ class Allocator {
             storeSecondaryAllocationStackMaybe(Options, OldPtr, NewSize);
           }
         }
+
+        // If we've increased the size, fill the extra bytes.
+        if (NewSize > OldSize) {
+          const FillContentsMode FillContents =
+              TSDRegistry.getDisableMemInit() ? NoFill
+                                              : Options.getFillContentsMode();
+          if (FillContents != NoFill) {
+            memset(reinterpret_cast<char *>(OldTaggedPtr) + OldSize,
+                   FillContents == ZeroFill ? 0 : PatternFillByte,
+                   NewSize - OldSize);
+          }
+        }
+
         return OldTaggedPtr;
       }
     }
diff --git a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
index 1a36155bcd423..4af0d44493b2a 100644
--- a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
@@ -347,7 +347,17 @@ SCUDO_TYPED_TEST(ScudoCombinedTest, ZeroFill) {
       EXPECT_NE(P, nullptr);
       for (scudo::uptr I = 0; I < Size; I++)
         ASSERT_EQ((reinterpret_cast<char *>(P))[I], '\0');
+
+      // Fill with a non-zero pattern.
       memset(P, 0xaa, Size);
+
+      // Shrink and then grow by one byte, verifying that it gets re-filled in
+      // the process. We assume that changing the size by just 1 is done in
+      // place.
+      ASSERT_EQ(Allocator->reallocate(P, Size - 1), P);
+      ASSERT_EQ(Allocator->reallocate(P, Size), P);
+      EXPECT_EQ((reinterpret_cast<unsigned char *>(P))[Size - 1], '\0');
+
       Allocator->deallocate(P, Origin, Size);
     }
   }
@@ -374,7 +384,18 @@ SCUDO_TYPED_TEST(ScudoCombinedTest, PatternOrZeroFill) {
         else
           ASSERT_TRUE(V == scudo::PatternFillByte || V == 0);
       }
+
+      // Fill with a known pattern different from PatternFillByte.
       memset(P, 0xaa, Size);
+
+      // Shrink and then grow by one byte, verifying that it gets re-filled in
+      // the process. We assume that changing the size by just 1 is done in
+      // place.
+      ASSERT_EQ(Allocator->reallocate(P, Size - 1), P);
+      ASSERT_EQ(Allocator->reallocate(P, Size), P);
+      EXPECT_EQ((reinterpret_cast<unsigned char *>(P))[Size - 1],
+                scudo::PatternFillByte);
+
       Allocator->deallocate(P, Origin, Size);
     }
   }

[ChiaHungDuan]

I think we shouldn't make the assumption that "changing the size by just 1 is done in place" in the test.

Given that this has no immediate impact and is not harmful if we don't test it. I think it's fine to leave it untested.

[ChiaHungDuan]

If you really want to test this, I would suggest having a dedicated config to ensure that the shrinking and expending use the same block

[fabio-d]

I'm seeing that there's already a similar assumption in ScudoCombinedDeathTest::ReallocateSame, so another possibility could maybe be to extend that test instead of having to maintain a new config. What about this?

 SCUDO_TYPED_TEST(ScudoCombinedDeathTest, ReallocateSame) {
   auto *Allocator = this->Allocator.get();
 
   // Check that reallocating a chunk to a slightly smaller or larger size
   // returns the same chunk. This requires that all the sizes we iterate on use
   // the same block size, but that should be the case for MaxSize - 64 with our
   // default class size maps.
   constexpr scudo::uptr ReallocSize =
       TypeParam::Primary::SizeClassMap::MaxSize - 64;
   void *P = Allocator->allocate(ReallocSize, Origin);
   const char Marker = 'A';
   memset(P, Marker, ReallocSize);
+  Allocator->setFillContents(scudo::PatternOrZeroFill);
   for (scudo::sptr Delta = -32; Delta < 32; Delta += 8) {
     const scudo::uptr NewSize =
         static_cast<scudo::uptr>(static_cast<scudo::sptr>(ReallocSize) + Delta);
     void *NewP = Allocator->reallocate(P, NewSize);
     EXPECT_EQ(NewP, P);

+    // Verify that existing contents have been preserved.
     for (scudo::uptr I = 0; I < ReallocSize - 32; I++)
       EXPECT_EQ((reinterpret_cast<char *>(NewP))[I], Marker);
+
+    // Verify that, if we have grown the allocation, new bytes have been set
+    // according to FillContentsMode.
+    for (scudo::uptr I = ReallocSize - 32; I < NewSize; I++)
+      EXPECT_EQ((reinterpret_cast<unsigned char *>(NewP))[I],
+                scudo::PatternFillByte);

     checkMemoryTaggingMaybe(Allocator, NewP, NewSize, 0);
   }
   Allocator->deallocate(P, Origin);
 }

[ChiaHungDuan]

Right, I think we can utilize this test. BTW, it seems to me that the test has some problem. ReallocSize - 32 only checks the constant range of memory with marker filled. We may want to do it like,

for (scudo::sptr Delta = -32; Delta < 32; Delta += 8) {
  // 1. memset(P, Marker, CurrentSize);
  // 2. Calculate NewSize and do the realloc
  // 3. Check the content needed to be preserved
  // 4. Check the in-place growing does fill the extended bytes
  }

[fabio-d]

Thanks! I've modified the test accordingly, LMK

[fabio-d]

Reverted in bddd8ea because it broke buildbot https://lab.llvm.org/buildbot/#/builders/169/builds/32309

[ChiaHungDuan]

I guess we only want to include this when it doesn't have GWP_ASAN_HOOKS defined.