Skip to content

Conversation

@cferris1000
Copy link
Contributor

The current code always unmaps a secondary allocation when MTE is enabled. Fix this to match the comment, namely only unmap if MTE was enabled and is no longer enabled after acquiring the lock.

In addition, allow quaratine to work in the secondary even if MTE is not enabled.

The current code always unmaps a secondary allocation when MTE is
enabled. Fix this to match the comment, namely only unmap if
MTE was enabled and is no longer enabled after acquiring the lock.

In addition, allow quaratine to work in the secondary even if MTE
is not enabled.
@llvmbot
Copy link
Member

llvmbot commented Jul 23, 2025

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

Author: Christopher Ferris (cferris1000)

Changes

The current code always unmaps a secondary allocation when MTE is enabled. Fix this to match the comment, namely only unmap if MTE was enabled and is no longer enabled after acquiring the lock.

In addition, allow quaratine to work in the secondary even if MTE is not enabled.


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

1 Files Affected:

  • (modified) compiler-rt/lib/scudo/standalone/secondary.h (+7-4)
diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index 286e5d332f57c..f04c5b7cfc2ea 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -269,7 +269,8 @@ class MapAllocatorCache {
     Entry.MemMap = MemMap;
     Entry.Time = UINT64_MAX;
 
-    if (useMemoryTagging<Config>(Options)) {
+    bool MemoryTaggingEnabled = useMemoryTagging<Config>(Options);
+    if (MemoryTaggingEnabled) {
       if (Interval == 0 && !SCUDO_FUCHSIA) {
         // Release the memory and make it inaccessible at the same time by
         // creating a new MAP_NOACCESS mapping on top of the existing mapping.
@@ -302,7 +303,7 @@ class MapAllocatorCache {
       if (Entry.Time != 0)
         Entry.Time = Time;
 
-      if (useMemoryTagging<Config>(Options) && QuarantinePos == -1U) {
+      if (MemoryTaggingEnabled && !useMemoryTagging<Config>(Options)) {
         // If we get here then memory tagging was disabled in between when we
         // read Options and when we locked Mutex. We can't insert our entry into
         // the quarantine or the cache because the permissions would be wrong so
@@ -310,7 +311,8 @@ class MapAllocatorCache {
         unmapCallBack(Entry.MemMap);
         break;
       }
-      if (Config::getQuarantineSize() && useMemoryTagging<Config>(Options)) {
+
+      if (Config::getQuarantineSize()) {
         QuarantinePos =
             (QuarantinePos + 1) % Max(Config::getQuarantineSize(), 1u);
         if (!Quarantine[QuarantinePos].isValid()) {
@@ -513,9 +515,10 @@ class MapAllocatorCache {
         Quarantine[I].invalidate();
       }
     }
+    QuarantinePos = -1U;
+
     for (CachedBlock &Entry : LRUEntries)
       Entry.MemMap.setMemoryPermission(Entry.CommitBase, Entry.CommitSize, 0);
-    QuarantinePos = -1U;
   }
 
   void disable() NO_THREAD_SAFETY_ANALYSIS { Mutex.lock(); }

@cferris1000
Copy link
Contributor Author

This fixes the issue from the other change.

@cferris1000
Copy link
Contributor Author

I ran this through tests on Android and no issues. Both with MTE and without.

I also ran the tests on 32 bit.

@cferris1000 cferris1000 merged commit 30532c1 into llvm:main Jul 28, 2025
13 checks passed
@cferris1000 cferris1000 deleted the quarantine_fix branch August 5, 2025 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants