From fbd575aedf1a60ca5528d5be945639e02d44b3e7 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 22 Feb 2021 14:30:03 +0000 Subject: [PATCH 1/7] process::unix: Handle other wait statuses in ExitStatus as Display Currently, on Nightly, this panics: ``` use std::process::ExitStatus; use std::os::unix::process::ExitStatusExt; fn main() { let st = ExitStatus::from_raw(0x007f); println!("st = {}", st); } ``` This is because the impl of Display assumes that if .code() is None, .signal() must be Some. That was a false assumption, although it was true with buggy code before 5b1316f78152a9c066b357ea9addf803d48e114a unix ExitStatus: Do not treat WIFSTOPPED as WIFSIGNALED This is not likely to have affected many people in practice, because `Command` will never produce such a wait status (`ExitStatus`). Signed-off-by: Ian Jackson --- library/std/src/sys/unix/process/process_unix.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index 9e82df7755e89..26cbb0a508318 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -527,9 +527,18 @@ impl fmt::Display for ExitStatus { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { if let Some(code) = self.code() { write!(f, "exit code: {}", code) + } else if let Some(signal) = self.signal() { + if self.core_dumped() { + write!(f, "signal: {} (core dumped)", signal) + } else { + write!(f, "signal: {}", signal) + } + } else if let Some(signal) = self.stopped_signal() { + write!(f, "stopped (not terminated) by signal: {}", signal) + } else if self.continued() { + write!(f, "continued (WIFCONTINUED)") } else { - let signal = self.signal().unwrap(); - write!(f, "signal: {}", signal) + write!(f, "unrecognised wait status: {} {:#x}", self.0, self.0) } } } From d8cfd56985bd8cc32274bfead4b1499da1c38810 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 22 Feb 2021 14:58:52 +0000 Subject: [PATCH 2/7] process::unix: Test wait status formatting Signed-off-by: Ian Jackson --- .../std/src/sys/unix/process/process_unix.rs | 4 ++++ .../sys/unix/process/process_unix/tests.rs | 22 +++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 library/std/src/sys/unix/process/process_unix/tests.rs diff --git a/library/std/src/sys/unix/process/process_unix.rs b/library/std/src/sys/unix/process/process_unix.rs index 26cbb0a508318..d2ef0eb128c5d 100644 --- a/library/std/src/sys/unix/process/process_unix.rs +++ b/library/std/src/sys/unix/process/process_unix.rs @@ -542,3 +542,7 @@ impl fmt::Display for ExitStatus { } } } + +#[cfg(test)] +#[path = "process_unix/tests.rs"] +mod tests; diff --git a/library/std/src/sys/unix/process/process_unix/tests.rs b/library/std/src/sys/unix/process/process_unix/tests.rs new file mode 100644 index 0000000000000..60cb161aca2ab --- /dev/null +++ b/library/std/src/sys/unix/process/process_unix/tests.rs @@ -0,0 +1,22 @@ +#[test] +fn exitstatus_display_tests() { + // In practice this is the same on every Unix. + // If some weird platform turns out to be different, and this test fails, use #[cfg]. + use crate::os::unix::process::ExitStatusExt; + use crate::process::ExitStatus; + + let t = |v, s| assert_eq!(s, format!("{}", ::from_raw(v))); + + t(0x0000f, "signal: 15"); + t(0x0008b, "signal: 11 (core dumped)"); + t(0x00000, "exit code: 0"); + t(0x0ff00, "exit code: 255"); + t(0x0137f, "stopped (not terminated) by signal: 19"); + t(0x0ffff, "continued (WIFCONTINUED)"); + + // Testing "unrecognised wait status" is hard because the wait.h macros typically + // assume that the value came from wait and isn't mad. With the glibc I have here + // this works: + #[cfg(target_env = "gnu")] + t(0x000ff, "unrecognised wait status: 255 0xff"); +} From 4bb8425af60eb673a932c90ee8d1b5f24c13a34e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 22 Feb 2021 15:26:19 +0000 Subject: [PATCH 3/7] ExitStatus: Improve documentation re wait status vs exit status The use of `ExitStatus` as the Rust type name for a Unix *wait status*, not an *exit status*, is very confusing, but sadly probably too late to change. This area is confusing enough in Unix already (and many programmers are already confuxed). We can at least document it. I chose *not* to mention the way shells like to exit with signal numbers, thus turning signal numbers into exit statuses. This is only relevant for Rust programs using `std::process` if they run shells. Signed-off-by: Ian Jackson --- library/std/src/process.rs | 25 ++++++++++++++++++------- library/std/src/sys/unix/ext/process.rs | 14 ++++++++++++-- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/library/std/src/process.rs b/library/std/src/process.rs index 6480e654c55f0..15ac9e402c589 100644 --- a/library/std/src/process.rs +++ b/library/std/src/process.rs @@ -885,7 +885,7 @@ impl Command { } /// Executes a command as a child process, waiting for it to finish and - /// collecting its exit status. + /// collecting its status. /// /// By default, stdin, stdout and stderr are inherited from the parent. /// @@ -899,7 +899,7 @@ impl Command { /// .status() /// .expect("failed to execute process"); /// - /// println!("process exited with: {}", status); + /// println!("process finished with: {}", status); /// /// assert!(status.success()); /// ``` @@ -1368,11 +1368,17 @@ impl From for Stdio { /// Describes the result of a process after it has terminated. /// -/// This `struct` is used to represent the exit status of a child process. +/// This `struct` is used to represent the exit status or other termination of a child process. /// Child processes are created via the [`Command`] struct and their exit /// status is exposed through the [`status`] method, or the [`wait`] method /// of a [`Child`] process. /// +/// An `ExitStatus` represents every possible disposition of a process. On Unix this +/// is the **wait status**. It is *not* simply an *exit status* (a value passed to `exit`). +/// +/// For proper error reporting of failed processes, print the value of `ExitStatus` using its +/// implementation of [`Display`](crate::fmt::Display). +/// /// [`status`]: Command::status /// [`wait`]: Child::wait #[derive(PartialEq, Eq, Clone, Copy, Debug)] @@ -1400,7 +1406,7 @@ impl ExitStatus { /// if status.success() { /// println!("'projects/' directory created"); /// } else { - /// println!("failed to create 'projects/' directory"); + /// println!("failed to create 'projects/' directory: {}", status); /// } /// ``` #[stable(feature = "process", since = "1.0.0")] @@ -1410,9 +1416,14 @@ impl ExitStatus { /// Returns the exit code of the process, if any. /// - /// On Unix, this will return `None` if the process was terminated - /// by a signal; `std::os::unix` provides an extension trait for - /// extracting the signal and other details from the `ExitStatus`. + /// In Unix terms the return value is the **exit status**: the value passed to `exit`, if the + /// process finished by calling `exit`. Note that on Unix the exit status is truncated to 8 + /// bits, and that values that didn't come from a program's call to `exit` may be invented the + /// runtime system (often, for example, 255, 254, 127 or 126). + /// + /// On Unix, this will return `None` if the process was terminated by a signal. + /// [`ExitStatusExt`](crate::os::unix::process::ExitStatusExt) is an + /// extension trait for extracting any such signal, and other details, from the `ExitStatus`. /// /// # Examples /// diff --git a/library/std/src/sys/unix/ext/process.rs b/library/std/src/sys/unix/ext/process.rs index 7559c1f1d9e29..6dc2033289a4a 100644 --- a/library/std/src/sys/unix/ext/process.rs +++ b/library/std/src/sys/unix/ext/process.rs @@ -186,12 +186,20 @@ impl CommandExt for process::Command { /// Unix-specific extensions to [`process::ExitStatus`]. /// +/// On Unix, `ExitStatus` **does not necessarily represent an exit status**, as passed to the +/// `exit` system call or returned by [`ExitStatus::code()`](crate::process::ExitStatus::code). +/// It represents **any wait status**, as returned by one of the `wait` family of system calls. +/// +/// This is because a Unix wait status (a Rust `ExitStatus`) can represent a Unix exit status, but +/// can also represent other kinds of process event. +/// /// This trait is sealed: it cannot be implemented outside the standard library. /// This is so that future additional methods are not breaking changes. #[stable(feature = "rust1", since = "1.0.0")] pub trait ExitStatusExt: Sealed { - /// Creates a new `ExitStatus` from the raw underlying `i32` return value of - /// a process. + /// Creates a new `ExitStatus` from the raw underlying integer status value from `wait` + /// + /// The value should be a **wait status, not an exit status**. #[stable(feature = "exit_status_from", since = "1.12.0")] fn from_raw(raw: i32) -> Self; @@ -220,6 +228,8 @@ pub trait ExitStatusExt: Sealed { fn continued(&self) -> bool; /// Returns the underlying raw `wait` status. + /// + /// The returned integer is a **wait status, not an exit status**. #[unstable(feature = "unix_process_wait_more", issue = "80695")] fn into_raw(self) -> i32; } From 67cfc22ee228cee1a795ca1f7430165984fe1b04 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 4 Mar 2021 12:18:04 +0000 Subject: [PATCH 4/7] ExitStatus stop signal display test: Make it Linux only MacOS uses a different representation. Signed-off-by: Ian Jackson --- library/std/src/sys/unix/process/process_unix/tests.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/library/std/src/sys/unix/process/process_unix/tests.rs b/library/std/src/sys/unix/process/process_unix/tests.rs index 60cb161aca2ab..3ab568eaa33fe 100644 --- a/library/std/src/sys/unix/process/process_unix/tests.rs +++ b/library/std/src/sys/unix/process/process_unix/tests.rs @@ -11,8 +11,13 @@ fn exitstatus_display_tests() { t(0x0008b, "signal: 11 (core dumped)"); t(0x00000, "exit code: 0"); t(0x0ff00, "exit code: 255"); - t(0x0137f, "stopped (not terminated) by signal: 19"); - t(0x0ffff, "continued (WIFCONTINUED)"); + + // On MacOS, 0x0137f is WIFCONTINUED, not WIFSTOPPED. Probably *BSD is similar. + // https://github.com/rust-lang/rust/pull/82749#issuecomment-790525956 + // The purpose of this test is to test our string formatting, not our understanding of the wait + // status magic numbers. So restrict these to Linux. + #[cfg(target_os = "linux")] t(0x0137f, "stopped (not terminated) by signal: 19"); + #[cfg(target_os = "linux")] t(0x0ffff, "continued (WIFCONTINUED)"); // Testing "unrecognised wait status" is hard because the wait.h macros typically // assume that the value came from wait and isn't mad. With the glibc I have here From a240ff5a7713f744755855c507eab327f90cd824 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 4 Mar 2021 12:26:27 +0000 Subject: [PATCH 5/7] ExitStatus unknown wait status test: Make it Linux only If different unices have different bit patterns for WIFSTOPPED and WIFCONTINUED then simply being glibc is probably not good enough for this rather ad-hoc test to work. Do it on Linux only. Signed-off-by: Ian Jackson --- library/std/src/sys/unix/process/process_unix/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sys/unix/process/process_unix/tests.rs b/library/std/src/sys/unix/process/process_unix/tests.rs index 3ab568eaa33fe..5d1bf47083513 100644 --- a/library/std/src/sys/unix/process/process_unix/tests.rs +++ b/library/std/src/sys/unix/process/process_unix/tests.rs @@ -22,6 +22,6 @@ fn exitstatus_display_tests() { // Testing "unrecognised wait status" is hard because the wait.h macros typically // assume that the value came from wait and isn't mad. With the glibc I have here // this works: - #[cfg(target_env = "gnu")] + #[cfg(all(target_os = "linux", target_env = "gnu"))] t(0x000ff, "unrecognised wait status: 255 0xff"); } From 8e4433ab3e6a0fe8cc6f83379b30a48f94da4f33 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 4 Mar 2021 12:44:19 +0000 Subject: [PATCH 6/7] ExitStatus tests: Make less legible to satisfy "tidy" I strongly disagree with tidy in this case but AIUI there is no way to override it. Signed-off-by: Ian Jackson --- library/std/src/sys/unix/process/process_unix/tests.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/std/src/sys/unix/process/process_unix/tests.rs b/library/std/src/sys/unix/process/process_unix/tests.rs index 5d1bf47083513..915402970f58c 100644 --- a/library/std/src/sys/unix/process/process_unix/tests.rs +++ b/library/std/src/sys/unix/process/process_unix/tests.rs @@ -16,8 +16,10 @@ fn exitstatus_display_tests() { // https://github.com/rust-lang/rust/pull/82749#issuecomment-790525956 // The purpose of this test is to test our string formatting, not our understanding of the wait // status magic numbers. So restrict these to Linux. - #[cfg(target_os = "linux")] t(0x0137f, "stopped (not terminated) by signal: 19"); - #[cfg(target_os = "linux")] t(0x0ffff, "continued (WIFCONTINUED)"); + #[cfg(target_os = "linux")] + t(0x0137f, "stopped (not terminated) by signal: 19"); + #[cfg(target_os = "linux")] + t(0x0ffff, "continued (WIFCONTINUED)"); // Testing "unrecognised wait status" is hard because the wait.h macros typically // assume that the value came from wait and isn't mad. With the glibc I have here From 11ca64401a5d562898e8b5f46bd36d6d1c6dc3ef Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Tue, 9 Mar 2021 10:53:03 +0000 Subject: [PATCH 7/7] Always compile the fragile wait status test cases, just run them conditionally Co-authored-by: David Tolnay --- .../std/src/sys/unix/process/process_unix/tests.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/library/std/src/sys/unix/process/process_unix/tests.rs b/library/std/src/sys/unix/process/process_unix/tests.rs index 915402970f58c..5819d2c2a5a26 100644 --- a/library/std/src/sys/unix/process/process_unix/tests.rs +++ b/library/std/src/sys/unix/process/process_unix/tests.rs @@ -16,14 +16,15 @@ fn exitstatus_display_tests() { // https://github.com/rust-lang/rust/pull/82749#issuecomment-790525956 // The purpose of this test is to test our string formatting, not our understanding of the wait // status magic numbers. So restrict these to Linux. - #[cfg(target_os = "linux")] - t(0x0137f, "stopped (not terminated) by signal: 19"); - #[cfg(target_os = "linux")] - t(0x0ffff, "continued (WIFCONTINUED)"); + if cfg!(target_os = "linux") { + t(0x0137f, "stopped (not terminated) by signal: 19"); + t(0x0ffff, "continued (WIFCONTINUED)"); + } // Testing "unrecognised wait status" is hard because the wait.h macros typically // assume that the value came from wait and isn't mad. With the glibc I have here // this works: - #[cfg(all(target_os = "linux", target_env = "gnu"))] - t(0x000ff, "unrecognised wait status: 255 0xff"); + if cfg!(all(target_os = "linux", target_env = "gnu")) { + t(0x000ff, "unrecognised wait status: 255 0xff"); + } }