[compiler-rt] Double free (PR #110345)

[Christopher Ferris via llvm-commits]

https://github.com/cferris1000 created https://github.com/llvm/llvm-project/pull/110345

None

>From 662d648e247e0b9e06b963de0cca0c749f953f7f Mon Sep 17 00:00:00 2001
From: Christopher Ferris <cferris at google.com>
Date: Fri, 27 Sep 2024 16:04:17 -0700
Subject: [PATCH 1/2] [scudo] Double frees result in chunk state error

Fixes bug where a device that supports tagged pointers doesn't use
the tagged pointer when computing the checksum.

Add tests to verify that double frees result in chunk state error
not corrupted header errors.
---
 compiler-rt/lib/scudo/standalone/combined.h   | 23 +++++++++++--------
 .../scudo/standalone/tests/combined_test.cpp  | 21 +++++++++++++++++
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index a5f1bc388e8824..be7a1b00a1bd9b 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -1252,22 +1252,25 @@ class Allocator {
     else
       Header->State = Chunk::State::Quarantined;
 
-    void *BlockBegin;
-    if (LIKELY(!useMemoryTagging<AllocatorConfig>(Options))) {
+    if (LIKELY(!useMemoryTagging<AllocatorConfig>(Options)))
       Header->OriginOrWasZeroed = 0U;
-      if (BypassQuarantine && allocatorSupportsMemoryTagging<AllocatorConfig>())
-        Ptr = untagPointer(Ptr);
-      BlockBegin = getBlockBegin(Ptr, Header);
-    } else {
+    else
       Header->OriginOrWasZeroed =
           Header->ClassId && !TSDRegistry.getDisableMemInit();
-      BlockBegin =
-          retagBlock(Options, TaggedPtr, Ptr, Header, Size, BypassQuarantine);
-    }
 
     Chunk::storeHeader(Cookie, Ptr, Header);
 
     if (BypassQuarantine) {
+      // Must do this after storeHeader because loadHeader uses a tagged ptr.
+      void *BlockBegin;
+      if (LIKELY(!useMemoryTagging<AllocatorConfig>(Options))) {
+        if (allocatorSupportsMemoryTagging<AllocatorConfig>())
+          Ptr = untagPointer(Ptr);
+        BlockBegin = getBlockBegin(Ptr, Header);
+      } else {
+        BlockBegin = retagBlock(Options, TaggedPtr, Ptr, Header, Size, true);
+      }
+
       const uptr ClassId = Header->ClassId;
       if (LIKELY(ClassId)) {
         bool CacheDrained;
@@ -1285,6 +1288,8 @@ class Allocator {
         Secondary.deallocate(Options, BlockBegin);
       }
     } else {
+      if (UNLIKELY(useMemoryTagging<AllocatorConfig>(Options)))
+        retagBlock(Options, TaggedPtr, Ptr, Header, Size, false);
       typename TSDRegistryT::ScopedTSD TSD(TSDRegistry);
       Quarantine.put(&TSD->getQuarantineCache(),
                      QuarantineCallback(*this, TSD->getCache()), Ptr, Size);
diff --git a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
index 16b19e807e11bc..ff98eb3397ee0e 100644
--- a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
@@ -534,6 +534,27 @@ SCUDO_TYPED_TEST(ScudoCombinedDeathTest, UseAfterFree) {
   }
 }
 
+SCUDO_TYPED_TEST(ScudoCombinedDeathTest, DoubleFreeFromPrimary) {
+  auto *Allocator = this->Allocator.get();
+
+  for (scudo::uptr SizeLog = 0U; SizeLog <= 20U; SizeLog++) {
+    const scudo::uptr Size = 1U << SizeLog;
+    if (!isPrimaryAllocation<TestAllocator<TypeParam>>(Size, 0))
+      break;
+
+    // Verify that a double free results in a chunk state error.
+    EXPECT_DEATH(
+        {
+          // Allocate from primary
+          void *P = Allocator->allocate(Size, Origin);
+          ASSERT_TRUE(P != nullptr);
+          Allocator->deallocate(P, Origin);
+          Allocator->deallocate(P, Origin);
+        },
+        "invalid chunk state");
+  }
+}
+
 SCUDO_TYPED_TEST(ScudoCombinedDeathTest, DisableMemoryTagging) {
   auto *Allocator = this->Allocator.get();
 

>From b264a8827e659f9f930bcbbf75db5a71e02200d7 Mon Sep 17 00:00:00 2001
From: Christopher Ferris <cferris at google.com>
Date: Fri, 27 Sep 2024 17:15:35 -0700
Subject: [PATCH 2/2] Move comment closer to usage.

---
 compiler-rt/lib/scudo/standalone/combined.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index be7a1b00a1bd9b..2e6867c6bafcb0 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -1261,9 +1261,9 @@ class Allocator {
     Chunk::storeHeader(Cookie, Ptr, Header);
 
     if (BypassQuarantine) {
-      // Must do this after storeHeader because loadHeader uses a tagged ptr.
       void *BlockBegin;
       if (LIKELY(!useMemoryTagging<AllocatorConfig>(Options))) {
+        // Must do this after storeHeader because loadHeader uses a tagged ptr.
         if (allocatorSupportsMemoryTagging<AllocatorConfig>())
           Ptr = untagPointer(Ptr);
         BlockBegin = getBlockBegin(Ptr, Header);