Auto merge of #31333 - lambda:31273-abort-on-stack-overflow, r=brson

Abort on stack overflow instead of re-raising SIGSEGV

We use guard pages that cause the process to abort to protect against
undefined behavior in the event of stack overflow.  We have a handler
that catches segfaults, prints out an error message if the segfault was
due to a stack overflow, then unregisters itself and returns to allow
the signal to be re-raised and kill the process.

This caused some confusion, as it was unexpected that safe code would be
able to cause a segfault, while it's easy to overflow the stack in safe
code.  To avoid this confusion, when we detect a segfault in the guard
page, abort instead of the previous behavior of re-raising SIGSEGV.

To test this, we need to adapt the tests for segfault to actually check
the exit status.  Doing so revealed that the existing test for segfault
behavior was actually invalid; LLVM optimizes the explicit null pointer
reference down to an illegal instruction, so the program aborts with
SIGILL instead of SIGSEGV and the test didn't actually trigger the
signal handler at all.  Use a C helper function to get a null pointer
that LLVM can't optimize away, so we get our segfault instead.

This is a [breaking-change] if anyone is relying on the exact signal
raised to kill a process on stack overflow.

Closes #31273

Author: bors

src/libstd/sys/unix/stack_overflow.rs

@@ -84,16 +84,19 @@ mod imp {
     // were originally supposed to do.
     //
     // This handler currently exists purely to print an informative message
-    // whenever a thread overflows its stack. When run the handler always
-    // un-registers itself after running and then returns (to allow the original
-    // signal to be delivered again). By returning we're ensuring that segfaults
-    // do indeed look like segfaults.
+    // whenever a thread overflows its stack. We then abort to exit and
+    // indicate a crash, but to avoid a misleading SIGSEGV that might lead
+    // users to believe that unsafe code has accessed an invalid pointer; the
+    // SIGSEGV encountered when overflowing the stack is expected and
+    // well-defined.
     //
-    // Returning from this kind of signal handler is technically not defined to
-    // work when reading the POSIX spec strictly, but in practice it turns 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.
+    // If this is not a stack overflow, the handler un-registers itself and
+    // then returns (to allow the original signal to be delivered again).
+    // Returning from this kind of signal handler is technically not defined
+    // to work when reading the POSIX spec strictly, but in practice it turns
+    // 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.
     unsafe extern fn signal_handler(signum: libc::c_int,
                                     info: *mut libc::siginfo_t,
                                     _data: *mut libc::c_void) {
@@ -103,17 +106,18 @@ mod imp {
         let addr = siginfo_si_addr(info);
 
         // If the faulting address is within the guard page, then we print a
-        // message saying so.
+        // message saying so and abort.
         if guard != 0 && guard - PAGE_SIZE <= addr && addr < guard {
             report_overflow();
+            rtabort!("stack overflow");
+        } else {
+            // Unregister ourselves by reverting back to the default behavior.
+            let mut action: sigaction = mem::zeroed();
+            action.sa_sigaction = SIG_DFL;
+            sigaction(signum, &action, ptr::null_mut());
+
+            // See comment above for why this function returns.
         }
-
-        // Unregister ourselves by reverting back to the default behavior.
-        let mut action: sigaction = mem::zeroed();
-        action.sa_sigaction = SIG_DFL;
-        sigaction(signum, &action, ptr::null_mut());
-
-        // See comment above for why this function returns.
     }
 
     static mut MAIN_ALTSTACK: *mut libc::c_void = ptr::null_mut();

src/rt/rust_test_helpers.c

@@ -158,6 +158,11 @@ rust_get_test_int() {
   return 1;
 }
 
+char *
+rust_get_null_ptr() {
+    return 0;
+}
+
 /* Debug helpers strictly to verify ABI conformance.
  *
  * FIXME (#2665): move these into a testcase when the testsuite

src/test/run-pass/out-of-stack.rs

@@ -12,6 +12,10 @@
 // ignore-musl
 
 #![feature(asm)]
+#![feature(libc)]
+
+#[cfg(unix)]
+extern crate libc;
 
 use std::env;
 use std::process::Command;
@@ -35,6 +39,24 @@ fn loud_recurse() {
     black_box(()); // don't optimize this into a tail call. please.
 }
 
+#[cfg(unix)]
+fn check_status(status: std::process::ExitStatus)
+{
+    use libc;
+    use std::os::unix::process::ExitStatusExt;
+
+    assert!(!status.success());
+    assert!(status.signal() != Some(libc::SIGSEGV)
+            && status.signal() != Some(libc::SIGBUS));
+}
+
+#[cfg(not(unix))]
+fn check_status(status: std::process::ExitStatus)
+{
+    assert!(!status.success());
+}
+
+
 fn main() {
     let args: Vec<String> = env::args().collect();
     if args.len() > 1 && args[1] == "silent" {
@@ -62,7 +84,9 @@ fn main() {
             println!("testing: {}", mode);
 
             let silent = Command::new(&args[0]).arg(mode).output().unwrap();
-            assert!(!silent.status.success());
+
+            check_status(silent.status);
+
             let error = String::from_utf8_lossy(&silent.stderr);
             assert!(error.contains("has overflowed its stack"),
                     "missing overflow message: {}", error);

src/test/run-pass/segfault-no-out-of-stack.rs

@@ -15,18 +15,39 @@ extern crate libc;
 use std::process::{Command, ExitStatus};
 use std::env;
 
+#[link(name = "rust_test_helpers")]
+extern {
+    fn rust_get_null_ptr() -> *mut ::libc::c_char;
+}
+
+#[cfg(unix)]
+fn check_status(status: std::process::ExitStatus)
+{
+    use libc;
+    use std::os::unix::process::ExitStatusExt;
+
+    assert!(status.signal() == Some(libc::SIGSEGV)
+            || status.signal() == Some(libc::SIGBUS));
+}
+
+#[cfg(not(unix))]
+fn check_status(status: std::process::ExitStatus)
+{
+    assert!(!status.success());
+}
+
 fn main() {
     let args: Vec<String> = env::args().collect();
     if args.len() > 1 && args[1] == "segfault" {
-        unsafe { *(0 as *mut isize) = 1 }; // trigger a segfault
+        unsafe { *rust_get_null_ptr() = 1; }; // trigger a segfault
     } else {
         let segfault = Command::new(&args[0]).arg("segfault").output().unwrap();
         let stderr = String::from_utf8_lossy(&segfault.stderr);
         let stdout = String::from_utf8_lossy(&segfault.stdout);
         println!("stdout: {}", stdout);
         println!("stderr: {}", stderr);
         println!("status: {}", segfault.status);
-        assert!(!segfault.status.success());
+        check_status(segfault.status);
         assert!(!stderr.contains("has overflowed its stack"));
     }
 }