Skip to content

Commit 6f415e0

Browse files
authored
Rollup merge of #140628 - joboet:async_signal_safe, r=Mark-Simulacrum
std: stop using TLS in signal handler TLS is not async-signal-safe, making its use in the signal handler used to detect stack overflows unsound (c.f. #133698). POSIX however lists two thread-specific identifiers that can be obtained in a signal handler: the current `pthread_t` and the address of `errno`. Since `pthread_equal` is not AS-safe, `pthread_t` should be considered opaque, so for our purposes, `&errno` is the only option. This however works nicely: we can use the address as a key into a map that stores information for each thread. This PR uses a `BTreeMap` protected by a spin lock to hold the guard page address and thread name and thus fixes #133698.
2 parents 2f0c6e2 + 2cdbd69 commit 6f415e0

File tree

2 files changed

+215
-58
lines changed

2 files changed

+215
-58
lines changed

library/std/src/sys/pal/unix/stack_overflow.rs

+86-58
Original file line numberDiff line numberDiff line change
@@ -25,15 +25,36 @@ impl Drop for Handler {
2525
}
2626
}
2727

28-
#[cfg(any(
29-
target_os = "linux",
30-
target_os = "freebsd",
31-
target_os = "hurd",
32-
target_os = "macos",
33-
target_os = "netbsd",
34-
target_os = "openbsd",
35-
target_os = "solaris",
36-
target_os = "illumos",
28+
#[cfg(all(
29+
not(miri),
30+
any(
31+
target_os = "linux",
32+
target_os = "freebsd",
33+
target_os = "hurd",
34+
target_os = "macos",
35+
target_os = "netbsd",
36+
target_os = "openbsd",
37+
target_os = "solaris",
38+
target_os = "illumos",
39+
),
40+
))]
41+
mod thread_info;
42+
43+
// miri doesn't model signals nor stack overflows and this code has some
44+
// synchronization properties that we don't want to expose to user code,
45+
// hence we disable it on miri.
46+
#[cfg(all(
47+
not(miri),
48+
any(
49+
target_os = "linux",
50+
target_os = "freebsd",
51+
target_os = "hurd",
52+
target_os = "macos",
53+
target_os = "netbsd",
54+
target_os = "openbsd",
55+
target_os = "solaris",
56+
target_os = "illumos",
57+
)
3758
))]
3859
mod imp {
3960
use libc::{
@@ -46,22 +67,13 @@ mod imp {
4667
use libc::{mmap64, mprotect, munmap};
4768

4869
use super::Handler;
49-
use crate::cell::Cell;
70+
use super::thread_info::{delete_current_info, set_current_info, with_current_info};
5071
use crate::ops::Range;
5172
use crate::sync::OnceLock;
5273
use crate::sync::atomic::{Atomic, AtomicBool, AtomicPtr, AtomicUsize, Ordering};
5374
use crate::sys::pal::unix::os;
54-
use crate::{io, mem, ptr, thread};
55-
56-
// We use a TLS variable to store the address of the guard page. While TLS
57-
// variables are not guaranteed to be signal-safe, this works out in practice
58-
// since we make sure to write to the variable before the signal stack is
59-
// installed, thereby ensuring that the variable is always allocated when
60-
// the signal handler is called.
61-
thread_local! {
62-
// FIXME: use `Range` once that implements `Copy`.
63-
static GUARD: Cell<(usize, usize)> = const { Cell::new((0, 0)) };
64-
}
75+
use crate::thread::with_current_name;
76+
use crate::{io, mem, panic, ptr};
6577

6678
// Signal handler for the SIGSEGV and SIGBUS handlers. We've got guard pages
6779
// (unmapped pages) at the end of every thread's stack, so if a thread ends
@@ -93,29 +105,35 @@ mod imp {
93105
info: *mut libc::siginfo_t,
94106
_data: *mut libc::c_void,
95107
) {
96-
let (start, end) = GUARD.get();
97108
// SAFETY: this pointer is provided by the system and will always point to a valid `siginfo_t`.
98-
let addr = unsafe { (*info).si_addr().addr() };
109+
let fault_addr = unsafe { (*info).si_addr().addr() };
110+
111+
// `with_current_info` expects that the process aborts after it is
112+
// called. If the signal was not caused by a memory access, this might
113+
// not be true. We detect this by noticing that the `si_addr` field is
114+
// zero if the signal is synthetic.
115+
if fault_addr != 0 {
116+
with_current_info(|thread_info| {
117+
// If the faulting address is within the guard page, then we print a
118+
// message saying so and abort.
119+
if let Some(thread_info) = thread_info
120+
&& thread_info.guard_page_range.contains(&fault_addr)
121+
{
122+
let name = thread_info.thread_name.as_deref().unwrap_or("<unknown>");
123+
rtprintpanic!("\nthread '{name}' has overflowed its stack\n");
124+
rtabort!("stack overflow");
125+
}
126+
})
127+
}
99128

100-
// If the faulting address is within the guard page, then we print a
101-
// message saying so and abort.
102-
if start <= addr && addr < end {
103-
thread::with_current_name(|name| {
104-
let name = name.unwrap_or("<unknown>");
105-
rtprintpanic!("\nthread '{name}' has overflowed its stack\n");
106-
});
129+
// Unregister ourselves by reverting back to the default behavior.
130+
// SAFETY: assuming all platforms define struct sigaction as "zero-initializable"
131+
let mut action: sigaction = unsafe { mem::zeroed() };
132+
action.sa_sigaction = SIG_DFL;
133+
// SAFETY: pray this is a well-behaved POSIX implementation of fn sigaction
134+
unsafe { sigaction(signum, &action, ptr::null_mut()) };
107135

108-
rtabort!("stack overflow");
109-
} else {
110-
// Unregister ourselves by reverting back to the default behavior.
111-
// SAFETY: assuming all platforms define struct sigaction as "zero-initializable"
112-
let mut action: sigaction = unsafe { mem::zeroed() };
113-
action.sa_sigaction = SIG_DFL;
114-
// SAFETY: pray this is a well-behaved POSIX implementation of fn sigaction
115-
unsafe { sigaction(signum, &action, ptr::null_mut()) };
116-
117-
// See comment above for why this function returns.
118-
}
136+
// See comment above for why this function returns.
119137
}
120138

121139
static PAGE_SIZE: Atomic<usize> = AtomicUsize::new(0);
@@ -128,9 +146,7 @@ mod imp {
128146
pub unsafe fn init() {
129147
PAGE_SIZE.store(os::page_size(), Ordering::Relaxed);
130148

131-
// Always write to GUARD to ensure the TLS variable is allocated.
132-
let guard = unsafe { install_main_guard().unwrap_or(0..0) };
133-
GUARD.set((guard.start, guard.end));
149+
let mut guard_page_range = unsafe { install_main_guard() };
134150

135151
// SAFETY: assuming all platforms define struct sigaction as "zero-initializable"
136152
let mut action: sigaction = unsafe { mem::zeroed() };
@@ -145,7 +161,13 @@ mod imp {
145161
let handler = unsafe { make_handler(true) };
146162
MAIN_ALTSTACK.store(handler.data, Ordering::Relaxed);
147163
mem::forget(handler);
164+
165+
if let Some(guard_page_range) = guard_page_range.take() {
166+
let thread_name = with_current_name(|name| name.map(Box::from));
167+
set_current_info(guard_page_range, thread_name);
168+
}
148169
}
170+
149171
action.sa_flags = SA_SIGINFO | SA_ONSTACK;
150172
action.sa_sigaction = signal_handler as sighandler_t;
151173
// SAFETY: only overriding signals if the default is set
@@ -214,9 +236,10 @@ mod imp {
214236
}
215237

216238
if !main_thread {
217-
// Always write to GUARD to ensure the TLS variable is allocated.
218-
let guard = unsafe { current_guard() }.unwrap_or(0..0);
219-
GUARD.set((guard.start, guard.end));
239+
if let Some(guard_page_range) = unsafe { current_guard() } {
240+
let thread_name = with_current_name(|name| name.map(Box::from));
241+
set_current_info(guard_page_range, thread_name);
242+
}
220243
}
221244

222245
// SAFETY: assuming stack_t is zero-initializable
@@ -261,6 +284,8 @@ mod imp {
261284
// a mapping that started one page earlier, so walk back a page and unmap from there.
262285
unsafe { munmap(data.sub(page_size), sigstack_size + page_size) };
263286
}
287+
288+
delete_current_info();
264289
}
265290

266291
/// Modern kernels on modern hardware can have dynamic signal stack sizes.
@@ -590,17 +615,20 @@ mod imp {
590615
// usually have fewer qualms about forwards compatibility, since the runtime
591616
// is shipped with the OS):
592617
// <https://github.com/apple/swift/blob/swift-5.10-RELEASE/stdlib/public/runtime/CrashHandlerMacOS.cpp>
593-
#[cfg(not(any(
594-
target_os = "linux",
595-
target_os = "freebsd",
596-
target_os = "hurd",
597-
target_os = "macos",
598-
target_os = "netbsd",
599-
target_os = "openbsd",
600-
target_os = "solaris",
601-
target_os = "illumos",
602-
target_os = "cygwin",
603-
)))]
618+
#[cfg(any(
619+
miri,
620+
not(any(
621+
target_os = "linux",
622+
target_os = "freebsd",
623+
target_os = "hurd",
624+
target_os = "macos",
625+
target_os = "netbsd",
626+
target_os = "openbsd",
627+
target_os = "solaris",
628+
target_os = "illumos",
629+
target_os = "cygwin",
630+
))
631+
))]
604632
mod imp {
605633
pub unsafe fn init() {}
606634

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
//! TLS, but async-signal-safe.
2+
//!
3+
//! Unfortunately, because thread local storage isn't async-signal-safe, we
4+
//! cannot soundly use it in our stack overflow handler. While this works
5+
//! without problems on most platforms, it can lead to undefined behaviour
6+
//! on others (such as GNU/Linux). Luckily, the POSIX specification documents
7+
//! two thread-specific values that can be accessed in asynchronous signal
8+
//! handlers: the value of `pthread_self()` and the address of `errno`. As
9+
//! `pthread_t` is an opaque platform-specific type, we use the address of
10+
//! `errno` here. As it is thread-specific and does not change over the
11+
//! lifetime of a thread, we can use `&errno` as a key for a `BTreeMap`
12+
//! that stores thread-specific data.
13+
//!
14+
//! Concurrent access to this map is synchronized by two locks – an outer
15+
//! [`Mutex`] and an inner spin lock that also remembers the identity of
16+
//! the lock owner:
17+
//! * The spin lock is the primary means of synchronization: since it only
18+
//! uses native atomics, it can be soundly used inside the signal handle
19+
//! as opposed to [`Mutex`], which might not be async-signal-safe.
20+
//! * The [`Mutex`] prevents busy-waiting in the setup logic, as all accesses
21+
//! there are performed with the [`Mutex`] held, which makes the spin-lock
22+
//! redundant in the common case.
23+
//! * Finally, by using the `errno` address as the locked value of the spin
24+
//! lock, we can detect cases where a SIGSEGV occurred while the thread
25+
//! info is being modified.
26+
27+
use crate::collections::BTreeMap;
28+
use crate::hint::spin_loop;
29+
use crate::ops::Range;
30+
use crate::sync::Mutex;
31+
use crate::sync::atomic::{AtomicUsize, Ordering};
32+
use crate::sys::os::errno_location;
33+
34+
pub struct ThreadInfo {
35+
pub guard_page_range: Range<usize>,
36+
pub thread_name: Option<Box<str>>,
37+
}
38+
39+
static LOCK: Mutex<()> = Mutex::new(());
40+
static SPIN_LOCK: AtomicUsize = AtomicUsize::new(0);
41+
// This uses a `BTreeMap` instead of a hashmap since it supports constant
42+
// initialization and automatically reduces the amount of memory used when
43+
// items are removed.
44+
static mut THREAD_INFO: BTreeMap<usize, ThreadInfo> = BTreeMap::new();
45+
46+
struct UnlockOnDrop;
47+
48+
impl Drop for UnlockOnDrop {
49+
fn drop(&mut self) {
50+
SPIN_LOCK.store(0, Ordering::Release);
51+
}
52+
}
53+
54+
/// Get the current thread's information, if available.
55+
///
56+
/// Calling this function might freeze other threads if they attempt to modify
57+
/// their thread information. Thus, the caller should ensure that the process
58+
/// is aborted shortly after this function is called.
59+
///
60+
/// This function is guaranteed to be async-signal-safe if `f` is too.
61+
pub fn with_current_info<R>(f: impl FnOnce(Option<&ThreadInfo>) -> R) -> R {
62+
let this = errno_location().addr();
63+
let mut attempt = 0;
64+
let _guard = loop {
65+
// If we are just spinning endlessly, it's very likely that the thread
66+
// modifying the thread info map has a lower priority than us and will
67+
// not continue until we stop running. Just give up in that case.
68+
if attempt == 10_000_000 {
69+
rtprintpanic!("deadlock in SIGSEGV handler");
70+
return f(None);
71+
}
72+
73+
match SPIN_LOCK.compare_exchange(0, this, Ordering::Acquire, Ordering::Relaxed) {
74+
Ok(_) => break UnlockOnDrop,
75+
Err(owner) if owner == this => {
76+
rtabort!("a thread received SIGSEGV while modifying its stack overflow information")
77+
}
78+
// Spin until the lock can be acquired – there is nothing better to
79+
// do. This is unfortunately a priority hole, but a stack overflow
80+
// is a fatal error anyway.
81+
Err(_) => {
82+
spin_loop();
83+
attempt += 1;
84+
}
85+
}
86+
};
87+
88+
// SAFETY: we own the spin lock, so `THREAD_INFO` cannot not be aliased.
89+
let thread_info = unsafe { &*(&raw const THREAD_INFO) };
90+
f(thread_info.get(&this))
91+
}
92+
93+
fn spin_lock_in_setup(this: usize) -> UnlockOnDrop {
94+
loop {
95+
match SPIN_LOCK.compare_exchange(0, this, Ordering::Acquire, Ordering::Relaxed) {
96+
Ok(_) => return UnlockOnDrop,
97+
Err(owner) if owner == this => {
98+
unreachable!("the thread info setup logic isn't recursive")
99+
}
100+
// This function is always called with the outer lock held,
101+
// meaning the only time locking can fail is if another thread has
102+
// encountered a stack overflow. Since that will abort the process,
103+
// we just stop the current thread until that time. We use `pause`
104+
// instead of spinning to avoid priority inversion.
105+
// SAFETY: this doesn't have any safety preconditions.
106+
Err(_) => drop(unsafe { libc::pause() }),
107+
}
108+
}
109+
}
110+
111+
pub fn set_current_info(guard_page_range: Range<usize>, thread_name: Option<Box<str>>) {
112+
let this = errno_location().addr();
113+
let _lock_guard = LOCK.lock();
114+
let _spin_guard = spin_lock_in_setup(this);
115+
116+
// SAFETY: we own the spin lock, so `THREAD_INFO` cannot be aliased.
117+
let thread_info = unsafe { &mut *(&raw mut THREAD_INFO) };
118+
thread_info.insert(this, ThreadInfo { guard_page_range, thread_name });
119+
}
120+
121+
pub fn delete_current_info() {
122+
let this = errno_location().addr();
123+
let _lock_guard = LOCK.lock();
124+
let _spin_guard = spin_lock_in_setup(this);
125+
126+
// SAFETY: we own the spin lock, so `THREAD_INFO` cannot not be aliased.
127+
let thread_info = unsafe { &mut *(&raw mut THREAD_INFO) };
128+
thread_info.remove(&this);
129+
}

0 commit comments

Comments
 (0)