[scudo] Add a config parameter to compile out quarantine code #150144
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In order to save code size, allow a config that does not suport quarantining to turn it off. Add tests to verify that disabling the quarantine code works properly.
llvmbot
added
compiler-rt
compiler-rt:scudo
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Christopher Ferris (cferris1000) ChangesIn order to save code size, allow a config that does not suport quarantining to turn it off. Add tests to verify that disabling the quarantine code works properly. Full diff: https://github.com/llvm/llvm-project/pull/150144.diff 5 Files Affected:
diff --git a/compiler-rt/lib/scudo/standalone/allocator_config.def b/compiler-rt/lib/scudo/standalone/allocator_config.def
index 84fcec0877d40..748530820cd64 100644
--- a/compiler-rt/lib/scudo/standalone/allocator_config.def
+++ b/compiler-rt/lib/scudo/standalone/allocator_config.def
@@ -54,6 +54,9 @@ BASE_REQUIRED_TEMPLATE_TYPE(SecondaryT)
// Indicates possible support for Memory Tagging.
BASE_OPTIONAL(const bool, MaySupportMemoryTagging, false)
+// Disable the quarantine code.
+BASE_OPTIONAL(const bool, QuarantineDisabled, false)
+
// PRIMARY_REQUIRED_TYPE(NAME)
//
// SizeClassMap to use with the Primary.
diff --git a/compiler-rt/lib/scudo/standalone/allocator_config_wrapper.h b/compiler-rt/lib/scudo/standalone/allocator_config_wrapper.h
index ac639ee9dd943..5bfa700c2f8d8 100644
--- a/compiler-rt/lib/scudo/standalone/allocator_config_wrapper.h
+++ b/compiler-rt/lib/scudo/standalone/allocator_config_wrapper.h
@@ -60,6 +60,10 @@ template <typename AllocatorConfig> struct PrimaryConfig {
return BaseConfig<AllocatorConfig>::getMaySupportMemoryTagging();
}
+ static constexpr bool getQuarantineDisabled() {
+ return BaseConfig<AllocatorConfig>::getQuarantineDisabled();
+ }
+
#define PRIMARY_REQUIRED_TYPE(NAME) \
using NAME = typename AllocatorConfig::Primary::NAME;
@@ -92,6 +96,10 @@ template <typename AllocatorConfig> struct SecondaryConfig {
return BaseConfig<AllocatorConfig>::getMaySupportMemoryTagging();
}
+ static constexpr bool getQuarantineDisabled() {
+ return BaseConfig<AllocatorConfig>::getQuarantineDisabled();
+ }
+
#define SECONDARY_REQUIRED_TEMPLATE_TYPE(NAME) \
template <typename T> \
using NAME = typename AllocatorConfig::Secondary::template NAME<T>;
@@ -111,6 +119,10 @@ template <typename AllocatorConfig> struct SecondaryConfig {
return BaseConfig<AllocatorConfig>::getMaySupportMemoryTagging();
}
+ static constexpr bool getQuarantineDisabled() {
+ return BaseConfig<AllocatorConfig>::getQuarantineDisabled();
+ }
+
#define SECONDARY_CACHE_OPTIONAL(TYPE, NAME, DEFAULT) \
OPTIONAL_TEMPLATE(TYPE, NAME, DEFAULT, Cache::NAME) \
static constexpr removeConst<TYPE>::type get##NAME() { \
diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index 87acdec2a3bac..48a30ad0306ec 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -184,9 +184,10 @@ class Allocator {
const s32 ReleaseToOsIntervalMs = getFlags()->release_to_os_interval_ms;
Primary.init(ReleaseToOsIntervalMs);
Secondary.init(&Stats, ReleaseToOsIntervalMs);
- Quarantine.init(
- static_cast<uptr>(getFlags()->quarantine_size_kb << 10),
- static_cast<uptr>(getFlags()->thread_local_quarantine_size_kb << 10));
+ if (!AllocatorConfig::getQuarantineDisabled())
+ Quarantine.init(
+ static_cast<uptr>(getFlags()->quarantine_size_kb << 10),
+ static_cast<uptr>(getFlags()->thread_local_quarantine_size_kb << 10));
}
void enableRingBuffer() NO_THREAD_SAFETY_ANALYSIS {
@@ -276,16 +277,18 @@ class Allocator {
// the last two items).
void commitBack(TSD<ThisT> *TSD) {
TSD->assertLocked(/*BypassCheck=*/true);
- Quarantine.drain(&TSD->getQuarantineCache(),
- QuarantineCallback(*this, TSD->getSizeClassAllocator()));
+ if (!AllocatorConfig::getQuarantineDisabled())
+ Quarantine.drain(&TSD->getQuarantineCache(),
+ QuarantineCallback(*this, TSD->getSizeClassAllocator()));
TSD->getSizeClassAllocator().destroy(&Stats);
}
void drainCache(TSD<ThisT> *TSD) {
TSD->assertLocked(/*BypassCheck=*/true);
- Quarantine.drainAndRecycle(
- &TSD->getQuarantineCache(),
- QuarantineCallback(*this, TSD->getSizeClassAllocator()));
+ if (!AllocatorConfig::getQuarantineDisabled())
+ Quarantine.drainAndRecycle(
+ &TSD->getQuarantineCache(),
+ QuarantineCallback(*this, TSD->getSizeClassAllocator()));
TSD->getSizeClassAllocator().drain();
}
void drainCaches() { TSDRegistry.drainCaches(this); }
@@ -612,7 +615,8 @@ class Allocator {
#endif
TSDRegistry.disable();
Stats.disable();
- Quarantine.disable();
+ if (!AllocatorConfig::getQuarantineDisabled())
+ Quarantine.disable();
Primary.disable();
Secondary.disable();
disableRingBuffer();
@@ -623,7 +627,8 @@ class Allocator {
enableRingBuffer();
Secondary.enable();
Primary.enable();
- Quarantine.enable();
+ if (!AllocatorConfig::getQuarantineDisabled())
+ Quarantine.enable();
Stats.enable();
TSDRegistry.enable();
#ifdef GWP_ASAN_HOOKS
@@ -1252,7 +1257,8 @@ class Allocator {
// If the quarantine is disabled, the actual size of a chunk is 0 or larger
// than the maximum allowed, we return a chunk directly to the backend.
// This purposefully underflows for Size == 0.
- const bool BypassQuarantine = !Quarantine.getCacheSize() ||
+ const bool BypassQuarantine = AllocatorConfig::getQuarantineDisabled() ||
+ !Quarantine.getCacheSize() ||
((Size - 1) >= QuarantineMaxChunkSize) ||
!Header->ClassId;
if (BypassQuarantine)
@@ -1642,7 +1648,8 @@ class Allocator {
uptr getStats(ScopedString *Str) {
Primary.getStats(Str);
Secondary.getStats(Str);
- Quarantine.getStats(Str);
+ if (!AllocatorConfig::getQuarantineDisabled())
+ Quarantine.getStats(Str);
TSDRegistry.getStats(Str);
return Str->length();
}
diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index 286e5d332f57c..d72aa86f45e7c 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -302,26 +302,28 @@ class MapAllocatorCache {
if (Entry.Time != 0)
Entry.Time = Time;
- if (useMemoryTagging<Config>(Options) && QuarantinePos == -1U) {
- // 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
- // just unmap it.
- unmapCallBack(Entry.MemMap);
- break;
- }
- if (Config::getQuarantineSize() && useMemoryTagging<Config>(Options)) {
- QuarantinePos =
- (QuarantinePos + 1) % Max(Config::getQuarantineSize(), 1u);
- if (!Quarantine[QuarantinePos].isValid()) {
+ if (useMemoryTagging<Config>(Options)) {
+ if (Config::getQuarantineDisabled() || QuarantinePos == -1U) {
+ // 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 just unmap it.
+ unmapCallBack(Entry.MemMap);
+ break;
+ }
+ if (!Config::getQuarantineDisabled() && Config::getQuarantineSize()) {
+ QuarantinePos =
+ (QuarantinePos + 1) % Max(Config::getQuarantineSize(), 1u);
+ if (!Quarantine[QuarantinePos].isValid()) {
+ Quarantine[QuarantinePos] = Entry;
+ return;
+ }
+ CachedBlock PrevEntry = Quarantine[QuarantinePos];
Quarantine[QuarantinePos] = Entry;
- return;
+ if (OldestTime == 0)
+ OldestTime = Entry.Time;
+ Entry = PrevEntry;
}
- CachedBlock PrevEntry = Quarantine[QuarantinePos];
- Quarantine[QuarantinePos] = Entry;
- if (OldestTime == 0)
- OldestTime = Entry.Time;
- Entry = PrevEntry;
}
// All excess entries are evicted from the cache. Note that when
@@ -506,16 +508,18 @@ class MapAllocatorCache {
void disableMemoryTagging() EXCLUDES(Mutex) {
ScopedLock L(Mutex);
- for (u32 I = 0; I != Config::getQuarantineSize(); ++I) {
- if (Quarantine[I].isValid()) {
- MemMapT &MemMap = Quarantine[I].MemMap;
- unmapCallBack(MemMap);
- Quarantine[I].invalidate();
+ if (!Config::getQuarantineDisabled()) {
+ for (u32 I = 0; I != Config::getQuarantineSize(); ++I) {
+ if (Quarantine[I].isValid()) {
+ MemMapT &MemMap = Quarantine[I].MemMap;
+ unmapCallBack(MemMap);
+ 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(); }
@@ -572,8 +576,9 @@ class MapAllocatorCache {
if (!LRUEntries.size() || OldestTime == 0 || OldestTime > Time)
return;
OldestTime = 0;
- for (uptr I = 0; I < Config::getQuarantineSize(); I++)
- releaseIfOlderThan(Quarantine[I], Time);
+ if (!Config::getQuarantineDisabled())
+ for (uptr I = 0; I < Config::getQuarantineSize(); I++)
+ releaseIfOlderThan(Quarantine[I], Time);
for (uptr I = 0; I < Config::getEntriesArraySize(); I++)
releaseIfOlderThan(Entries[I], Time);
}
diff --git a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
index 7e8d5b4396d2e..8b55c6570650e 100644
--- a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
@@ -623,20 +623,20 @@ SCUDO_TYPED_TEST(ScudoCombinedDeathTest, DisableMemoryTagging) {
SCUDO_TYPED_TEST(ScudoCombinedTest, Stats) {
auto *Allocator = this->Allocator.get();
- scudo::uptr BufferSize = 8192;
- std::vector<char> Buffer(BufferSize);
- scudo::uptr ActualSize = Allocator->getStats(Buffer.data(), BufferSize);
- while (ActualSize > BufferSize) {
- BufferSize = ActualSize + 1024;
- Buffer.resize(BufferSize);
- ActualSize = Allocator->getStats(Buffer.data(), BufferSize);
+ std::string Stats(10000, '\0');
+ scudo::uptr ActualSize = Allocator->getStats(Stats.data(), Stats.size());
+ if (ActualSize > Stats.size()) {
+ Stats.resize(ActualSize);
+ ActualSize = Allocator->getStats(Stats.data(), Stats.size());
}
- std::string Stats(Buffer.begin(), Buffer.end());
+ EXPECT_GE(Stats.size(), ActualSize);
+
// Basic checks on the contents of the statistics output, which also allows us
// to verify that we got it all.
EXPECT_NE(Stats.find("Stats: SizeClassAllocator"), std::string::npos);
EXPECT_NE(Stats.find("Stats: MapAllocator"), std::string::npos);
- EXPECT_NE(Stats.find("Stats: Quarantine"), std::string::npos);
+ // Do not explicitly check for quarantine stats since a config can disable
+ // them. Other tests verify this (QuarantineEnabled/QuarantineDisabled).
}
SCUDO_TYPED_TEST_SKIP_THREAD_SAFETY(ScudoCombinedTest, Drain) {
@@ -1076,3 +1076,107 @@ TEST(ScudoCombinedTest, BasicTrustyConfig) {
#endif
#endif
+
+struct TestQuarantineSizeClassConfig {
+ static const scudo::uptr NumBits = 1;
+ static const scudo::uptr MinSizeLog = 10;
+ static const scudo::uptr MidSizeLog = 10;
+ static const scudo::uptr MaxSizeLog = 13;
+ static const scudo::u16 MaxNumCachedHint = 8;
+ static const scudo::uptr MaxBytesCachedLog = 12;
+ static const scudo::uptr SizeDelta = 0;
+};
+
+struct TestQuarantineConfig {
+ static const bool MaySupportMemoryTagging = false;
+
+ template <class A> using TSDRegistryT = scudo::TSDRegistrySharedT<A, 1U, 1U>;
+
+ struct Primary {
+ // Tiny allocator, its Primary only serves chunks of four sizes.
+ using SizeClassMap = scudo::FixedSizeClassMap<DeathSizeClassConfig>;
+ static const scudo::uptr RegionSizeLog = DeathRegionSizeLog;
+ static const scudo::s32 MinReleaseToOsIntervalMs = INT32_MIN;
+ static const scudo::s32 MaxReleaseToOsIntervalMs = INT32_MAX;
+ typedef scudo::uptr CompactPtrT;
+ static const scudo::uptr CompactPtrScale = 0;
+ static const bool EnableRandomOffset = true;
+ static const scudo::uptr MapSizeIncrement = 1UL << 18;
+ static const scudo::uptr GroupSizeLog = 18;
+ };
+ template <typename Config>
+ using PrimaryT = scudo::SizeClassAllocator64<Config>;
+
+ struct Secondary {
+ template <typename Config>
+ using CacheT = scudo::MapAllocatorNoCache<Config>;
+ };
+
+ template <typename Config> using SecondaryT = scudo::MapAllocator<Config>;
+};
+
+TEST(ScudoCombinedTest, QuarantineEnabled) {
+ // Enable quarantine options.
+ EXPECT_EQ(0, setenv("SCUDO_OPTIONS",
+ "quarantine_max_chunk_size=65535:quarantine_size_kb=500:"
+ "thread_local_quarantine_size_kb=600",
+ true));
+
+ using AllocatorT = scudo::Allocator<TestQuarantineConfig>;
+ auto Allocator = std::unique_ptr<AllocatorT>(new AllocatorT());
+
+ void *P = Allocator->allocate(100U, Origin);
+ EXPECT_NE(P, nullptr);
+ Allocator->deallocate(P, Origin);
+
+ std::string Stats(10000, '\0');
+ scudo::uptr ActualSize = Allocator->getStats(Stats.data(), Stats.size());
+ if (ActualSize > Stats.size()) {
+ Stats.resize(ActualSize);
+ ActualSize = Allocator->getStats(Stats.data(), Stats.size());
+ }
+ EXPECT_GE(Stats.size(), ActualSize);
+
+ // Verify we get quarantine stats.
+ EXPECT_NE(Stats.find("Stats: Quarantine"), std::string::npos);
+ // Verify that quarantine values get set properly.
+ EXPECT_NE(Stats.find("Quarantine limits: global: 500K; thread local: 600K"),
+ std::string::npos);
+
+ // Clear the options for all other tests.
+ EXPECT_EQ(0, setenv("SCUDO_OPTIONS", "", true));
+}
+
+struct TestQuarantineDisabledConfig : TestQuarantineConfig {
+ static const bool QuarantineDisabled = true;
+};
+
+TEST(ScudoCombinedTest, QuarantineDisabled) {
+ // Enable quarantine options.
+ EXPECT_EQ(0, setenv("SCUDO_OPTIONS",
+ "quarantine_max_chunk_size=65535:quarantine_size_kb=500:"
+ "thread_local_quarantine_size_kb=600",
+ true));
+
+ using AllocatorT = scudo::Allocator<TestQuarantineDisabledConfig>;
+ auto Allocator = std::unique_ptr<AllocatorT>(new AllocatorT());
+
+ const scudo::uptr Size = 1000U;
+ void *P = Allocator->allocate(Size, Origin);
+ EXPECT_NE(P, nullptr);
+ Allocator->deallocate(P, Origin);
+
+ std::string Stats(10000, '\0');
+ scudo::uptr ActualSize = Allocator->getStats(Stats.data(), Stats.size());
+ if (ActualSize > Stats.size()) {
+ Stats.resize(ActualSize);
+ ActualSize = Allocator->getStats(Stats.data(), Stats.size());
+ }
+ EXPECT_GE(Stats.size(), ActualSize);
+
+ // No quarantine stats should exist because the code is not enabled.
+ EXPECT_EQ(Stats.find("Stats: Quarantine"), std::string::npos);
+
+ // Clear the options for all other tests.
+ EXPECT_EQ(0, setenv("SCUDO_OPTIONS", "", true));
+}
|
|
Looking at the secondary code, it looks like there is a bug in there so I'm looking at it. Specifically, the comment says if memory tagging was disabled, but it's checking if memory tagging is enabled. I'm trying to figure out if the comment is wrong or the code is wrong. |
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to save code size, allow a config that does not suport quarantining to turn it off.
Add tests to verify that disabling the quarantine code works properly.