From 7e577476d81f36cac9f3dd64df924b3761e53b49 Mon Sep 17 00:00:00 2001 From: Smicry Date: Sun, 12 Sep 2021 23:20:05 +0800 Subject: [PATCH 01/11] add kill -l final new line #2644 --- src/uu/kill/src/kill.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/uu/kill/src/kill.rs b/src/uu/kill/src/kill.rs index 494dc0602..b1cbba17a 100644 --- a/src/uu/kill/src/kill.rs +++ b/src/uu/kill/src/kill.rs @@ -160,18 +160,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<()> { From 7d445612a29a8a2afba61c4024923be13253aca8 Mon Sep 17 00:00:00 2001 From: Smicry Date: Mon, 13 Sep 2021 00:15:53 +0800 Subject: [PATCH 02/11] add kill list final new line test --- tests/by-util/test_kill.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/by-util/test_kill.rs b/tests/by-util/test_kill.rs index fe5d4557a..9afac2862 100644 --- a/tests/by-util/test_kill.rs +++ b/tests/by-util/test_kill.rs @@ -56,6 +56,14 @@ fn test_kill_list_all_signals() { .stdout_contains("HUP"); } +#[test] +fn test_kill_list_final_new_line() { + new_ucmd!() + .arg("-l") + .succeeds() + .stdout_matches(&Regex::new("\\n$").unwrap()); +} + #[test] fn test_kill_list_all_signals_as_table() { // Check for a few signals. Do not try to be comprehensive. From db21fef95de9931a2d0c69cf0ccf3b18966f013d Mon Sep 17 00:00:00 2001 From: Smicry Date: Mon, 13 Sep 2021 09:27:14 +0800 Subject: [PATCH 03/11] fix kill list final new line test --- tests/by-util/test_kill.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/by-util/test_kill.rs b/tests/by-util/test_kill.rs index 9afac2862..a16760689 100644 --- a/tests/by-util/test_kill.rs +++ b/tests/by-util/test_kill.rs @@ -61,7 +61,7 @@ fn test_kill_list_final_new_line() { new_ucmd!() .arg("-l") .succeeds() - .stdout_matches(&Regex::new("\\n$").unwrap()); + .stdout_matches(&Regex::new("[\\n\\r]$").unwrap()); } #[test] From 89428a77f31c26024980be35ed89e4b7de7983d7 Mon Sep 17 00:00:00 2001 From: Smicry Date: Tue, 14 Sep 2021 23:56:08 +0800 Subject: [PATCH 04/11] fix kill list final new line test --- tests/by-util/test_kill.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/by-util/test_kill.rs b/tests/by-util/test_kill.rs index a16760689..91e524733 100644 --- a/tests/by-util/test_kill.rs +++ b/tests/by-util/test_kill.rs @@ -58,10 +58,8 @@ fn test_kill_list_all_signals() { #[test] fn test_kill_list_final_new_line() { - new_ucmd!() - .arg("-l") - .succeeds() - .stdout_matches(&Regex::new("[\\n\\r]$").unwrap()); + let re = Regex::new("\\n$").unwrap(); + assert!(re.is_match(new_ucmd!().arg("-l").succeeds().stdout_str())); } #[test] From 9d5133157ae70da60eb9c30553c9e3cf1ac120d2 Mon Sep 17 00:00:00 2001 From: Jan Verbeek Date: Fri, 27 Aug 2021 14:58:04 +0200 Subject: [PATCH 05/11] uucore::fsext: Replace some unsafe calls GetLastError() and libc::stat() were unnecessary as libstd offered equivalents. LPWSTR2String() was technically unsafe if passed a slice without zeroes, but it's a private function and was probably always called correctly in practice. --- src/uucore/src/lib/features/fsext.rs | 48 +++++++++++++--------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/src/uucore/src/lib/features/fsext.rs b/src/uucore/src/lib/features/fsext.rs index 7525d9e41..771ef7283 100644 --- a/src/uucore/src/lib/features/fsext.rs +++ b/src/uucore/src/lib/features/fsext.rs @@ -30,12 +30,8 @@ use std::ffi::OsString; #[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::{ @@ -62,10 +58,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 +71,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 +150,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 @@ -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; From 8cfe0290cd142946de7337187d94a31ee636631a Mon Sep 17 00:00:00 2001 From: Jan Verbeek Date: Fri, 27 Aug 2021 15:09:42 +0200 Subject: [PATCH 06/11] uucore::fsext: Avoid unnecessary allocations --- src/uucore/src/lib/features/fsext.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/uucore/src/lib/features/fsext.rs b/src/uucore/src/lib/features/fsext.rs index 771ef7283..97c1da79c 100644 --- a/src/uucore/src/lib/features/fsext.rs +++ b/src/uucore/src/lib/features/fsext.rs @@ -26,7 +26,7 @@ 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)] @@ -43,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::>() @@ -238,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) From cc652c7fe9b0fb64e388408f846fb0dc09bd3257 Mon Sep 17 00:00:00 2001 From: Jan Verbeek Date: Tue, 14 Sep 2021 19:54:40 +0200 Subject: [PATCH 07/11] uucore::mode: Add notes about umask and platform support --- src/uucore/src/lib/features.rs | 2 +- src/uucore/src/lib/features/mode.rs | 7 +++++++ src/uucore/src/lib/lib.rs | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) 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/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/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 From 601ea3ef1918feb1bb69687a6b43c091cc9f4e13 Mon Sep 17 00:00:00 2001 From: Jan Verbeek Date: Fri, 27 Aug 2021 16:16:59 +0200 Subject: [PATCH 08/11] uucore::process: Add a few notes --- src/uucore/src/lib/features/process.rs | 5 +++++ 1 file changed, 5 insertions(+) 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. From 6931dd11f1fd6e0526003b5704b4d6442548f776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Th=C3=A9riault?= Date: Sun, 19 Sep 2021 12:32:06 -0700 Subject: [PATCH 09/11] Use non-yanked version of digest crate in hashsum --- Cargo.lock | 4 ++-- src/uu/hashsum/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d767526ab..bc7cd7d0d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -669,9 +669,9 @@ checksum = "0e25ea47919b1560c4e3b7fe0aaab9becf5b84a10325ddf7db0f0ba5e1026499" [[package]] name = "digest" -version = "0.6.2" +version = "0.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e5b29bf156f3f4b3c4f610a25ff69370616ae6e0657d416de22645483e72af0a" +checksum = "ecae1c064e29fcabb6c2e9939e53dc7da72ed90234ae36ebfe03a478742efbd1" dependencies = [ "generic-array", ] diff --git a/src/uu/hashsum/Cargo.toml b/src/uu/hashsum/Cargo.toml index 9ea142c8b..7cb88dede 100644 --- a/src/uu/hashsum/Cargo.toml +++ b/src/uu/hashsum/Cargo.toml @@ -15,7 +15,7 @@ edition = "2018" path = "src/hashsum.rs" [dependencies] -digest = "0.6.2" +digest = "0.6.1" clap = { version = "2.33", features = ["wrap_help"] } hex = "0.2.0" libc = "0.2.42" From 3882df5cdcde6e96f559176f9936f49e81a966b2 Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Sat, 25 Sep 2021 00:22:01 -0300 Subject: [PATCH 10/11] expr: use UResult --- src/uu/expr/src/expr.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) 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()) } From e9371dc57d6f323f9d8953126af9fa23d81054b5 Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Fri, 1 Oct 2021 23:25:56 +0200 Subject: [PATCH 11/11] common/util: fix parsing of coreutil version For the CICD on macOS, this fixes: ``` ---- common::util::tests::test_check_coreutil_version stdout ---- ---- common::util::tests::test_expected_result stdout ---- thread 'common::util::tests::test_expected_result' panicked at 'byte index 4 is out of bounds of `9.0`', tests/common/util.rs:1172:41 ``` --- tests/common/util.rs | 46 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) 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() {