From 9e354daf7b3ef64740fe2ecf3c5aae9563b68189 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 16 Jul 2024 19:31:23 -0700 Subject: [PATCH 1/5] unix: Unsafe-wrap stack_overflow::signal_handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sometimes a safety comment is a prayer. avoid fuzzy provenance casts after deref. Co-authored-by: Jonas Böttiger --- library/std/src/sys/pal/unix/stack_overflow.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/library/std/src/sys/pal/unix/stack_overflow.rs b/library/std/src/sys/pal/unix/stack_overflow.rs index 2e5bd85327a..0886624160c 100644 --- a/library/std/src/sys/pal/unix/stack_overflow.rs +++ b/library/std/src/sys/pal/unix/stack_overflow.rs @@ -86,13 +86,18 @@ mod imp { // out many large systems and all implementations allow returning from a // signal handler to work. For a more detailed explanation see the // comments on #26458. + /// SIGSEGV/SIGBUS entry point + /// # Safety + /// Rust doesn't call this, it *gets called*. + #[forbid(unsafe_op_in_unsafe_fn)] unsafe extern "C" fn signal_handler( signum: libc::c_int, info: *mut libc::siginfo_t, _data: *mut libc::c_void, ) { let (start, end) = GUARD.get(); - let addr = (*info).si_addr() as usize; + // SAFETY: this pointer is provided by the system and will always point to a valid `siginfo_t`. + let addr = unsafe { (*info).si_addr().addr() }; // If the faulting address is within the guard page, then we print a // message saying so and abort. @@ -104,9 +109,11 @@ mod imp { rtabort!("stack overflow"); } else { // Unregister ourselves by reverting back to the default behavior. - let mut action: sigaction = mem::zeroed(); + // SAFETY: assuming all platforms define struct sigaction as "zero-initializable" + let mut action: sigaction = unsafe { mem::zeroed() }; action.sa_sigaction = SIG_DFL; - sigaction(signum, &action, ptr::null_mut()); + // SAFETY: pray this is a well-behaved POSIX implementation of fn sigaction + unsafe { sigaction(signum, &action, ptr::null_mut()) }; // See comment above for why this function returns. } From 357ba1f8ec0326b9f285224fee8a97691485475f Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 16 Jul 2024 20:19:16 -0700 Subject: [PATCH 2/5] unix: lift init of sigaltstack before sigaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is technically "not necessary", as we will "just" segfault instead if we e.g. arrive inside the handler fn with the null altstack. However, it seems incorrect to go about this hoping that segfaulting is okay, seeing as how our purpose here is to mitigate stack overflow problems. Make sure NEED_ALTSTACK syncs with PAGE_SIZE when we do. Co-authored-by: Jonas Böttiger --- .../std/src/sys/pal/unix/stack_overflow.rs | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/library/std/src/sys/pal/unix/stack_overflow.rs b/library/std/src/sys/pal/unix/stack_overflow.rs index 0886624160c..160e58e16a5 100644 --- a/library/std/src/sys/pal/unix/stack_overflow.rs +++ b/library/std/src/sys/pal/unix/stack_overflow.rs @@ -123,28 +123,36 @@ mod imp { static MAIN_ALTSTACK: AtomicPtr = AtomicPtr::new(ptr::null_mut()); static NEED_ALTSTACK: AtomicBool = AtomicBool::new(false); + /// # Safety + /// Must be called only once + #[forbid(unsafe_op_in_unsafe_fn)] pub unsafe fn init() { PAGE_SIZE.store(os::page_size(), Ordering::Relaxed); // Always write to GUARD to ensure the TLS variable is allocated. - let guard = install_main_guard().unwrap_or(0..0); + let guard = unsafe { install_main_guard().unwrap_or(0..0) }; GUARD.set((guard.start, guard.end)); - let mut action: sigaction = mem::zeroed(); + // SAFETY: assuming all platforms define struct sigaction as "zero-initializable" + let mut action: sigaction = unsafe { mem::zeroed() }; for &signal in &[SIGSEGV, SIGBUS] { - sigaction(signal, ptr::null_mut(), &mut action); + // SAFETY: just fetches the current signal handler into action + unsafe { sigaction(signal, ptr::null_mut(), &mut action) }; // Configure our signal handler if one is not already set. if action.sa_sigaction == SIG_DFL { + if !NEED_ALTSTACK.load(Ordering::Relaxed) { + // haven't set up our sigaltstack yet + NEED_ALTSTACK.store(true, Ordering::Release); + let handler = unsafe { make_handler(true) }; + MAIN_ALTSTACK.store(handler.data, Ordering::Relaxed); + mem::forget(handler); + } action.sa_flags = SA_SIGINFO | SA_ONSTACK; action.sa_sigaction = signal_handler as sighandler_t; - sigaction(signal, &action, ptr::null_mut()); - NEED_ALTSTACK.store(true, Ordering::Relaxed); + // SAFETY: only overriding signals if the default is set + unsafe { sigaction(signal, &action, ptr::null_mut()) }; } } - - let handler = make_handler(true); - MAIN_ALTSTACK.store(handler.data, Ordering::Relaxed); - mem::forget(handler); } pub unsafe fn cleanup() { From fa628ceaff4e24fad699cc651cc6e7d521b642a9 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 16 Jul 2024 21:07:44 -0700 Subject: [PATCH 3/5] unix: Unsafe-wrap stack_overflow::cleanup Editorialize on the wisdom of this as we do. --- library/std/src/sys/pal/unix/stack_overflow.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/library/std/src/sys/pal/unix/stack_overflow.rs b/library/std/src/sys/pal/unix/stack_overflow.rs index 160e58e16a5..e016f3b3ca4 100644 --- a/library/std/src/sys/pal/unix/stack_overflow.rs +++ b/library/std/src/sys/pal/unix/stack_overflow.rs @@ -155,8 +155,13 @@ mod imp { } } + /// # Safety + /// Must be called only once + #[forbid(unsafe_op_in_unsafe_fn)] pub unsafe fn cleanup() { - drop_handler(MAIN_ALTSTACK.load(Ordering::Relaxed)); + // FIXME: I probably cause more bugs than I'm worth! + // see https://github.com/rust-lang/rust/issues/111272 + unsafe { drop_handler(MAIN_ALTSTACK.load(Ordering::Relaxed)) }; } unsafe fn get_stack() -> libc::stack_t { From c1740eee1ea8dc046f9195d7578d3079ef423f7b Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Tue, 16 Jul 2024 21:08:06 -0700 Subject: [PATCH 4/5] unix: Unsafe-wrap stack_overflow::{drop,make}_handler Note that current_guard is probably not unsafe for future work. --- .../std/src/sys/pal/unix/stack_overflow.rs | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/library/std/src/sys/pal/unix/stack_overflow.rs b/library/std/src/sys/pal/unix/stack_overflow.rs index e016f3b3ca4..3944e4e1b48 100644 --- a/library/std/src/sys/pal/unix/stack_overflow.rs +++ b/library/std/src/sys/pal/unix/stack_overflow.rs @@ -206,6 +206,9 @@ mod imp { libc::stack_t { ss_sp: stackp, ss_flags: 0, ss_size: sigstack_size } } + /// # Safety + /// Mutates the alternate signal stack + #[forbid(unsafe_op_in_unsafe_fn)] pub unsafe fn make_handler(main_thread: bool) -> Handler { if !NEED_ALTSTACK.load(Ordering::Relaxed) { return Handler::null(); @@ -213,27 +216,38 @@ mod imp { if !main_thread { // Always write to GUARD to ensure the TLS variable is allocated. - let guard = current_guard().unwrap_or(0..0); + let guard = unsafe { current_guard() }.unwrap_or(0..0); GUARD.set((guard.start, guard.end)); } - let mut stack = mem::zeroed(); - sigaltstack(ptr::null(), &mut stack); + // SAFETY: assuming stack_t is zero-initializable + let mut stack = unsafe { mem::zeroed() }; + // SAFETY: reads current stack_t into stack + unsafe { sigaltstack(ptr::null(), &mut stack) }; // Configure alternate signal stack, if one is not already set. if stack.ss_flags & SS_DISABLE != 0 { - stack = get_stack(); - sigaltstack(&stack, ptr::null_mut()); + // SAFETY: We warned our caller this would happen! + unsafe { + stack = get_stack(); + sigaltstack(&stack, ptr::null_mut()); + } Handler { data: stack.ss_sp as *mut libc::c_void } } else { Handler::null() } } + /// # Safety + /// Must be called + /// - only with our handler or nullptr + /// - only when done with our altstack + /// This disables the alternate signal stack! + #[forbid(unsafe_op_in_unsafe_fn)] pub unsafe fn drop_handler(data: *mut libc::c_void) { if !data.is_null() { let sigstack_size = sigstack_size(); let page_size = PAGE_SIZE.load(Ordering::Relaxed); - let stack = libc::stack_t { + let disabling_stack = libc::stack_t { ss_sp: ptr::null_mut(), ss_flags: SS_DISABLE, // Workaround for bug in macOS implementation of sigaltstack @@ -242,10 +256,11 @@ mod imp { // both ss_sp and ss_size should be ignored in this case. ss_size: sigstack_size, }; - sigaltstack(&stack, ptr::null_mut()); - // We know from `get_stackp` that the alternate stack we installed is part of a mapping - // that started one page earlier, so walk back a page and unmap from there. - munmap(data.sub(page_size), sigstack_size + page_size); + // SAFETY: we warned the caller this disables the alternate signal stack! + unsafe { sigaltstack(&disabling_stack, ptr::null_mut()) }; + // SAFETY: We know from `get_stackp` that the alternate stack we installed is part of + // a mapping that started one page earlier, so walk back a page and unmap from there. + unsafe { munmap(data.sub(page_size), sigstack_size + page_size) }; } } @@ -446,6 +461,7 @@ mod imp { } #[cfg(any(target_os = "macos", target_os = "openbsd", target_os = "solaris"))] + // FIXME: I am probably not unsafe. unsafe fn current_guard() -> Option> { let stackptr = get_stack_start()?; let stackaddr = stackptr.addr(); @@ -460,6 +476,7 @@ mod imp { target_os = "netbsd", target_os = "l4re" ))] + // FIXME: I am probably not unsafe. unsafe fn current_guard() -> Option> { let mut ret = None; let mut attr: libc::pthread_attr_t = crate::mem::zeroed(); From 529fcbcd6d7c72e942f0e70fefb7db182338ee43 Mon Sep 17 00:00:00 2001 From: Jubilee <46493976+workingjubilee@users.noreply.github.com> Date: Thu, 18 Jul 2024 15:24:40 -0700 Subject: [PATCH 5/5] unix: acquire-load NEED_ALTSTACK MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jonas Böttiger --- library/std/src/sys/pal/unix/stack_overflow.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/pal/unix/stack_overflow.rs b/library/std/src/sys/pal/unix/stack_overflow.rs index 3944e4e1b48..047edebb861 100644 --- a/library/std/src/sys/pal/unix/stack_overflow.rs +++ b/library/std/src/sys/pal/unix/stack_overflow.rs @@ -210,7 +210,7 @@ mod imp { /// Mutates the alternate signal stack #[forbid(unsafe_op_in_unsafe_fn)] pub unsafe fn make_handler(main_thread: bool) -> Handler { - if !NEED_ALTSTACK.load(Ordering::Relaxed) { + if !NEED_ALTSTACK.load(Ordering::Acquire) { return Handler::null(); }