From c094edb491bb6caf2afa2b52984ecf9b685cd7e7 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Fri, 19 Sep 2025 18:42:20 -0700 Subject: [PATCH 1/3] [scudo] Skip special quarantine blocks in iterateOverChunks If a quarantine block is allocated, it will be passed through to a user who calls iterateOverChunks. Make these special block allocations state Quarantined so they are not passed to iterateOverChunks. Remove the FIXME in the combined tests for quarantine since this fixes those tests too. Also add a specific new test that fails without this fix. --- compiler-rt/lib/scudo/standalone/combined.h | 5 +- .../scudo/standalone/tests/combined_test.cpp | 77 ++++++++++++------- 2 files changed, 54 insertions(+), 28 deletions(-) diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h index c9ba28a52f780..1ef6c5cd5698a 100644 --- a/compiler-rt/lib/scudo/standalone/combined.h +++ b/compiler-rt/lib/scudo/standalone/combined.h @@ -101,7 +101,8 @@ class Allocator { Chunk::UnpackedHeader Header = {}; Header.ClassId = QuarantineClassId & Chunk::ClassIdMask; Header.SizeOrUnusedBytes = sizeof(QuarantineBatch); - Header.State = Chunk::State::Allocated; + // Do not use Allocated or this block will show up in iterateOverChunks. + Header.State = Chunk::State::Quarantined; Chunk::storeHeader(Allocator.Cookie, Ptr, &Header); // Reset tag to 0 as this chunk may have been previously used for a tagged @@ -120,7 +121,7 @@ class Allocator { Chunk::UnpackedHeader Header; Chunk::loadHeader(Allocator.Cookie, Ptr, &Header); - if (UNLIKELY(Header.State != Chunk::State::Allocated)) + if (UNLIKELY(Header.State != Chunk::State::Quarantined)) reportInvalidChunkState(AllocatorAction::Deallocating, Ptr); DCHECK_EQ(Header.ClassId, QuarantineClassId); DCHECK_EQ(Header.Offset, 0); diff --git a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp index 1eff9ebcb7a4f..5de0846624507 100644 --- a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp +++ b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include static constexpr scudo::Chunk::Origin Origin = scudo::Chunk::Origin::Malloc; @@ -151,12 +152,10 @@ void TestAllocator::operator delete(void *ptr) { template struct ScudoCombinedTest : public Test { ScudoCombinedTest() { - UseQuarantine = std::is_same::value; Allocator = std::make_unique(); } ~ScudoCombinedTest() { Allocator->releaseToOS(scudo::ReleaseToOS::Force); - UseQuarantine = true; } void RunTest(); @@ -525,30 +524,25 @@ SCUDO_TYPED_TEST(ScudoCombinedTest, IterateOverChunks) { auto *Allocator = this->Allocator.get(); // Allocates a bunch of chunks, then iterate over all the chunks, ensuring // they are the ones we allocated. This requires the allocator to not have any - // other allocated chunk at this point (eg: won't work with the Quarantine). - // FIXME: Make it work with UseQuarantine and tagging enabled. Internals of - // iterateOverChunks reads header by tagged and non-tagger pointers so one of - // them will fail. - if (!UseQuarantine) { - std::vector V; - for (scudo::uptr I = 0; I < 64U; I++) - V.push_back(Allocator->allocate( - static_cast(std::rand()) % - (TypeParam::Primary::SizeClassMap::MaxSize / 2U), - Origin)); - Allocator->disable(); - Allocator->iterateOverChunks( - 0U, static_cast(SCUDO_MMAP_RANGE_SIZE - 1), - [](uintptr_t Base, UNUSED size_t Size, void *Arg) { - std::vector *V = reinterpret_cast *>(Arg); - void *P = reinterpret_cast(Base); - EXPECT_NE(std::find(V->begin(), V->end(), P), V->end()); - }, - reinterpret_cast(&V)); - Allocator->enable(); - for (auto P : V) - Allocator->deallocate(P, Origin); - } + // other allocated chunk at this point. + std::vector V; + for (scudo::uptr I = 0; I < 64U; I++) + V.push_back(Allocator->allocate( + static_cast(std::rand()) % + (TypeParam::Primary::SizeClassMap::MaxSize / 2U), + Origin)); + Allocator->disable(); + Allocator->iterateOverChunks( + 0U, static_cast(SCUDO_MMAP_RANGE_SIZE - 1), + [](uintptr_t Base, UNUSED size_t Size, void *Arg) { + std::vector *V = reinterpret_cast *>(Arg); + void *P = reinterpret_cast(Base); + EXPECT_NE(std::find(V->begin(), V->end(), P), V->end()); + }, + reinterpret_cast(&V)); + Allocator->enable(); + for (auto P : V) + Allocator->deallocate(P, Origin); } SCUDO_TYPED_TEST(ScudoCombinedDeathTest, UseAfterFree) { @@ -1161,3 +1155,34 @@ TEST(ScudoCombinedTest, QuarantineDisabled) { // No quarantine stats should not be present. EXPECT_EQ(Stats.find("Stats: Quarantine"), std::string::npos); } + +// Verify that no special quarantine blocks appear in iterateOverChunks. +TEST(ScudoCombinedTest, QuarantineIterateOverChunks) { + using AllocatorT = scudo::Allocator; + auto Allocator = std::unique_ptr(new AllocatorT()); + + // Do a bunch of allocations and deallocations. At the end there should + // be no special quarantine blocks in our callbacks, and no blocks at all. + std::vector Sizes = {128, 128, 256, 256}; + for (auto const Size : Sizes) { + void *Ptr = Allocator->allocate(Size, Origin); + EXPECT_NE(Ptr, nullptr); + Allocator->deallocate(Ptr, Origin); + } + std::unordered_map Pointers; + Allocator->disable(); + Allocator->iterateOverChunks( + 0, static_cast(SCUDO_MMAP_RANGE_SIZE - 1), + [](uintptr_t Base, size_t Size, void *Arg) { + std::unordered_map *Pointers = + reinterpret_cast *>(Arg); + (*Pointers)[Base] = Size; + }, + reinterpret_cast(&Pointers)); + Allocator->enable(); + + for (const auto [Base, Size] : Pointers) { + EXPECT_TRUE(false) << "Unexpected pointer found in iterateOverChunks " + << std::hex << Base << " Size " << std::dec << Size; + } +} From 4bf445d10024b1f3d57b8576a9b6e19d95ff5dec Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Fri, 19 Sep 2025 19:01:59 -0700 Subject: [PATCH 2/3] clang-format update. --- compiler-rt/lib/scudo/standalone/tests/combined_test.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp index 5de0846624507..5b56b973d55f8 100644 --- a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp +++ b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp @@ -151,12 +151,8 @@ void TestAllocator::operator delete(void *ptr) { } template struct ScudoCombinedTest : public Test { - ScudoCombinedTest() { - Allocator = std::make_unique(); - } - ~ScudoCombinedTest() { - Allocator->releaseToOS(scudo::ReleaseToOS::Force); - } + ScudoCombinedTest() { Allocator = std::make_unique(); } + ~ScudoCombinedTest() { Allocator->releaseToOS(scudo::ReleaseToOS::Force); } void RunTest(); From 177aa9c1021bb7fb316eab006ade78ea2cde2718 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Mon, 22 Sep 2025 13:54:39 -0700 Subject: [PATCH 3/3] Remove commment as mentioned by reviewer. --- compiler-rt/lib/scudo/standalone/combined.h | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h index 1ef6c5cd5698a..329ec4596482b 100644 --- a/compiler-rt/lib/scudo/standalone/combined.h +++ b/compiler-rt/lib/scudo/standalone/combined.h @@ -101,7 +101,6 @@ class Allocator { Chunk::UnpackedHeader Header = {}; Header.ClassId = QuarantineClassId & Chunk::ClassIdMask; Header.SizeOrUnusedBytes = sizeof(QuarantineBatch); - // Do not use Allocated or this block will show up in iterateOverChunks. Header.State = Chunk::State::Quarantined; Chunk::storeHeader(Allocator.Cookie, Ptr, &Header);