-
Notifications
You must be signed in to change notification settings - Fork 471
materialized: attempt to produce nice backtraces on stack overflow #5549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,13 @@ | |
|
||
//! System support functions. | ||
|
||
use std::alloc::{self, Layout}; | ||
use std::ptr; | ||
|
||
use anyhow::{bail, Context}; | ||
use log::{trace, warn}; | ||
use nix::errno; | ||
use nix::sys::signal; | ||
|
||
#[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "ios")))] | ||
pub fn adjust_rlimits() { | ||
|
@@ -90,3 +96,79 @@ pub fn adjust_rlimits() { | |
) | ||
} | ||
} | ||
|
||
/// Attempts to enable backtraces when SIGSEGV occurs. | ||
/// | ||
/// In particular, this means producing backtraces on stack overflow, as | ||
/// stack overflow raises SIGSEGV. The approach here involves making system | ||
/// calls to handle SIGSEGV on an alternate signal stack, which seems to work | ||
/// well in practice but may technically be undefined behavior. | ||
/// | ||
/// Rust may someday do this by default. | ||
/// Follow: https://github.com/rust-lang/rust/issues/51405. | ||
pub fn enable_sigsegv_backtraces() -> Result<(), anyhow::Error> { | ||
// This code is derived from the code in the backtrace-on-stack-overflow | ||
// crate, which is freely available under the terms of the Apache 2.0 | ||
// license. The modifications here provide better error messages if any of | ||
// the various system calls fail. | ||
// | ||
// See: https://github.com/matklad/backtrace-on-stack-overflow | ||
|
||
// NOTE(benesch): The stack size was chosen to match the default Rust thread | ||
// stack size of 2MiB. Probably overkill, but we'd much rather have | ||
// backtraces on stack overflow than squabble over a few megabytes. Using | ||
// libc::SIGSTKSZ is tempting, but its default of 8KiB on my system makes me | ||
// nervous. Rust code isn't used to running with a stack that small. | ||
const STACK_SIZE: usize = 2 << 20; | ||
|
||
// x86_64 and aarch64 require 16-byte alignment. Its hard to imagine other | ||
// platforms that would have more stringent requirements. | ||
const STACK_ALIGN: usize = 16; | ||
|
||
// Allocate a stack. | ||
let buf_layout = | ||
Layout::from_size_align(STACK_SIZE, STACK_ALIGN).expect("layout known to be valid"); | ||
// SAFETY: layout has non-zero size and the uninitialized memory that is | ||
// returned is never read (at least, not by Rust). | ||
let buf = unsafe { alloc::alloc(buf_layout) }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New coding standard I just invented: you must leave a comment about why any usage of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I do try to follow that in general—I'm just not sure what it would say in this function. Like, there aren't invariants that must be upheld. We're just calling unsafe C functions. I can add comments like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Which points to...
So I think you can just mention that (1) you have ensured that the size is non-zero, and (2) that nobody is reading the uninitialized value (or at any rate, not doing so before it gets passed to C World and outside the purview of Rust's safety guarantees anyway). Given your response, I think you might have instead been thinking of your later uses of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, took another stab at it. |
||
|
||
// Request that signals be delivered to this alternate stack. | ||
let stack = libc::stack_t { | ||
ss_sp: buf as *mut libc::c_void, | ||
ss_flags: 0, | ||
ss_size: STACK_SIZE, | ||
}; | ||
// SAFETY: `stack` is a valid pointer to a `stack_t` object and the second | ||
// parameter, `old_ss`, is permitted to be `NULL` according to POSIX. | ||
let ret = unsafe { libc::sigaltstack(&stack, ptr::null_mut()) }; | ||
if ret == -1 { | ||
let errno = errno::from_i32(errno::errno()); | ||
bail!("failed to configure alternate signal stack: {}", errno); | ||
} | ||
|
||
// Install a handler for SIGSEGV. | ||
let action = signal::SigAction::new( | ||
signal::SigHandler::Handler(handle_sigsegv), | ||
signal::SaFlags::SA_NODEFER | signal::SaFlags::SA_ONSTACK, | ||
signal::SigSet::empty(), | ||
); | ||
// SAFETY: see `handle_sigsegv`. | ||
unsafe { signal::sigaction(signal::SIGSEGV, &action) } | ||
.context("failed to install SIGSEGV handler")?; | ||
|
||
Ok(()) | ||
} | ||
|
||
extern "C" fn handle_sigsegv(_: i32) { | ||
// SAFETY: this is is a signal handler function and technically must be | ||
// "async-signal safe" [0]. That typically means no memory allocation, which | ||
// means no panics or backtraces... but if we're here, we're already doomed | ||
// by a segfault. So there is little harm to ignoring the rules and | ||
// panicking. If we're successful, as we often are, the panic will be caught | ||
// by our panic handler and displayed nicely with a backtrace that traces | ||
// *through* the signal handler and includes the frames that led to the | ||
// SIGSEGV. | ||
// | ||
// [0]: https://man7.org/linux/man-pages/man7/signal-safety.7.html | ||
panic!("stack overflow"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to use
libc::SIGSTKSZ
insteadThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, that's a good point you bring up. The thing is that
libc::SIGSTKSZ
makes me a little nervous because I'm not sure thatBacktrace::backtrace
is equipped to run on an 8KiB stack. I mean, it should be, because a few stack frames should not take up 8KIB... but I figured it'd be better to be safe than sorry. Don't want some crazy change to how gimli works down the road causing a stack overflow while we're trying to handle a stack overflow.Happy to be convinced otherwise, but for now I just added a comment that briefly explains this.