[asan] Fix misalignment of variables in fake stack frames #152819
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
ASan's instrumentation pass uses ASanStackFrameLayout::ComputeASanStackFrameLayout() to calculate the offset of variables, taking into account alignment. However, the fake stacks (specifically, GetFrame()) are not sufficiently aligned, hence the offset addresses are not guaranteed to be aligned. This change fixes the misalignment issue by aligning the memory used to back the fake stacks, and also padding the space used for flags, to maintain an alignment invariant. This costs approximately 128KB of additional memory per fake stack.
llvmbot
added
compiler-rt
compiler-rt:asan
|
@alazarev FYI - this fixes the internal bug |
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Thurston Dang (thurstond) ChangesASan's instrumentation pass uses ASanStackFrameLayout::ComputeASanStackFrameLayout() to calculate the offset of variables, taking into account alignment. However, the fake stacks (specifically, GetFrame()) are not sufficiently aligned, hence the offset addresses are not guaranteed to be aligned. This change fixes the misalignment issue by aligning the fake stacks and padding the space used for flags, to maintain the alignment invariant. This costs approximately 60KB of additional memory per fake stack. Full diff: https://github.com/llvm/llvm-project/pull/152819.diff 3 Files Affected:
diff --git a/compiler-rt/lib/asan/asan_fake_stack.cpp b/compiler-rt/lib/asan/asan_fake_stack.cpp
index 0f696075fb78d..baef8069a485b 100644
--- a/compiler-rt/lib/asan/asan_fake_stack.cpp
+++ b/compiler-rt/lib/asan/asan_fake_stack.cpp
@@ -55,9 +55,16 @@ FakeStack *FakeStack::Create(uptr stack_size_log) {
if (stack_size_log > kMaxStackSizeLog)
stack_size_log = kMaxStackSizeLog;
uptr size = RequiredSize(stack_size_log);
+ uptr padded_size = size + (1 << kMaxStackFrameSizeLog);
+ // Alignment here is needed to protect the alignment invariant in GetFrame()
+ // MmapAlignedOrDieOnFatalError requires that the *size* is a power of 2,
+ // which is an overly strong condition.
+ void *true_res = reinterpret_cast<void *>(
+ flags()->uar_noreserve ? MmapNoReserveOrDie(padded_size, "FakeStack")
+ : MmapOrDie(padded_size, "FakeStack"));
FakeStack *res = reinterpret_cast<FakeStack *>(
- flags()->uar_noreserve ? MmapNoReserveOrDie(size, "FakeStack")
- : MmapOrDie(size, "FakeStack"));
+ RoundUpTo((uptr)true_res, 1 << kMaxStackFrameSizeLog));
+ res->true_start = true_res;
res->stack_size_log_ = stack_size_log;
u8 *p = reinterpret_cast<u8 *>(res);
VReport(1,
@@ -79,8 +86,11 @@ void FakeStack::Destroy(int tid) {
Report("T%d: FakeStack destroyed: %s\n", tid, str.data());
}
uptr size = RequiredSize(stack_size_log_);
- FlushUnneededASanShadowMemory(reinterpret_cast<uptr>(this), size);
- UnmapOrDie(this, size);
+ uptr padded_size = size + (1 << kMaxStackFrameSizeLog);
+ void *true_start = this->true_start;
+ FlushUnneededASanShadowMemory(reinterpret_cast<uptr>(true_start),
+ padded_size);
+ UnmapOrDie(true_start, padded_size);
}
void FakeStack::PoisonAll(u8 magic) {
diff --git a/compiler-rt/lib/asan/asan_fake_stack.h b/compiler-rt/lib/asan/asan_fake_stack.h
index 270a19816d6e2..5255f1bf443f7 100644
--- a/compiler-rt/lib/asan/asan_fake_stack.h
+++ b/compiler-rt/lib/asan/asan_fake_stack.h
@@ -42,11 +42,16 @@ struct FakeFrame {
// is a power of two, starting from 64 bytes. Each size class occupies
// stack_size bytes and thus can allocate
// NumberOfFrames=(stack_size/BytesInSizeClass) fake frames (also a power of 2).
+//
// For each size class we have NumberOfFrames allocation flags,
// each flag indicates whether the given frame is currently allocated.
-// All flags for size classes 0 .. 10 are stored in a single contiguous region
-// followed by another contiguous region which contains the actual memory for
-// size classes. The addresses are computed by GetFlags and GetFrame without
+//
+// All flags for size classes 0 .. 10 are stored in a single contiguous region,
+// padded to 2**kMaxStackFrameSizeLog (to prevent frames from becoming
+// unaligned), followed by another contiguous region which contains the actual
+// memory for size classes.
+//
+// The addresses are computed by GetFlags and GetFrame without
// any memory accesses solely based on 'this' and stack_size_log.
// Allocate() flips the appropriate allocation flag atomically, thus achieving
// async-signal safety.
@@ -68,7 +73,9 @@ class FakeStack {
// stack_size_log is at least 15 (stack_size >= 32K).
static uptr SizeRequiredForFlags(uptr stack_size_log) {
- return ((uptr)1) << (stack_size_log + 1 - kMinStackFrameSizeLog);
+ // Padding is needed to protect alignment in GetFrame().
+ uptr size = ((uptr)1) << (stack_size_log + 1 - kMinStackFrameSizeLog);
+ return RoundUpTo(size + 1, 1 << kMaxStackFrameSizeLog) - kFlagsOffset;
}
// Each size class occupies stack_size bytes.
@@ -156,7 +163,7 @@ class FakeStack {
private:
FakeStack() { }
- static const uptr kFlagsOffset = 4096; // This is were the flags begin.
+ static const uptr kFlagsOffset = 4096; // This is where the flags begin.
// Must match the number of uses of DEFINE_STACK_MALLOC_FREE_WITH_CLASS_ID
COMPILER_CHECK(kNumberOfSizeClasses == 11);
static const uptr kMaxStackMallocSize = ((uptr)1) << kMaxStackFrameSizeLog;
@@ -165,6 +172,10 @@ class FakeStack {
uptr stack_size_log_;
// a bit is set if something was allocated from the corresponding size class.
bool needs_gc_;
+ // We allocated more memory than needed to ensure the FakeStack (and, by
+ // extension, each of the fake stack frames) is aligned. We keep track of the
+ // true start so that we can unmap it.
+ void *true_start;
};
FakeStack *GetTLSFakeStack();
diff --git a/compiler-rt/lib/asan/tests/asan_fake_stack_test.cpp b/compiler-rt/lib/asan/tests/asan_fake_stack_test.cpp
index 504b0aaf30143..d48a10532685a 100644
--- a/compiler-rt/lib/asan/tests/asan_fake_stack_test.cpp
+++ b/compiler-rt/lib/asan/tests/asan_fake_stack_test.cpp
@@ -25,9 +25,9 @@
namespace __asan {
TEST(FakeStack, FlagsSize) {
- EXPECT_EQ(FakeStack::SizeRequiredForFlags(10), 1U << 5);
- EXPECT_EQ(FakeStack::SizeRequiredForFlags(11), 1U << 6);
- EXPECT_EQ(FakeStack::SizeRequiredForFlags(20), 1U << 15);
+ EXPECT_EQ(FakeStack::SizeRequiredForFlags(10), (1U << 16) - 4096);
+ EXPECT_EQ(FakeStack::SizeRequiredForFlags(11), (1U << 16) - 4096);
+ EXPECT_EQ(FakeStack::SizeRequiredForFlags(20), (1U << 16) - 4096);
}
TEST(FakeStack, RequiredSize) {
@@ -35,11 +35,11 @@ TEST(FakeStack, RequiredSize) {
// uptr alloc_size = FakeStack::RequiredSize(i);
// printf("%zdK ==> %zd\n", 1 << (i - 10), alloc_size);
// }
- EXPECT_EQ(FakeStack::RequiredSize(15), 365568U);
- EXPECT_EQ(FakeStack::RequiredSize(16), 727040U);
- EXPECT_EQ(FakeStack::RequiredSize(17), 1449984U);
- EXPECT_EQ(FakeStack::RequiredSize(18), 2895872U);
- EXPECT_EQ(FakeStack::RequiredSize(19), 5787648U);
+ EXPECT_EQ(FakeStack::RequiredSize(15), 425984U);
+ EXPECT_EQ(FakeStack::RequiredSize(16), 786432U);
+ EXPECT_EQ(FakeStack::RequiredSize(17), 1507328U);
+ EXPECT_EQ(FakeStack::RequiredSize(18), 2949120U);
+ EXPECT_EQ(FakeStack::RequiredSize(19), 5832704U);
}
TEST(FakeStack, FlagsOffset) {
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
thurstond
added a commit
to thurstond/llvm-project
that referenced
this pull request
Aug 10, 2025
This test case demonstrates that ASan does not currently align FakeStack frames correctly for 4KB objects. It deliberately uses a smaller thread stack size (64KB), which forces the FakeStack frames to no longer be 4KB aligned. This differs from llvm#152889, which is a test case for objects >4KB, which relies on the fact that the default 4KB alignment for fake stack sizes >64KB is insufficient. llvm#152819 will fix both issues.
|
LGTM |
fmayer
reviewed
Aug 11, 2025
| FlushUnneededASanShadowMemory(reinterpret_cast<uptr>(this), size); | ||
| UnmapOrDie(this, size); | ||
| uptr padded_size = size + (1 << kMaxStackFrameSizeLog); | ||
| void *true_start = this->true_start; |
There was a problem hiding this comment.
Why do we need this? this-> is implicit
| @@ -55,17 +55,32 @@ FakeStack *FakeStack::Create(uptr stack_size_log) { | |||
| if (stack_size_log > kMaxStackSizeLog) | |||
| stack_size_log = kMaxStackSizeLog; | |||
| uptr size = RequiredSize(stack_size_log); | |||
| uptr padded_size = size + (1 << kMaxStackFrameSizeLog); | |||
There was a problem hiding this comment.
I think this would be a bit easier to read if we had
constexpr auto kMaxStackFrameSize = 1 << kMaxStackFrameSizeLog;
and then used this
There was a problem hiding this comment.
Added a static const
thurstond
added a commit
that referenced
this pull request
Aug 11, 2025
This test case demonstrates that ASan does not currently align FakeStack frames correctly: - for 4KB objects on a 64KB stack, alignment is deterministically incorrect - for objects larger than 4KB, even with large stack sizes, alignment is not guaranteed #152819 will fix it.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Aug 11, 2025
…52889) This test case demonstrates that ASan does not currently align FakeStack frames correctly: - for 4KB objects on a 64KB stack, alignment is deterministically incorrect - for objects larger than 4KB, even with large stack sizes, alignment is not guaranteed llvm/llvm-project#152819 will fix it.
Update test to passing.
fmayer
approved these changes
Aug 11, 2025
fmayer
approved these changes
Aug 12, 2025
|
Remoev XFAIL of the tests you precommited |
Already done: 44b8a3c |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/22621 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/107/builds/13547 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/16933 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/14039 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/12048 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/11509 Here is the relevant piece of the build log for the reference |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Aug 12, 2025
… frames" (#153139) Reverts llvm/llvm-project#152819 due to buildbot failures
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/202/builds/2807 Here is the relevant piece of the build log for the reference |
thurstond
added a commit
to thurstond/llvm-project
that referenced
this pull request
Aug 12, 2025
…lvm#153139) This reverts commit 29ad073 i.e., relands 927e19f. It was reverted because of buildbot breakages. This reland adds "-pthread" and also moves the test to Posix-only. Original commit message: [asan] Fix misalignment of variables in fake stack frames (llvm#152819) ASan's instrumentation pass uses `ASanStackFrameLayout::ComputeASanStackFrameLayout()` to calculate the offset of variables, taking into account alignment. However, the fake stack frames returned by the runtime's `GetFrame()` are not guaranteed to be sufficiently aligned (and in some cases, even guaranteed to be misaligned), hence the offset addresses may sometimes be misaligned. This change fixes the misalignment issue by padding the FakeStack. Every fake stack frame is guaranteed to be aligned to the size of the frame. The memory overhead is low: 64KB per FakeStack, compared to the FakeStack size of ~700KB (min) to 11MB (max). Updates the test case from llvm#152889.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/9005 Here is the relevant piece of the build log for the reference |
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.
ASan's instrumentation pass uses
ASanStackFrameLayout::ComputeASanStackFrameLayout()to calculate the offset of variables, taking into account alignment. However, the fake stack frames returned by the runtime'sGetFrame()are not guaranteed to be sufficiently aligned, hence the offset addresses may sometimes be misaligned.This change fixes the misalignment issue by padding the FakeStack.
The memory overhead is low: 64KB per FakeStack, compared to the FakeStack size of ~700KB (min) to 11MB (max).
Updates the test case from #152889.