Skip to content

[asan] Fix misalignment of variables in fake stack frames #152819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 29 commits into from
Aug 12, 2025
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
45198b2
[asan] Fix misalignment of variables in fake stack frames
thurstond Aug 9, 2025
fdfcc01
Update SizeRequiredForFlags
thurstond Aug 9, 2025
0b69b3b
Proof of alignment invariant in GetFrame()
thurstond Aug 9, 2025
fe98da4
GetFrame() wording
thurstond Aug 9, 2025
58e1841
Punctuation
thurstond Aug 9, 2025
222c89f
Wording
thurstond Aug 9, 2025
ccb538f
Fix claim
thurstond Aug 9, 2025
48bf1aa
Comment on near-optimality
thurstond Aug 9, 2025
bbecef4
Wording
thurstond Aug 9, 2025
0e5b514
Simplify alignment step to FakeStack::Create() only
thurstond Aug 9, 2025
570f8cf
Revert test
thurstond Aug 9, 2025
d4441e4
Update FakeStack comment
thurstond Aug 9, 2025
7b314b5
Remove duplicated comment
thurstond Aug 9, 2025
6cef8c5
Update comment
thurstond Aug 9, 2025
46e7c1e
Formatting
thurstond Aug 9, 2025
07ee0bb
Add alignment check to test
thurstond Aug 9, 2025
8282b9b
clang-format
thurstond Aug 9, 2025
a378876
More logging
thurstond Aug 9, 2025
a1859f0
Fix edge case of frame with small variables that need heavy alignment
thurstond Aug 9, 2025
d8dab9f
Reword comment
thurstond Aug 9, 2025
8529f76
Partly revert aligned local stack size change, to maximize protection
thurstond Aug 9, 2025
8df0f39
Statically assert that FakeStack as a whole is aligned
thurstond Aug 9, 2025
cc040e1
Drive-by fix: remove deprecated comment
thurstond Aug 9, 2025
e19176e
Add note that most modern compilers have sizeof(type) >= alignof(type)
thurstond Aug 10, 2025
4df767d
Revert ASan instrumentation change
thurstond Aug 10, 2025
6ccb151
Update commentary on min stack size and GetFrame 4K alignment
thurstond Aug 10, 2025
d31eea6
Merge remote-tracking branch 'upstream/main' into asan_align_fake_stack
thurstond Aug 11, 2025
44b8a3c
Use kMaxStackFrameSize and remove local true_start.
thurstond Aug 11, 2025
96845f2
Replace other usages of 1 << kMaxStackFrameSizeLog
thurstond Aug 11, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions compiler-rt/lib/asan/asan_fake_stack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be a bit easier to read if we had

constexpr auto kMaxStackFrameSize = 1 << kMaxStackFrameSizeLog;

and then used this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a static const

void *true_res = reinterpret_cast<void *>(
flags()->uar_noreserve ? MmapNoReserveOrDie(padded_size, "FakeStack")
: MmapOrDie(padded_size, "FakeStack"));
// GetFrame() requires the property that
// (res + kFlagsOffset + SizeRequiredForFlags(stack_size_log)) is aligned to
// (1 << kMaxStackFrameSizeLog).
// We didn't use MmapAlignedOrDieOnFatalError, because it requires that the
// *size* is a power of 2, which is an overly strong condition.
static_assert(alignof(FakeStack) <= (1 << kMaxStackFrameSizeLog));
FakeStack *res = reinterpret_cast<FakeStack *>(
flags()->uar_noreserve ? MmapNoReserveOrDie(size, "FakeStack")
: MmapOrDie(size, "FakeStack"));
RoundUpTo(
(uptr)true_res + kFlagsOffset + SizeRequiredForFlags(stack_size_log),
1 << kMaxStackFrameSizeLog) -
kFlagsOffset - SizeRequiredForFlags(stack_size_log));
res->true_start = true_res;
res->stack_size_log_ = stack_size_log;
u8 *p = reinterpret_cast<u8 *>(res);
VReport(1,
"T%d: FakeStack created: %p -- %p stack_size_log: %zd; "
"mmapped %zdK, noreserve=%d \n",
"mmapped %zdK, noreserve=%d, true_start: %p, start of first frame: "
"0x%zx\n",
GetCurrentTidOrInvalid(), (void *)p,
(void *)(p + FakeStack::RequiredSize(stack_size_log)), stack_size_log,
size >> 10, flags()->uar_noreserve);
size >> 10, flags()->uar_noreserve, res->true_start,
res->GetFrame(stack_size_log, /*class_id*/ 0, /*pos*/ 0));
return res;
}

