[scudo] Double frees result in chunk state error #110345
Merged
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
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.
llvmbot
added
compiler-rt
compiler-rt:scudo
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Christopher Ferris (cferris1000) ChangesFull diff: https://github.com/llvm/llvm-project/pull/110345.diff 2 Files Affected:
diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index a5f1bc388e8824..2e6867c6bafcb0 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) {
+ 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);
+ } 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();
|
cferris1000
changed the title
|
I didn't add secondary tests because that's a bit trickier since we tend to wipe the header in many cases. |
ChiaHungDuan
approved these changes
Sep 30, 2024
| Ptr = untagPointer(Ptr); | ||
| BlockBegin = getBlockBegin(Ptr, Header); | ||
| } else { | ||
| else | ||
| Header->OriginOrWasZeroed = | ||
| Header->ClassId && !TSDRegistry.getDisableMemInit(); |
There was a problem hiding this comment.
Small nit:
It's a multiple lines statement so I would add brackets here
ChiaHungDuan
approved these changes
Sep 30, 2024
|
Originally, I thought that the test might fail with a real crash without an abort when mte enabled. But I verified that it does properly detect the chunk state even when MTE is enabled. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/7118 Here is the relevant piece of the build log for the reference |
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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.