diff --git a/src/uu/expr/src/expr.rs b/src/uu/expr/src/expr.rs index 2d82300ff..eaf329bc5 100644 --- a/src/uu/expr/src/expr.rs +++ b/src/uu/expr/src/expr.rs @@ -9,6 +9,7 @@ extern crate uucore; use clap::{crate_version, App, Arg}; +use uucore::error::{UResult, USimpleError}; use uucore::InvalidEncodingHandling; mod syntax_tree; @@ -23,7 +24,8 @@ pub fn uu_app() -> App<'static, 'static> { .arg(Arg::with_name(HELP).long(HELP)) } -pub fn uumain(args: impl uucore::Args) -> i32 { +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { let args = args .collect_str(InvalidEncodingHandling::ConvertLossy) .accept_any(); @@ -32,13 +34,13 @@ pub fn uumain(args: impl uucore::Args) -> i32 { // The following usage should work without escaping hyphens: `expr -15 = 1 + 2 \* \( 3 - -4 \)` if maybe_handle_help_or_version(&args) { - 0 + Ok(()) } else { let token_strings = args[1..].to_vec(); match process_expr(&token_strings) { Ok(expr_result) => print_expr_ok(&expr_result), - Err(expr_error) => print_expr_error(&expr_error), + Err(expr_error) => Err(USimpleError::new(2, &expr_error)), } } } @@ -49,19 +51,15 @@ fn process_expr(token_strings: &[String]) -> Result { evaluate_ast(maybe_ast) } -fn print_expr_ok(expr_result: &str) -> i32 { +fn print_expr_ok(expr_result: &str) -> UResult<()> { println!("{}", expr_result); if expr_result == "0" || expr_result.is_empty() { - 1 + Err(1.into()) } else { - 0 + Ok(()) } } -fn print_expr_error(expr_error: &str) -> ! { - crash!(2, "{}", expr_error) -} - fn evaluate_ast(maybe_ast: Result, String>) -> Result { maybe_ast.and_then(|ast| ast.evaluate()) } diff --git a/src/uu/kill/src/kill.rs b/src/uu/kill/src/kill.rs index f269f7283..e9b7ee349 100644 --- a/src/uu/kill/src/kill.rs +++ b/src/uu/kill/src/kill.rs @@ -158,18 +158,13 @@ fn print_signal(signal_name_or_value: &str) -> UResult<()> { } fn print_signals() { - let mut pos = 0; for (idx, signal) in ALL_SIGNALS.iter().enumerate() { - pos += signal.len(); - print!("{}", signal); - if idx > 0 && pos > 73 { - println!(); - pos = 0; - } else { - pos += 1; + if idx > 0 { print!(" "); } + print!("{}", signal); } + println!(); } fn list(arg: Option) -> UResult<()> { diff --git a/src/uucore/src/lib/features.rs b/src/uucore/src/lib/features.rs index 42d8ffbe7..546cbea87 100644 --- a/src/uucore/src/lib/features.rs +++ b/src/uucore/src/lib/features.rs @@ -10,7 +10,7 @@ pub mod fsext; pub mod ringbuffer; // * (platform-specific) feature-gated modules -// ** non-windows +// ** non-windows (i.e. Unix + Fuchsia) #[cfg(all(not(windows), feature = "mode"))] pub mod mode; diff --git a/src/uucore/src/lib/features/fsext.rs b/src/uucore/src/lib/features/fsext.rs index 7525d9e41..97c1da79c 100644 --- a/src/uucore/src/lib/features/fsext.rs +++ b/src/uucore/src/lib/features/fsext.rs @@ -26,16 +26,12 @@ const MAX_PATH: usize = 266; static EXIT_ERR: i32 = 1; #[cfg(windows)] -use std::ffi::OsString; +use std::ffi::OsStr; #[cfg(windows)] use std::os::windows::ffi::OsStrExt; #[cfg(windows)] -use std::os::windows::ffi::OsStringExt; -#[cfg(windows)] use winapi::shared::minwindef::DWORD; #[cfg(windows)] -use winapi::um::errhandlingapi::GetLastError; -#[cfg(windows)] use winapi::um::fileapi::GetDiskFreeSpaceW; #[cfg(windows)] use winapi::um::fileapi::{ @@ -47,11 +43,12 @@ use winapi::um::handleapi::INVALID_HANDLE_VALUE; #[cfg(windows)] use winapi::um::winbase::DRIVE_REMOTE; +// Warning: the pointer has to be used *immediately* or the Vec +// it points to will be dropped! #[cfg(windows)] macro_rules! String2LPWSTR { ($str: expr) => { - OsString::from($str.clone()) - .as_os_str() + OsStr::new(&$str) .encode_wide() .chain(Some(0)) .collect::>() @@ -62,10 +59,8 @@ macro_rules! String2LPWSTR { #[cfg(windows)] #[allow(non_snake_case)] fn LPWSTR2String(buf: &[u16]) -> String { - let len = unsafe { libc::wcslen(buf.as_ptr()) }; - OsString::from_wide(&buf[..len as usize]) - .into_string() - .unwrap() + let len = buf.iter().position(|&n| n == 0).unwrap(); + String::from_utf16(&buf[..len]).unwrap() } use self::time::Timespec; @@ -77,7 +72,6 @@ use std::borrow::Cow; use std::convert::{AsRef, From}; #[cfg(unix)] use std::ffi::CString; -#[cfg(unix)] use std::io::Error as IOError; #[cfg(unix)] use std::mem; @@ -157,16 +151,14 @@ impl MountInfo { fn set_missing_fields(&mut self) { #[cfg(unix)] { + use std::os::unix::fs::MetadataExt; // We want to keep the dev_id on Windows // but set dev_id - let path = CString::new(self.mount_dir.clone()).unwrap(); - unsafe { - let mut stat = mem::zeroed(); - if libc::stat(path.as_ptr(), &mut stat) == 0 { - self.dev_id = (stat.st_dev as i32).to_string(); - } else { - self.dev_id = "".to_string(); - } + if let Ok(stat) = std::fs::metadata(&self.mount_dir) { + // Why do we cast this to i32? + self.dev_id = (stat.dev() as i32).to_string() + } else { + self.dev_id = "".to_string(); } } // set MountInfo::dummy @@ -247,8 +239,7 @@ impl MountInfo { volume_name.pop(); unsafe { QueryDosDeviceW( - OsString::from(volume_name.clone()) - .as_os_str() + OsStr::new(&volume_name) .encode_wide() .chain(Some(0)) .skip(4) @@ -445,9 +436,11 @@ pub fn read_fs_list() -> Vec { FindFirstVolumeW(volume_name_buf.as_mut_ptr(), volume_name_buf.len() as DWORD) }; if INVALID_HANDLE_VALUE == find_handle { - crash!(EXIT_ERR, "FindFirstVolumeW failed: {}", unsafe { - GetLastError() - }); + crash!( + EXIT_ERR, + "FindFirstVolumeW failed: {}", + IOError::last_os_error() + ); } let mut mounts = Vec::::new(); loop { @@ -466,8 +459,9 @@ pub fn read_fs_list() -> Vec { volume_name_buf.len() as DWORD, ) } { - let err = unsafe { GetLastError() }; - if err != winapi::shared::winerror::ERROR_NO_MORE_FILES { + let err = IOError::last_os_error(); + if err.raw_os_error() != Some(winapi::shared::winerror::ERROR_NO_MORE_FILES as i32) + { crash!(EXIT_ERR, "FindNextVolumeW failed: {}", err); } break; @@ -527,7 +521,7 @@ impl FsUsage { crash!( EXIT_ERR, "GetVolumePathNamesForVolumeNameW failed: {}", - unsafe { GetLastError() } + IOError::last_os_error() ); } @@ -547,9 +541,11 @@ impl FsUsage { }; if 0 == success { // Fails in case of CD for example - //crash!(EXIT_ERR, "GetDiskFreeSpaceW failed: {}", unsafe { - //GetLastError() - //}); + // crash!( + // EXIT_ERR, + // "GetDiskFreeSpaceW failed: {}", + // IOError::last_os_error() + // ); } let bytes_per_cluster = sectors_per_cluster as u64 * bytes_per_sector as u64; diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index 02e78ba9b..72083e799 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -143,6 +143,13 @@ pub fn parse_mode(mode: &str) -> Result { } pub fn get_umask() -> u32 { + // There's no portable way to read the umask without changing it. + // We have to replace it and then quickly set it back, hopefully before + // some other thread is affected. + // On modern Linux kernels the current umask could instead be read + // from /proc/self/status. But that's a lot of work. + // SAFETY: umask always succeeds and doesn't operate on memory. Races are + // possible but it can't violate Rust's guarantees. let mask = unsafe { umask(0) }; unsafe { umask(mask) }; mask as u32 diff --git a/src/uucore/src/lib/features/process.rs b/src/uucore/src/lib/features/process.rs index fc4b7ed48..d0f530a5a 100644 --- a/src/uucore/src/lib/features/process.rs +++ b/src/uucore/src/lib/features/process.rs @@ -19,6 +19,8 @@ use std::process::ExitStatus as StdExitStatus; use std::thread; use std::time::{Duration, Instant}; +// SAFETY: These functions always succeed and return simple integers. + /// `geteuid()` returns the effective user ID of the calling process. pub fn geteuid() -> uid_t { unsafe { libc::geteuid() } @@ -96,6 +98,9 @@ impl fmt::Display for ExitStatus { /// Missing methods for Child objects pub trait ChildExt { /// Send a signal to a Child process. + /// + /// Caller beware: if the process already exited then you may accidentally + /// send the signal to an unrelated process that recycled the PID. fn send_signal(&mut self, signal: usize) -> io::Result<()>; /// Wait for a process to finish or return after the specified duration. diff --git a/src/uucore/src/lib/lib.rs b/src/uucore/src/lib/lib.rs index 79cc2afc3..3d2d867bd 100644 --- a/src/uucore/src/lib/lib.rs +++ b/src/uucore/src/lib/lib.rs @@ -41,7 +41,7 @@ pub use crate::features::fsext; pub use crate::features::ringbuffer; // * (platform-specific) feature-gated modules -// ** non-windows +// ** non-windows (i.e. Unix + Fuchsia) #[cfg(all(not(windows), feature = "mode"))] pub use crate::features::mode; // ** unix-only diff --git a/tests/by-util/test_kill.rs b/tests/by-util/test_kill.rs index f5166c428..40b9cec67 100644 --- a/tests/by-util/test_kill.rs +++ b/tests/by-util/test_kill.rs @@ -56,6 +56,12 @@ fn test_kill_list_all_signals() { .stdout_contains("HUP"); } +#[test] +fn test_kill_list_final_new_line() { + let re = Regex::new("\\n$").unwrap(); + assert!(re.is_match(new_ucmd!().arg("-l").succeeds().stdout_str())); +} + #[test] fn test_kill_list_all_signals_as_table() { // Check for a few signals. Do not try to be comprehensive. diff --git a/tests/common/util.rs b/tests/common/util.rs index f3cdec010..8e9078e9c 100644 --- a/tests/common/util.rs +++ b/tests/common/util.rs @@ -1169,7 +1169,7 @@ pub fn check_coreutil_version( if s.contains(&format!("(GNU coreutils) {}", version_expected)) { Ok(format!("{}: {}", UUTILS_INFO, s.to_string())) } else if s.contains("(GNU coreutils)") { - let version_found = s.split_whitespace().last().unwrap()[..4].parse::().unwrap_or_default(); + let version_found = parse_coreutil_version(s); let version_expected = version_expected.parse::().unwrap_or_default(); if version_found > version_expected { Ok(format!("{}: version for the reference coreutil '{}' is higher than expected; expected: {}, found: {}", UUTILS_INFO, util_name, version_expected, version_found)) @@ -1182,6 +1182,20 @@ pub fn check_coreutil_version( ) } +// simple heuristic to parse the coreutils SemVer string, e.g. "id (GNU coreutils) 8.32.263-0475" +fn parse_coreutil_version(version_string: &str) -> f32 { + version_string + .split_whitespace() + .last() + .unwrap() + .split('.') + .take(2) + .collect::>() + .join(".") + .parse::() + .unwrap_or_default() +} + /// This runs the GNU coreutils `util_name` binary in `$PATH` in order to /// dynamically gather reference values on the system. /// If the `util_name` in `$PATH` doesn't include a coreutils version string, @@ -1474,6 +1488,36 @@ mod tests { res.normalized_newlines_stdout_is("A\r\nB\nC\n"); } + #[test] + #[cfg(unix)] + fn test_parse_coreutil_version() { + use std::assert_eq; + assert_eq!( + parse_coreutil_version("id (GNU coreutils) 9.0.123-0123").to_string(), + "9" + ); + assert_eq!( + parse_coreutil_version("id (GNU coreutils) 8.32.263-0475").to_string(), + "8.32" + ); + assert_eq!( + parse_coreutil_version("id (GNU coreutils) 8.25.123-0123").to_string(), + "8.25" + ); + assert_eq!( + parse_coreutil_version("id (GNU coreutils) 9.0").to_string(), + "9" + ); + assert_eq!( + parse_coreutil_version("id (GNU coreutils) 8.32").to_string(), + "8.32" + ); + assert_eq!( + parse_coreutil_version("id (GNU coreutils) 8.25").to_string(), + "8.25" + ); + } + #[test] #[cfg(unix)] fn test_check_coreutil_version() {