[compiler-rt] [compiler-rt] Hardened altsigstack unmap fix from PR #179000 (PR #196460)
via llvm-commits
llvm-commits at lists.llvm.org
Thu May 7 19:21:11 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-compiler-rt-sanitizer
Author: accauble
<details>
<summary>Changes</summary>
This is an extension of PR #<!-- -->179000, which was approved a few weeks ago, but hasn't been able to be merged due to formatting issues.
Before the PR, the sanitizer assumed that the registered alternate signal stack was the same one that it allocated which posed a problem when other components (like LLVM) registered their own alternate signal stack. The fix made by the PR was to save the allocated pointer and only unmap if the registered alternate signal stack
I made an almost identical fix to @<!-- -->ilovepi before realizing the PR existed. There were a couple differences that feel worthy of adding, so I have integrated my changes into the changes made by PR #<!-- -->179000. The differences are:
* The original only unmapped the region if the pointer was still the registered alternate signal stack. This changes it to unmapping the region no matter what, since it was the sanitizer that mmapped it.
* The `UnmapOrDie` function requires the size of the region which can no longer be guaranteed by registered sigaltstack size. So instead of having `SetAlternateSignalStack` return the new stack pointer, it now returns a struct with the pointer and the size.
* The original disabled the alternate signal stack no matter what. This changes it so that it is only disabled if it the alternate signal stack registered by the sanitizer. Otherwise, we could be disabling another component's alt sig stack.
* I added the reproducer (created with AI) when I found the bug. It's very similar to @<!-- -->ilovepi's test, but it changes the size of the alt sig stack.
---
Full diff: https://github.com/llvm/llvm-project/pull/196460.diff
8 Files Affected:
- (modified) compiler-rt/lib/asan/asan_thread.cpp (+2-2)
- (modified) compiler-rt/lib/asan/asan_thread.h (+1)
- (modified) compiler-rt/lib/sanitizer_common/sanitizer_common.h (+6-2)
- (modified) compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp (+2-2)
- (modified) compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp (+28-11)
- (modified) compiler-rt/lib/sanitizer_common/sanitizer_win.cpp (+3-2)
- (added) compiler-rt/test/asan/TestCases/Posix/multiple_sigaltstack.cpp (+24)
- (added) compiler-rt/test/asan/TestCases/Posix/sigaltstack-replaced-by-app.cpp (+79)
``````````diff
diff --git a/compiler-rt/lib/asan/asan_thread.cpp b/compiler-rt/lib/asan/asan_thread.cpp
index 32ab723e89001..e4fc1aabd1c80 100644
--- a/compiler-rt/lib/asan/asan_thread.cpp
+++ b/compiler-rt/lib/asan/asan_thread.cpp
@@ -132,7 +132,7 @@ void AsanThread::Destroy() {
CHECK_EQ(this, thread);
malloc_storage().CommitBack();
if (common_flags()->use_sigaltstack)
- UnsetAlternateSignalStack();
+ UnsetAlternateSignalStack(altstack_);
FlushToDeadThreadStats(&stats_);
// We also clear the shadow on thread destruction because
// some code may still be executing in later TSD destructors
@@ -288,7 +288,7 @@ void AsanThread::ThreadStart(ThreadID os_id) {
asanThreadRegistry().StartThread(tid(), os_id, ThreadType::Regular, nullptr);
if (common_flags()->use_sigaltstack)
- SetAlternateSignalStack();
+ altstack_ = SetAlternateSignalStack();
}
AsanThread *CreateMainThread() {
diff --git a/compiler-rt/lib/asan/asan_thread.h b/compiler-rt/lib/asan/asan_thread.h
index e9ca6b6a59016..39a1c1665ad3e 100644
--- a/compiler-rt/lib/asan/asan_thread.h
+++ b/compiler-rt/lib/asan/asan_thread.h
@@ -190,6 +190,7 @@ class AsanThread {
AsanStats stats_;
bool unwinding_;
uptr extra_spill_area_;
+ AlternateSignalStack altstack_ = {nullptr, 0};
char start_data_[];
};
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common.h b/compiler-rt/lib/sanitizer_common/sanitizer_common.h
index 4dd2187df2272..996d34ff505e6 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_common.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_common.h
@@ -387,8 +387,12 @@ void ReportDeadlySignal(const SignalContext &sig, u32 tid,
const void *unwind_context);
// Alternative signal stack (POSIX-only).
-void SetAlternateSignalStack();
-void UnsetAlternateSignalStack();
+struct AlternateSignalStack {
+ void *sp;
+ uptr size;
+};
+AlternateSignalStack SetAlternateSignalStack();
+void UnsetAlternateSignalStack(AlternateSignalStack altstack);
bool IsSignalHandlerFromSanitizer(int signum);
bool SetSignalHandlerFromSanitizer(int signum, bool new_state);
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp
index 3c61b60802996..edc6dae9dd797 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_fuchsia.cpp
@@ -93,8 +93,8 @@ void CheckMPROTECT() {}
void PlatformPrepareForSandboxing(void *args) {}
void DisableCoreDumperIfNecessary() {}
void InstallDeadlySignalHandlers(SignalHandlerType handler) {}
-void SetAlternateSignalStack() {}
-void UnsetAlternateSignalStack() {}
+AlternateSignalStack SetAlternateSignalStack() { return {nullptr, 0}; }
+void UnsetAlternateSignalStack(AlternateSignalStack altstack) {}
bool SignalContext::IsStackOverflow() const { return false; }
void SignalContext::DumpAllRegisters(void *context) { UNIMPLEMENTED(); }
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
index 35b596de30fff..a3c9eb22ddde3 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
@@ -188,12 +188,13 @@ static uptr GetAltStackSize() {
return SIGSTKSZ * 4;
}
-void SetAlternateSignalStack() {
+AlternateSignalStack SetAlternateSignalStack() {
stack_t altstack, oldstack;
CHECK_EQ(0, sigaltstack(nullptr, &oldstack));
// If the alternate stack is already in place, do nothing.
// Android always sets an alternate stack, but it's too small for us.
- if (!SANITIZER_ANDROID && !(oldstack.ss_flags & SS_DISABLE)) return;
+ if (!SANITIZER_ANDROID && !(oldstack.ss_flags & SS_DISABLE))
+ return {nullptr, 0};
// TODO(glider): the mapped stack should have the MAP_STACK flag in the
// future. It is not required by man 2 sigaltstack now (they're using
// malloc()).
@@ -201,15 +202,31 @@ void SetAlternateSignalStack() {
altstack.ss_sp = (char *)MmapOrDie(altstack.ss_size, __func__);
altstack.ss_flags = 0;
CHECK_EQ(0, sigaltstack(&altstack, nullptr));
-}
-
-void UnsetAlternateSignalStack() {
- stack_t altstack, oldstack;
- altstack.ss_sp = nullptr;
- altstack.ss_flags = SS_DISABLE;
- altstack.ss_size = GetAltStackSize(); // Some sane value required on Darwin.
- CHECK_EQ(0, sigaltstack(&altstack, &oldstack));
- UnmapOrDie(oldstack.ss_sp, oldstack.ss_size);
+ return {altstack.ss_sp, altstack.ss_size};
+}
+
+void UnsetAlternateSignalStack(AlternateSignalStack altstack_handle) {
+ if (!altstack_handle.sp)
+ return;
+ // Only un-register the alt stack with the kernel if it still points at the
+ // buffer we installed. Another component (e.g. CreateSigAltStack in llvm)
+ // may have replaced our registration; SS_DISABLE'ing in that case would
+ // silently break their signal handling. Our buffer is always safe to
+ // unmap because the kernel no longer references it once it has been
+ // replaced.
+ stack_t current;
+ CHECK_EQ(0, sigaltstack(nullptr, ¤t));
+ bool still_ours = !(current.ss_flags & SS_DISABLE) &&
+ current.ss_sp == altstack_handle.sp &&
+ current.ss_size == altstack_handle.size;
+ if (still_ours) {
+ stack_t altstack;
+ altstack.ss_sp = nullptr;
+ altstack.ss_flags = SS_DISABLE;
+ altstack.ss_size = GetAltStackSize(); // Sane value required on Darwin.
+ CHECK_EQ(0, sigaltstack(&altstack, nullptr));
+ }
+ UnmapOrDie(altstack_handle.sp, altstack_handle.size);
}
bool IsSignalHandlerFromSanitizer(int signum) {
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
index ed4f60deeffc8..7ff470f0d9d63 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
@@ -913,11 +913,12 @@ void ReportFile::Write(const char *buffer, uptr length) {
}
}
-void SetAlternateSignalStack() {
+AlternateSignalStack SetAlternateSignalStack() {
// FIXME: Decide what to do on Windows.
+ return {nullptr, 0};
}
-void UnsetAlternateSignalStack() {
+void UnsetAlternateSignalStack(AlternateSignalStack altstack) {
// FIXME: Decide what to do on Windows.
}
diff --git a/compiler-rt/test/asan/TestCases/Posix/multiple_sigaltstack.cpp b/compiler-rt/test/asan/TestCases/Posix/multiple_sigaltstack.cpp
new file mode 100644
index 0000000000000..a7cf4b3a43b91
--- /dev/null
+++ b/compiler-rt/test/asan/TestCases/Posix/multiple_sigaltstack.cpp
@@ -0,0 +1,24 @@
+// RUN: %clangxx_asan %s -o %t && %env_asan_opts=use_sigaltstack=1 %run %t
+
+#include <pthread.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+char global_alt_stack[4096 * 4];
+
+int main() {
+ stack_t altstack;
+ altstack.ss_sp = global_alt_stack;
+ altstack.ss_size = sizeof(global_alt_stack);
+ altstack.ss_flags = 0;
+ if (sigaltstack(&altstack, nullptr) != 0) {
+ perror("sigaltstack");
+ exit(1);
+ }
+
+ // UnsetAlternateSignalStack will get called when the thread exists. If we
+ // don't *only* unmap a signal stack the runtime owns, we'll get a fault on
+ // the munmap operation, since that memory isn't mmaped.
+ return 0;
+}
diff --git a/compiler-rt/test/asan/TestCases/Posix/sigaltstack-replaced-by-app.cpp b/compiler-rt/test/asan/TestCases/Posix/sigaltstack-replaced-by-app.cpp
new file mode 100644
index 0000000000000..c782762aa3f1d
--- /dev/null
+++ b/compiler-rt/test/asan/TestCases/Posix/sigaltstack-replaced-by-app.cpp
@@ -0,0 +1,79 @@
+// Regression test for UnsetAlternateSignalStack: when another component
+// installs its own alternate signal stack on top of ASan's (e.g.
+// llvm/lib/Support/Unix/Signals.inc CreateSigAltStack does this when it
+// finds ASan's stack too small), ASan's per-thread destructor must not
+// try to free the replacement. ASan does not own that buffer; calling
+// UnmapOrDie on it crashes the process at thread teardown:
+//
+// AsanThread::Destroy
+// -> UnsetAlternateSignalStack
+// sigaltstack(SS_DISABLE, &oldstack) // returns *current* stack
+// UnmapOrDie(oldstack.ss_sp, oldstack.ss_size)
+// munmap fails (the pointer was not allocated by mmap, is
+// mid-VMA, or is not page aligned)
+// -> ReportMunmapFailureAndDie -> CHECK fail -> abort
+//
+// Reproducer:
+// * A worker thread replaces ASan's alt-stack registration with an
+// mmap'd buffer whose ss_sp is intentionally not page aligned.
+// sigaltstack() accepts misaligned pointers; munmap() does not, so
+// ASan's eventual munmap fails deterministically with EINVAL.
+// (Without the misalignment, whether the bug fires depends on VMA
+// fragmentation, which is not reliable in a self-contained test.)
+// * Returning from the worker triggers __nptl_deallocate_tsd, which
+// runs ASan's per-thread destructor and reaches the bad path.
+// main()'s thread does not go through __nptl_deallocate_tsd on
+// process exit, which is why this only manifests with a spawned
+// thread.
+//
+// LeakSanitizer doesn't track raw mmap regions, so the intentionally
+// leaked alt-stack mapping does not require detect_leaks=0.
+//
+// RUN: %clangxx_asan -O0 %s -pthread -o %t && %run %t
+
+#include <pthread.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+static void *worker(void *) {
+ size_t pagesize = sysconf(_SC_PAGESIZE);
+ size_t mapped_size = 32 * pagesize;
+ char *region =
+ static_cast<char *>(mmap(nullptr, mapped_size, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, -1, 0));
+ if (region == MAP_FAILED) {
+ perror("mmap");
+ abort();
+ }
+
+ // ss_sp deliberately not page aligned: see file header.
+ stack_t new_stack = {};
+ new_stack.ss_sp = region + 1;
+ new_stack.ss_size = mapped_size - 1;
+ new_stack.ss_flags = 0;
+ if (sigaltstack(&new_stack, nullptr) != 0) {
+ perror("sigaltstack");
+ abort();
+ }
+
+ // Returning here drives ASan's per-thread destructor, which is where
+ // the bug used to abort.
+ return nullptr;
+}
+
+int main() {
+ pthread_t t;
+ if (pthread_create(&t, nullptr, worker, nullptr) != 0) {
+ perror("pthread_create");
+ return 1;
+ }
+ if (pthread_join(t, nullptr) != 0) {
+ perror("pthread_join");
+ return 1;
+ }
+ printf("OK\n");
+ return 0;
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/196460
More information about the llvm-commits
mailing list