From 35dc9443bdd9ed2bb034f94778149eb5177c5c04 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Tue, 12 Aug 2025 05:41:41 +0000 Subject: [PATCH 1/3] Reapply "[asan] Fix misalignment of variables in fake stack frames" (#153139) This reverts commit 29ad073c6c325dbf92c1aa5a285ca48e55cb918b i.e., relands 927e19f5f3b357823f86f6c4f1378abedccadf27. 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 (#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 https://github.com/llvm/llvm-project/pull/152889. --- compiler-rt/lib/asan/asan_fake_stack.cpp | 30 ++++++++++++---- compiler-rt/lib/asan/asan_fake_stack.h | 36 ++++++++++++++++--- .../lib/asan/tests/asan_fake_stack_test.cpp | 1 + .../asan/TestCases/fakestack_alignment.cpp | 10 +++--- 4 files changed, 61 insertions(+), 16 deletions(-) diff --git a/compiler-rt/lib/asan/asan_fake_stack.cpp b/compiler-rt/lib/asan/asan_fake_stack.cpp index 0f696075fb78d..c3ed2526f0ed4 100644 --- a/compiler-rt/lib/asan/asan_fake_stack.cpp +++ b/compiler-rt/lib/asan/asan_fake_stack.cpp @@ -54,18 +54,34 @@ FakeStack *FakeStack::Create(uptr stack_size_log) { stack_size_log = kMinStackSizeLog; if (stack_size_log > kMaxStackSizeLog) stack_size_log = kMaxStackSizeLog; + CHECK_LE(kMaxStackFrameSizeLog, stack_size_log); uptr size = RequiredSize(stack_size_log); + uptr padded_size = size + kMaxStackFrameSize; + void *true_res = reinterpret_cast( + 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 + // kMaxStackFrameSize. + // 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) <= kMaxStackFrameSize); FakeStack *res = reinterpret_cast( - flags()->uar_noreserve ? MmapNoReserveOrDie(size, "FakeStack") - : MmapOrDie(size, "FakeStack")); + RoundUpTo( + (uptr)true_res + kFlagsOffset + SizeRequiredForFlags(stack_size_log), + kMaxStackFrameSize) - + kFlagsOffset - SizeRequiredForFlags(stack_size_log)); + res->true_start = true_res; res->stack_size_log_ = stack_size_log; u8 *p = reinterpret_cast(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; } @@ -79,8 +95,10 @@ void FakeStack::Destroy(int tid) { Report("T%d: FakeStack destroyed: %s\n", tid, str.data()); } uptr size = RequiredSize(stack_size_log_); - FlushUnneededASanShadowMemory(reinterpret_cast(this), size); - UnmapOrDie(this, size); + uptr padded_size = size + kMaxStackFrameSize; + FlushUnneededASanShadowMemory(reinterpret_cast(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..50706e6e5876c 100644 --- a/compiler-rt/lib/asan/asan_fake_stack.h +++ b/compiler-rt/lib/asan/asan_fake_stack.h @@ -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 @@ -56,6 +56,9 @@ struct FakeFrame { class FakeStack { static const uptr kMinStackFrameSizeLog = 6; // Min frame is 64B. static const uptr kMaxStackFrameSizeLog = 16; // Max stack frame is 64K. + static_assert(kMaxStackFrameSizeLog >= kMinStackFrameSizeLog); + + static const u64 kMaxStackFrameSize = 1 << kMaxStackFrameSizeLog; public: static const uptr kNumberOfSizeClasses = @@ -66,7 +69,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); } @@ -110,6 +113,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 (otherwise you + // couldn't store a single frame of that size in the entire stack) + // hence (1<(this) + kFlagsOffset + SizeRequiredForFlags(stack_size_log) + @@ -156,15 +181,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(); 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..c60e2eadc35db 100644 --- a/compiler-rt/lib/asan/tests/asan_fake_stack_test.cpp +++ b/compiler-rt/lib/asan/tests/asan_fake_stack_test.cpp @@ -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(ff) % bytes_in_class, 0U); uptr x = reinterpret_cast(ff); EXPECT_TRUE(s.insert(std::make_pair(ff, cid)).second); EXPECT_EQ(x, fs->AddrIsInFakeStack(x)); diff --git a/compiler-rt/test/asan/TestCases/fakestack_alignment.cpp b/compiler-rt/test/asan/TestCases/fakestack_alignment.cpp index 01c073088694b..f052e3e8261d6 100644 --- a/compiler-rt/test/asan/TestCases/fakestack_alignment.cpp +++ b/compiler-rt/test/asan/TestCases/fakestack_alignment.cpp @@ -1,11 +1,11 @@ // Regression test 1: -// This deterministically fails: when the stack size is 1<<16, FakeStack's -// GetFrame() is out of alignment, because SizeRequiredForFlags(16) == 2K. +// When the stack size is 1<<16, SizeRequiredForFlags(16) == 2KB. This forces +// FakeStack's GetFrame() out of alignment if the FakeStack isn't padded. // RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=4096 -DTHREAD_COUNT=1 -DTHREAD_STACK_SIZE=65536 %s -o %t && %run %t 2>&1 // Regression test 2: -// The FakeStack frame is not guaranteed to be aligned, but alignment can -// happen by chance, so try this on many threads. +// Check that the FakeStack frame is aligned, beyond the typical 4KB page +// alignment. Alignment can happen by chance, so try this on many threads. // RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=8192 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 // RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=16384 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 @@ -17,8 +17,6 @@ // RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=8192 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 // RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=16384 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 -// XFAIL: * - #include #include #include From b195b7a9fbef3e8ef3af49d3b786b16f061f07b6 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Tue, 12 Aug 2025 05:44:04 +0000 Subject: [PATCH 2/3] Move to Posix, and add "-pthread" --- .../test/asan/TestCases/{ => Posix}/fakestack_alignment.cpp | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename compiler-rt/test/asan/TestCases/{ => Posix}/fakestack_alignment.cpp (100%) diff --git a/compiler-rt/test/asan/TestCases/fakestack_alignment.cpp b/compiler-rt/test/asan/TestCases/Posix/fakestack_alignment.cpp similarity index 100% rename from compiler-rt/test/asan/TestCases/fakestack_alignment.cpp rename to compiler-rt/test/asan/TestCases/Posix/fakestack_alignment.cpp From 1ffdb044722a8d1aaa62136b1bc105c997566c05 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Tue, 12 Aug 2025 15:24:56 +0000 Subject: [PATCH 3/3] Add -pthread --- .../TestCases/Posix/fakestack_alignment.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/compiler-rt/test/asan/TestCases/Posix/fakestack_alignment.cpp b/compiler-rt/test/asan/TestCases/Posix/fakestack_alignment.cpp index f052e3e8261d6..0ac963a857a18 100644 --- a/compiler-rt/test/asan/TestCases/Posix/fakestack_alignment.cpp +++ b/compiler-rt/test/asan/TestCases/Posix/fakestack_alignment.cpp @@ -1,21 +1,21 @@ // Regression test 1: // When the stack size is 1<<16, SizeRequiredForFlags(16) == 2KB. This forces // FakeStack's GetFrame() out of alignment if the FakeStack isn't padded. -// RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=4096 -DTHREAD_COUNT=1 -DTHREAD_STACK_SIZE=65536 %s -o %t && %run %t 2>&1 +// RUN: %clangxx_asan -pthread -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=4096 -DTHREAD_COUNT=1 -DTHREAD_STACK_SIZE=65536 %s -o %t && %run %t 2>&1 // Regression test 2: // Check that the FakeStack frame is aligned, beyond the typical 4KB page // alignment. Alignment can happen by chance, so try this on many threads. -// RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=8192 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 -// RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=16384 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 +// RUN: %clangxx_asan -pthread -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=8192 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 +// RUN: %clangxx_asan -pthread -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=16384 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 // Extra tests: -// RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=4096 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=65536 %s -o %t && %run %t 2>&1 -// RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=8192 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=65536 %s -o %t && %run %t 2>&1 -// RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=16384 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=65536 %s -o %t && %run %t 2>&1 -// RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=4096 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 -// RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=8192 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 -// RUN: %clangxx_asan -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=16384 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 +// RUN: %clangxx_asan -pthread -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=4096 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=65536 %s -o %t && %run %t 2>&1 +// RUN: %clangxx_asan -pthread -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=8192 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=65536 %s -o %t && %run %t 2>&1 +// RUN: %clangxx_asan -pthread -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=16384 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=65536 %s -o %t && %run %t 2>&1 +// RUN: %clangxx_asan -pthread -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=4096 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 +// RUN: %clangxx_asan -pthread -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=8192 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 +// RUN: %clangxx_asan -pthread -fsanitize-address-use-after-return=always -O0 -DALIGNMENT=16384 -DTHREAD_COUNT=32 -DTHREAD_STACK_SIZE=131072 %s -o %t && %run %t 2>&1 #include #include