From a8bb7fa76b3d9d288ad12e9a86be96ee0e6abadf Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 13 May 2021 18:59:41 +0100 Subject: [PATCH 1/7] aborts: Clarify documentation and comments In the docs for intrinsics::abort(): * Strengthen the recommendation by to use process::abort instead. * Document the fact that it (ab)uses an LLVM debug trap and what the likely consequences are. * State that the precise behaviour is unstable. In the docs for process::abort(): * Promise that we have the same behaviour as C `abort()`. * Document the likely consequences, including, specifically, the consequences on Unix. In the internal comment for unix::abort_internal: * Refer to the public docs for the public API functions. * Correct and expand the description of libc::abort. Specifically: * Do not claim that abort() unregisters signal handlers. It doesn't; it honours the SIGABRT handler. * Discuss, extensively, the issue with abort() flushing stdio buffers. * Describe the glibc behaviour in some detail. Co-authored-by: Mark Wooding Signed-off-by: Ian Jackson --- library/core/src/intrinsics.rs | 8 +++++-- library/std/src/process.rs | 4 ++++ library/std/src/sys/unix/mod.rs | 42 +++++++++++++++++++++++++++------ 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index b4311bbe5f41f..8be4f5997693c 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -717,8 +717,12 @@ extern "rust-intrinsic" { /// Therefore, implementations must not require the user to uphold /// any safety invariants. /// - /// A more user-friendly and stable version of this operation is - /// [`std::process::abort`](../../std/process/fn.abort.html). + /// [`std::process::abort`](../../std/process/fn.abort.html) is to be preferred if possible, + /// as its behaviour is more user-friendly and more stable. + /// + /// The current implementation of `intrinsics::abort` (ab)uses a debug trap. On Unix, the + /// process will probably die of a signal like `SIGABRT`, `SIGILL`, `SIGTRAP`, `SIGSEGV` or + /// `SIGBUS`. The precise behaviour is not guaranteed and not stable. pub fn abort() -> !; /// Informs the optimizer that this point in the code is not reachable, diff --git a/library/std/src/process.rs b/library/std/src/process.rs index b46d3dfc1e7c6..95108f96e061c 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -1908,6 +1908,10 @@ pub fn exit(code: i32) -> ! { /// this function at a known point where there are no more destructors left /// to run. /// +/// The process's termination will be similar to that from the C `abort()` +/// function. On Unix, the process will die with signal `SIGABRT`, which +/// typically means that the shell prints "Aborted". +/// /// # Examples /// /// ```no_run diff --git a/library/std/src/sys/unix/mod.rs b/library/std/src/sys/unix/mod.rs index f3535b27128b3..2da71b2a448ac 100644 --- a/library/std/src/sys/unix/mod.rs +++ b/library/std/src/sys/unix/mod.rs @@ -217,13 +217,41 @@ pub fn cvt_nz(error: libc::c_int) -> crate::io::Result<()> { if error == 0 { Ok(()) } else { Err(crate::io::Error::from_raw_os_error(error)) } } -// On Unix-like platforms, libc::abort will unregister signal handlers -// including the SIGABRT handler, preventing the abort from being blocked, and -// fclose streams, with the side effect of flushing them so libc buffered -// output will be printed. Additionally the shell will generally print a more -// understandable error message like "Abort trap" rather than "Illegal -// instruction" that intrinsics::abort would cause, as intrinsics::abort is -// implemented as an illegal instruction. +// libc::abort() will run the SIGABRT handler. That's fine because anyone who +// installs a SIGABRT handler already has to expect it to run in Very Bad +// situations (eg, malloc crashing). +// +// Current glibc's abort() function unblocks SIGABRT, raises SIGABRT, clears the +// SIGABRT handler and raises it again, and then starts to get creative. +// +// See the public documentation for `intrinsics::abort()` and `process::abort()` +// for further discussion. +// +// There is confusion about whether libc::abort() flushes stdio streams. +// libc::abort() is required by ISO C 99 (7.14.1.1p5) to be async-signal-safe, +// so flushing streams is at least extremely hard, if not entirely impossible. +// +// However, some versions of POSIX (eg IEEE Std 1003.1-2001) required abort to +// do so. In 1003.1-2004 this was fixed. +// +// glibc's implementation did the flush, unsafely, before glibc commit +// 91e7cf982d01 `abort: Do not flush stdio streams [BZ #15436]' by Florian +// Weimer. According to glibc's NEWS: +// +// The abort function terminates the process immediately, without flushing +// stdio streams. Previous glibc versions used to flush streams, resulting +// in deadlocks and further data corruption. This change also affects +// process aborts as the result of assertion failures. +// +// This is an accurate description of the problem. The only solution for +// program with nontrivial use of C stdio is a fixed libc - one which does not +// try to flush in abort - since even libc-internal errors, and assertion +// failures generated from C, will go via abort(). +// +// On systems with old, buggy, libcs, the impact can be severe for a +// multithreaded C program. It is much less severe for Rust, because Rust +// stdlib doesn't use libc stdio buffering. In a typical Rust program, which +// does not use C stdio, even a buggy libc::abort() is, in fact, safe. pub fn abort_internal() -> ! { unsafe { libc::abort() } } From de19e4d2b6328f09f11abea5e56cb22a0fe6536e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 17 May 2021 15:23:47 +0100 Subject: [PATCH 2/7] abort docs: Do not claim that intrinsics::abort is always a debug trap As per discussion here https://github.com/rust-lang/rust/pull/85377#pullrequestreview-660460501 Signed-off-by: Ian Jackson --- library/core/src/intrinsics.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index 8be4f5997693c..12229009ec831 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -720,7 +720,9 @@ extern "rust-intrinsic" { /// [`std::process::abort`](../../std/process/fn.abort.html) is to be preferred if possible, /// as its behaviour is more user-friendly and more stable. /// - /// The current implementation of `intrinsics::abort` (ab)uses a debug trap. On Unix, the + /// The current implementation of `intrinsics::abort` (ab)uses a debug trap + /// on some popular platforms. + /// On Unix, the /// process will probably die of a signal like `SIGABRT`, `SIGILL`, `SIGTRAP`, `SIGSEGV` or /// `SIGBUS`. The precise behaviour is not guaranteed and not stable. pub fn abort() -> !; From 4e7c348140b0ddc074c874f27399a3149a67e84d Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 17 May 2021 15:29:47 +0100 Subject: [PATCH 3/7] abort docs: Document buffer non-flushing There is discussion of this in #40230 which requests clarification. Signed-off-by: Ian Jackson --- library/std/src/process.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index 95108f96e061c..00027591a13ba 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -1898,6 +1898,9 @@ pub fn exit(code: i32) -> ! { /// process, no destructors on the current stack or any other thread's stack /// will be run. /// +/// Rust IO buffers (eg, from `BufWriter`) will not be flushed. +/// Likewise, C stdio buffers will (on most platforms) not be flushed. +/// /// This is in contrast to the default behaviour of [`panic!`] which unwinds /// the current thread's stack and calls all destructors. /// When `panic="abort"` is set, either as an argument to `rustc` or in a From 44852e06039c037b3b44bea306ef7a7ce4d83929 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 7 Jun 2021 12:10:46 +0100 Subject: [PATCH 4/7] Talk about invalid instructions rather than debug traps And withdraw the allegation of "abuse". Adapted from a suggestion by @m-ou-se. Co-authored-by: Mara Bos Signed-off-by: Ian Jackson --- library/core/src/intrinsics.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index 12229009ec831..57508c1869275 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -720,8 +720,8 @@ extern "rust-intrinsic" { /// [`std::process::abort`](../../std/process/fn.abort.html) is to be preferred if possible, /// as its behaviour is more user-friendly and more stable. /// - /// The current implementation of `intrinsics::abort` (ab)uses a debug trap - /// on some popular platforms. + /// The current implementation of `intrinsics::abort` is to invoke an invalid instruction, + /// on most platforms. /// On Unix, the /// process will probably die of a signal like `SIGABRT`, `SIGILL`, `SIGTRAP`, `SIGSEGV` or /// `SIGBUS`. The precise behaviour is not guaranteed and not stable. From 19c347ede96a439cc7045f23b7e087a17ac6bd0a Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 7 Jun 2021 12:12:53 +0100 Subject: [PATCH 5/7] Talk about "terminate" rather than "die" Adapted from a suggestion by @m-ou-se. Co-authored-by: Mara Bos Signed-off-by: Ian Jackson --- library/core/src/intrinsics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index 57508c1869275..7391992ca090f 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -723,7 +723,7 @@ extern "rust-intrinsic" { /// The current implementation of `intrinsics::abort` is to invoke an invalid instruction, /// on most platforms. /// On Unix, the - /// process will probably die of a signal like `SIGABRT`, `SIGILL`, `SIGTRAP`, `SIGSEGV` or + /// process will probably terminate with a signal like `SIGABRT`, `SIGILL`, `SIGTRAP`, `SIGSEGV` or /// `SIGBUS`. The precise behaviour is not guaranteed and not stable. pub fn abort() -> !; From f73a555fc9b5def7f444b58eff69965910a662ff Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 5 Jul 2021 12:40:23 +0200 Subject: [PATCH 6/7] Use american spelling for behaviour Co-authored-by: Yuki Okushi --- library/core/src/intrinsics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/core/src/intrinsics.rs b/library/core/src/intrinsics.rs index 7391992ca090f..c5a4bbd320804 100644 --- a/library/core/src/intrinsics.rs +++ b/library/core/src/intrinsics.rs @@ -718,7 +718,7 @@ extern "rust-intrinsic" { /// any safety invariants. /// /// [`std::process::abort`](../../std/process/fn.abort.html) is to be preferred if possible, - /// as its behaviour is more user-friendly and more stable. + /// as its behavior is more user-friendly and more stable. /// /// The current implementation of `intrinsics::abort` is to invoke an invalid instruction, /// on most platforms. From 08d912fdcc4b9c8af3c00db78804f85abfadbd7a Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Mon, 5 Jul 2021 12:43:45 +0200 Subject: [PATCH 7/7] s/die/terminate/ in abort documentation. --- library/std/src/process.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index 00027591a13ba..11a0432ce27a1 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -1912,7 +1912,7 @@ pub fn exit(code: i32) -> ! { /// to run. /// /// The process's termination will be similar to that from the C `abort()` -/// function. On Unix, the process will die with signal `SIGABRT`, which +/// function. On Unix, the process will terminate with signal `SIGABRT`, which /// typically means that the shell prints "Aborted". /// /// # Examples