Expand All @@ -79,8 +94,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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? this-> is implicit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

FlushUnneededASanShadowMemory(reinterpret_cast<uptr>(true_start),
padded_size);
UnmapOrDie(true_start, padded_size);
}

void FakeStack::PoisonAll(u8 magic) {
Expand Down
33 changes: 29 additions & 4 deletions compiler-rt/lib/asan/asan_fake_stack.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ struct FakeFrame {
// is not popped but remains there for quite some time until gets used again.
// So, we poison the objects on the fake stack when function returns.
// It helps us find use-after-return bugs.
//
// The FakeStack objects is allocated by a single mmap call and has no other
// pointers. The size of the fake stack depends on the actual thread stack size
// and thus can not be a constant.
// stack_size is a power of two greater or equal to the thread's stack size;
// we store it as its logarithm (stack_size_log).
// FakeStack is padded such that GetFrame() is aligned to BytesInSizeClass().
// FakeStack has kNumberOfSizeClasses (11) size classes, each size class
// is a power of two, starting from 64 bytes. Each size class occupies
// stack_size bytes and thus can allocate
Expand Down Expand Up @@ -66,7 +66,7 @@ class FakeStack {

void Destroy(int tid);

// stack_size_log is at least 15 (stack_size >= 32K).
// min_uar_stack_size_log is 16 (stack_size >= 64KB)
static uptr SizeRequiredForFlags(uptr stack_size_log) {
return ((uptr)1) << (stack_size_log + 1 - kMinStackFrameSizeLog);
}
Expand Down Expand Up @@ -110,6 +110,28 @@ class FakeStack {
}

// Get frame by class_id and pos.
// Return values are guaranteed to be aligned to BytesInSizeClass(class_id),
// which is useful in combination with
// ASanStackFrameLayout::ComputeASanStackFrameLayout().
//
// Note that alignment to 1<<kMaxStackFrameSizeLog (aka
// BytesInSizeClass(max_class_id)) implies alignment to BytesInSizeClass()
// for any class_id, since the class sizes are increasing powers of 2.
//
// 1) (this + kFlagsOffset + SizeRequiredForFlags())) is aligned to
// 1<<kMaxStackFrameSizeLog (see FakeStack::Create)
//
// Note that SizeRequiredForFlags(16) == 2048. If FakeStack::Create() had
// merely returned an address from mmap (4K-aligned), the addition would
// not be 4K-aligned.
// 2) We know that stack_size_log >= kMaxStackFrameSizeLog (otherwise you
// couldn't store a single frame of that size in the entire stack)
// hence (1<<stack_size_log) is aligned to 1<<kMaxStackFrameSizeLog
// and ((1<<stack_size_log) * class_id) is aligned to
// 1<<kMaxStackFrameSizeLog
// 3) BytesInSizeClass(class_id) * pos is aligned to
// BytesInSizeClass(class_id)
// The sum of these is aligned to BytesInSizeClass(class_id).
u8 *GetFrame(uptr stack_size_log, uptr class_id, uptr pos) {
return reinterpret_cast<u8 *>(this) + kFlagsOffset +
SizeRequiredForFlags(stack_size_log) +
Expand Down Expand Up @@ -156,15 +178,18 @@ 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;

uptr hint_position_[kNumberOfSizeClasses];
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();
Expand Down
1 change: 1 addition & 0 deletions compiler-rt/lib/asan/tests/asan_fake_stack_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ TEST(FakeStack, Allocate) {
uptr bytes_in_class = FakeStack::BytesInSizeClass(cid);
for (uptr j = 0; j < n; j++) {
FakeFrame *ff = fs->Allocate(stack_size_log, cid, 0);
EXPECT_EQ(reinterpret_cast<uptr>(ff) % bytes_in_class, 0U);
uptr x = reinterpret_cast<uptr>(ff);
EXPECT_TRUE(s.insert(std::make_pair(ff, cid)).second);
EXPECT_EQ(x, fs->AddrIsInFakeStack(x));
Expand Down
Loading