[scudo] Fix the use of ASSERT_CAPABILITY in TSD #68273

ChiaHungDuan · 2023-10-04T23:38:55Z

In getCache()/getQuarantineCache(), they return a reference to variable guarded by a mutex. After #67776, thread-safey analysis checks if a variable return by reference has the lock held. The ASSERT_CAPABILITY only claims after calling that function, the lock will be held. But not asserting that the lock is held before calling that function.

In the patch, we switch to use REQUIRES() and assertLocked() to mark the code paths. Also remove the misused ASSERT_CAPABILITY.

Fixes #67795, #67796

In getCache()/getQuarantineCache(), they return a reference to variable guarded by a mutex. After llvm#67776, thread-safey analysis checks if a variable return by reference has the lock held. The ASSERT_CAPABILITY only claims after calling that function, the lock will be held. But not asserting that the lock is held *before* calling that function. In the patch, we switch to use REQUIRES() and assertLocked() to mark the code paths. Also remove the misused ASSERT_CAPABILITY.

llvmbot · 2023-10-04T23:39:57Z

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

Changes

In getCache()/getQuarantineCache(), they return a reference to variable guarded by a mutex. After #67776, thread-safey analysis checks if a variable return by reference has the lock held. The ASSERT_CAPABILITY only claims after calling that function, the lock will be held. But not asserting that the lock is held before calling that function.

In the patch, we switch to use REQUIRES() and assertLocked() to mark the code paths. Also remove the misused ASSERT_CAPABILITY.

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

5 Files Affected:

(modified) compiler-rt/lib/scudo/standalone/combined.h (+5)
(modified) compiler-rt/lib/scudo/standalone/tests/combined_test.cpp (+3)
(modified) compiler-rt/lib/scudo/standalone/tests/tsd_test.cpp (+4)
(modified) compiler-rt/lib/scudo/standalone/tsd.h (+11-4)
(modified) compiler-rt/lib/scudo/standalone/tsd_shared.h (+5)

diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index e4ba6ee7886f24d..b013f03a73ae40d 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -245,12 +245,14 @@ class Allocator {
   // - unlinking the local stats from the global ones (destroying the cache does
   //   the last two items).
   void commitBack(TSD<ThisT> *TSD) {
+    TSD->assertLocked(/*BypassCheck=*/true);
     Quarantine.drain(&TSD->getQuarantineCache(),
                      QuarantineCallback(*this, TSD->getCache()));
     TSD->getCache().destroy(&Stats);
   }
 
   void drainCache(TSD<ThisT> *TSD) {
+    TSD->assertLocked(/*BypassCheck=*/true);
     Quarantine.drainAndRecycle(&TSD->getQuarantineCache(),
                                QuarantineCallback(*this, TSD->getCache()));
     TSD->getCache().drain();
@@ -363,6 +365,7 @@ class Allocator {
       DCHECK_NE(ClassId, 0U);
       bool UnlockRequired;
       auto *TSD = TSDRegistry.getTSDAndLock(&UnlockRequired);
+      TSD->assertLocked(/*BypassCheck=*/!UnlockRequired);
       Block = TSD->getCache().allocate(ClassId);
       // If the allocation failed, the most likely reason with a 32-bit primary
       // is the region being full. In that event, retry in each successively
@@ -1147,6 +1150,7 @@ class Allocator {
       if (LIKELY(ClassId)) {
         bool UnlockRequired;
         auto *TSD = TSDRegistry.getTSDAndLock(&UnlockRequired);
+        TSD->assertLocked(/*BypassCheck=*/!UnlockRequired);
         const bool CacheDrained =
             TSD->getCache().deallocate(ClassId, BlockBegin);
         if (UnlockRequired)
@@ -1166,6 +1170,7 @@ class Allocator {
     } else {
       bool UnlockRequired;
       auto *TSD = TSDRegistry.getTSDAndLock(&UnlockRequired);
+      TSD->assertLocked(/*BypassCheck=*/!UnlockRequired);
       Quarantine.put(&TSD->getQuarantineCache(),
                      QuarantineCallback(*this, TSD->getCache()), Ptr, Size);
       if (UnlockRequired)
diff --git a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
index 1143aaab8371d37..7958e9dabf60db2 100644
--- a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
@@ -512,6 +512,7 @@ SCUDO_TYPED_TEST(ScudoCombinedTest, CacheDrain) NO_THREAD_SAFETY_ANALYSIS {
 
   bool UnlockRequired;
   auto *TSD = Allocator->getTSDRegistry()->getTSDAndLock(&UnlockRequired);
+  TSD->assertLocked(/*BypassCheck=*/!UnlockRequired);
   EXPECT_TRUE(!TSD->getCache().isEmpty());
   TSD->getCache().drain();
   EXPECT_TRUE(TSD->getCache().isEmpty());
@@ -536,6 +537,7 @@ SCUDO_TYPED_TEST(ScudoCombinedTest, ForceCacheDrain) NO_THREAD_SAFETY_ANALYSIS {
 
   bool UnlockRequired;
   auto *TSD = Allocator->getTSDRegistry()->getTSDAndLock(&UnlockRequired);
+  TSD->assertLocked(/*BypassCheck=*/!UnlockRequired);
   EXPECT_TRUE(TSD->getCache().isEmpty());
   EXPECT_EQ(TSD->getQuarantineCache().getSize(), 0U);
   EXPECT_TRUE(Allocator->getQuarantine()->isEmpty());
@@ -842,6 +844,7 @@ TEST(ScudoCombinedTest, BasicTrustyConfig) {
 
   bool UnlockRequired;
   auto *TSD = Allocator->getTSDRegistry()->getTSDAndLock(&UnlockRequired);
+  TSD->assertLocked(/*BypassCheck=*/!UnlockRequired);
   TSD->getCache().drain();
 
   Allocator->releaseToOS(scudo::ReleaseToOS::Force);
diff --git a/compiler-rt/lib/scudo/standalone/tests/tsd_test.cpp b/compiler-rt/lib/scudo/standalone/tests/tsd_test.cpp
index 5953d759a69a97e..fad8fcf90077116 100644
--- a/compiler-rt/lib/scudo/standalone/tests/tsd_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/tsd_test.cpp
@@ -101,6 +101,7 @@ template <class AllocatorT> static void testRegistry() {
 
   bool UnlockRequired;
   auto TSD = Registry->getTSDAndLock(&UnlockRequired);
+  TSD->assertLocked(/*BypassCheck=*/!UnlockRequired);
   EXPECT_NE(TSD, nullptr);
   EXPECT_EQ(TSD->getCache().Canary, 0U);
   if (UnlockRequired)
@@ -108,6 +109,7 @@ template <class AllocatorT> static void testRegistry() {
 
   Registry->initThreadMaybe(Allocator.get(), /*MinimalInit=*/false);
   TSD = Registry->getTSDAndLock(&UnlockRequired);
+  TSD->assertLocked(/*BypassCheck=*/!UnlockRequired);
   EXPECT_NE(TSD, nullptr);
   EXPECT_EQ(TSD->getCache().Canary, 0U);
   memset(&TSD->getCache(), 0x42, sizeof(TSD->getCache()));
@@ -137,6 +139,7 @@ template <typename AllocatorT> static void stressCache(AllocatorT *Allocator) {
   Registry->initThreadMaybe(Allocator, /*MinimalInit=*/false);
   bool UnlockRequired;
   auto TSD = Registry->getTSDAndLock(&UnlockRequired);
+  TSD->assertLocked(/*BypassCheck=*/!UnlockRequired);
   EXPECT_NE(TSD, nullptr);
   // For an exclusive TSD, the cache should be empty. We cannot guarantee the
   // same for a shared TSD.
@@ -195,6 +198,7 @@ static void stressSharedRegistry(MockAllocator<SharedCaches> *Allocator) {
   bool UnlockRequired;
   for (scudo::uptr I = 0; I < 4096U; I++) {
     auto TSD = Registry->getTSDAndLock(&UnlockRequired);
+    TSD->assertLocked(/*BypassCheck=*/!UnlockRequired);
     EXPECT_NE(TSD, nullptr);
     Set.insert(reinterpret_cast<void *>(TSD));
     if (UnlockRequired)
diff --git a/compiler-rt/lib/scudo/standalone/tsd.h b/compiler-rt/lib/scudo/standalone/tsd.h
index f4fa545de5e0468..5eb1bc0cc07ec97 100644
--- a/compiler-rt/lib/scudo/standalone/tsd.h
+++ b/compiler-rt/lib/scudo/standalone/tsd.h
@@ -53,10 +53,18 @@ template <class Allocator> struct alignas(SCUDO_CACHE_LINE_SIZE) TSD {
   inline void unlock() NO_THREAD_SAFETY_ANALYSIS { Mutex.unlock(); }
   inline uptr getPrecedence() { return atomic_load_relaxed(&Precedence); }
 
-  void commitBack(Allocator *Instance) ASSERT_CAPABILITY(Mutex) {
+  void commitBack(Allocator *Instance) {
     Instance->commitBack(this);
   }
 
+  // As the comments attached to `getCache()`, the TSD doesn't always need to be
+  // locked. In that case, we would only skip the check before we have all TSDs
+  // locked in all paths.
+  void assertLocked(bool BypassCheck) ASSERT_CAPABILITY(Mutex) {
+    if (SCUDO_DEBUG && !BypassCheck)
+      Mutex.assertHeld();
+  }
+
   // Ideally, we may want to assert that all the operations on
   // Cache/QuarantineCache always have the `Mutex` acquired. However, the
   // current architecture of accessing TSD is not easy to cooperate with the
@@ -66,11 +74,10 @@ template <class Allocator> struct alignas(SCUDO_CACHE_LINE_SIZE) TSD {
   // TODO(chiahungduan): Ideally, we want to do `Mutex.assertHeld` but acquiring
   // TSD doesn't always require holding the lock. Add this assertion while the
   // lock is always acquired.
-  typename Allocator::CacheT &getCache() ASSERT_CAPABILITY(Mutex) {
+  typename Allocator::CacheT &getCache() REQUIRES(Mutex) {
     return Cache;
   }
-  typename Allocator::QuarantineCacheT &getQuarantineCache()
-      ASSERT_CAPABILITY(Mutex) {
+  typename Allocator::QuarantineCacheT &getQuarantineCache() REQUIRES(Mutex) {
     return QuarantineCache;
   }
 
diff --git a/compiler-rt/lib/scudo/standalone/tsd_shared.h b/compiler-rt/lib/scudo/standalone/tsd_shared.h
index dcb0948ad78faa7..80b13eeffc80062 100644
--- a/compiler-rt/lib/scudo/standalone/tsd_shared.h
+++ b/compiler-rt/lib/scudo/standalone/tsd_shared.h
@@ -120,6 +120,11 @@ struct TSDRegistrySharedT {
                 TSDsArraySize);
     for (uptr I = 0; I < NumberOfTSDs; ++I) {
       TSDs[I].lock();
+      // Theoratically, we want to mark TSD::lock()/TSD::unlock() with proper
+      // thread annotations. However, given the TSD is only locked on shared
+      // path, do the assertion in a separate path to avoid confusing the
+      // analyzer.
+      TSDs[I].assertLocked(/*BypassCheck=*/true);
       Str->append("  Shared TSD[%zu]:\n", I);
       TSDs[I].getCache().getStats(Str);
       TSDs[I].unlock();

github-actions · 2023-10-04T23:49:42Z

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

ChiaHungDuan · 2023-10-04T23:53:56Z

See more context in #68072

cferris1000

LGTM.

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

llvm#67776)" Now that `scudo` issues have been fixed (llvm#68273). This reverts commit 462bdd5.

#68394) …. (#67776)" Now that `scudo` issues have been fixed (#68273). This reverts commit 462bdd5.

Local branch amd-gfx e1b9ddb Merged main:4f98fb2e937a into amd-gfx:5c2235853bc1 Remote branch main ea2036e [scudo] Fix the use of ASSERT_CAPABILITY in TSD (llvm#68273)

ChiaHungDuan requested a review from cferris1000 October 4, 2023 23:38

llvmbot added compiler-rt compiler-rt:scudo Scudo Hardened Allocator compiler-rt:sanitizer labels Oct 4, 2023

ChiaHungDuan mentioned this pull request Oct 4, 2023

[scudo] Add missing thread-safety analysis annotations. #68072

Closed

ChiaHungDuan requested a review from legrosbuffle October 4, 2023 23:47

fixup! [scudo] Fix the use of ASSERT_CAPABILITY in TSD

9df9199

cferris1000 approved these changes Oct 5, 2023

View reviewed changes

legrosbuffle reviewed Oct 5, 2023

View reviewed changes

compiler-rt/lib/scudo/standalone/tsd_shared.h Outdated Show resolved Hide resolved

legrosbuffle approved these changes Oct 5, 2023

View reviewed changes

fixup! fixup! [scudo] Fix the use of ASSERT_CAPABILITY in TSD

d963804

ChiaHungDuan merged commit ea2036e into llvm:main Oct 5, 2023
3 checks passed

legrosbuffle added a commit to legrosbuffle/llvm-project that referenced this pull request Oct 6, 2023

Reapply "[clang analysis][thread-safety] Handle return-by-reference... (

6bf1af8

llvm#67776)" Now that `scudo` issues have been fixed (llvm#68273). This reverts commit 462bdd5.

legrosbuffle mentioned this pull request Oct 6, 2023

Reapply "[clang analysis][thread-safety] Handle return-by-reference..… #68394

Merged

legrosbuffle added a commit that referenced this pull request Oct 6, 2023

Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (

cd184c8

#68394) …. (#67776)" Now that `scudo` issues have been fixed (#68273). This reverts commit 462bdd5.

stepthomas mentioned this pull request Oct 10, 2023

AMDGPU stepthomas atomic csub no rtn forms ver2 stepthomas/llvm-project#1

Closed

ChiaHungDuan mentioned this pull request Oct 16, 2023

scudo: fails to build on linux with error: returning variable 'QuarantineCache' by reference requires holding mutex 'Mutex' #67796

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[scudo] Fix the use of ASSERT_CAPABILITY in TSD #68273

[scudo] Fix the use of ASSERT_CAPABILITY in TSD #68273

ChiaHungDuan commented Oct 4, 2023 •

edited

llvmbot commented Oct 4, 2023

github-actions bot commented Oct 4, 2023 •

edited

ChiaHungDuan commented Oct 4, 2023

cferris1000 left a comment

[scudo] Fix the use of ASSERT_CAPABILITY in TSD #68273

[scudo] Fix the use of ASSERT_CAPABILITY in TSD #68273

Conversation

ChiaHungDuan commented Oct 4, 2023 • edited

llvmbot commented Oct 4, 2023

github-actions bot commented Oct 4, 2023 • edited

ChiaHungDuan commented Oct 4, 2023

cferris1000 left a comment

Choose a reason for hiding this comment

ChiaHungDuan commented Oct 4, 2023 •

edited

github-actions bot commented Oct 4, 2023 •

edited