From 872c0fac1d08850e2fffbe3abcbb65df4f9f5711 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20EBL=C3=89?= Date: Sat, 11 Sep 2021 22:37:55 +0200 Subject: [PATCH 01/26] tests/kill: test old form args with signal name Add two tests of the old form signal arg using the signal name instead of the number. Bonus: add a test for the new form but with the prefix SIG- --- tests/by-util/test_kill.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/by-util/test_kill.rs b/tests/by-util/test_kill.rs index fe5d4557a..f5166c428 100644 --- a/tests/by-util/test_kill.rs +++ b/tests/by-util/test_kill.rs @@ -104,6 +104,26 @@ fn test_kill_with_signal_number_old_form() { assert_eq!(target.wait_for_signal(), Some(9)); } +#[test] +fn test_kill_with_signal_name_old_form() { + let mut target = Target::new(); + new_ucmd!() + .arg("-KILL") + .arg(format!("{}", target.pid())) + .succeeds(); + assert_eq!(target.wait_for_signal(), Some(libc::SIGKILL)); +} + +#[test] +fn test_kill_with_signal_prefixed_name_old_form() { + let mut target = Target::new(); + new_ucmd!() + .arg("-SIGKILL") + .arg(format!("{}", target.pid())) + .succeeds(); + assert_eq!(target.wait_for_signal(), Some(libc::SIGKILL)); +} + #[test] fn test_kill_with_signal_number_new_form() { let mut target = Target::new(); @@ -125,3 +145,14 @@ fn test_kill_with_signal_name_new_form() { .succeeds(); assert_eq!(target.wait_for_signal(), Some(libc::SIGKILL)); } + +#[test] +fn test_kill_with_signal_prefixed_name_new_form() { + let mut target = Target::new(); + new_ucmd!() + .arg("-s") + .arg("SIGKILL") + .arg(format!("{}", target.pid())) + .succeeds(); + assert_eq!(target.wait_for_signal(), Some(libc::SIGKILL)); +} From df64c191078f613726ba347e30320d4fac2ff284 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20EBL=C3=89?= Date: Sun, 12 Sep 2021 00:44:11 +0200 Subject: [PATCH 02/26] kill: kill old form with signal name This commit aim to correct #2645. The origin of the problem was that `handle_obsolete` was not looking for named signals but only for numbers preceded by '-'. I have made a few changes: - Change `handle_obsolete` to use `signal_by_name_or_value` to parse the possible signal. - Since now the signal is already parsed return an `Option` instead of `Option<&str>` to parse later. - Change the way to decide the pid to use from a `match` to a `if` because the tested element are actually not the same for each branch. - Parse the signal from the `-s` argument outside of `kill` function for consistency with the "obsolete" signal case. - Again for consistency, parse the pids outside the `kill` function. --- src/uu/kill/src/kill.rs | 99 ++++++++++++++++++++--------------------- 1 file changed, 49 insertions(+), 50 deletions(-) diff --git a/src/uu/kill/src/kill.rs b/src/uu/kill/src/kill.rs index 494dc0602..f269f7283 100644 --- a/src/uu/kill/src/kill.rs +++ b/src/uu/kill/src/kill.rs @@ -15,7 +15,7 @@ use libc::{c_int, pid_t}; use std::io::Error; use uucore::display::Quotable; use uucore::error::{UResult, USimpleError}; -use uucore::signals::ALL_SIGNALS; +use uucore::signals::{signal_by_name_or_value, ALL_SIGNALS}; use uucore::InvalidEncodingHandling; static ABOUT: &str = "Send signal to processes or list information about signals."; @@ -37,10 +37,10 @@ pub enum Mode { #[uucore_procs::gen_uumain] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - let args = args + let mut args = args .collect_str(InvalidEncodingHandling::Ignore) .accept_any(); - let (args, obs_signal) = handle_obsolete(args); + let obs_signal = handle_obsolete(&mut args); let usage = format!("{} [OPTIONS]... PID...", uucore::execution_phrase()); let matches = uu_app().usage(&usage[..]).get_matches_from(args); @@ -60,13 +60,15 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { match mode { Mode::Kill => { - let sig = match (obs_signal, matches.value_of(options::SIGNAL)) { - (Some(s), Some(_)) => s, // -s takes precedence - (Some(s), None) => s, - (None, Some(s)) => s.to_owned(), - (None, None) => "TERM".to_owned(), + let sig = if let Some(signal) = obs_signal { + signal + } else if let Some(signal) = matches.value_of(options::SIGNAL) { + parse_signal_value(signal)? + } else { + 15_usize //SIGTERM }; - kill(&sig, &pids_or_signals) + let pids = parse_pids(&pids_or_signals)?; + kill(sig, &pids) } Mode::Table => { table(); @@ -109,26 +111,22 @@ pub fn uu_app() -> App<'static, 'static> { ) } -fn handle_obsolete(mut args: Vec) -> (Vec, Option) { - let mut i = 0; - while i < args.len() { - // this is safe because slice is valid when it is referenced - let slice = &args[i].clone(); - if slice.starts_with('-') && slice.chars().nth(1).map_or(false, |c| c.is_digit(10)) { - let val = &slice[1..]; - match val.parse() { - Ok(num) => { - if uucore::signals::is_signal(num) { - args.remove(i); - return (args, Some(val.to_owned())); - } - } - Err(_) => break, /* getopts will error out for us */ +fn handle_obsolete(args: &mut Vec) -> Option { + // Sanity check + if args.len() > 2 { + // Old signal can only be in the first argument position + let slice = args[1].as_str(); + if let Some(signal) = slice.strip_prefix('-') { + // Check if it is a valid signal + let opt_signal = signal_by_name_or_value(signal); + if opt_signal.is_some() { + // remove the signal before return + args.remove(1); + return opt_signal; } } - i += 1; } - (args, None) + None } fn table() { @@ -184,31 +182,32 @@ fn list(arg: Option) -> UResult<()> { } } -fn kill(signalname: &str, pids: &[String]) -> UResult<()> { - let optional_signal_value = uucore::signals::signal_by_name_or_value(signalname); - let signal_value = match optional_signal_value { - Some(x) => x, - None => { - return Err(USimpleError::new( - 1, - format!("unknown signal name {}", signalname.quote()), - )); +fn parse_signal_value(signal_name: &str) -> UResult { + let optional_signal_value = signal_by_name_or_value(signal_name); + match optional_signal_value { + Some(x) => Ok(x), + None => Err(USimpleError::new( + 1, + format!("unknown signal name {}", signal_name.quote()), + )), + } +} + +fn parse_pids(pids: &[String]) -> UResult> { + pids.iter() + .map(|x| { + x.parse::().map_err(|e| { + USimpleError::new(1, format!("failed to parse argument {}: {}", x.quote(), e)) + }) + }) + .collect() +} + +fn kill(signal_value: usize, pids: &[usize]) -> UResult<()> { + for &pid in pids { + if unsafe { libc::kill(pid as pid_t, signal_value as c_int) } != 0 { + show!(USimpleError::new(1, format!("{}", Error::last_os_error()))); } - }; - for pid in pids { - match pid.parse::() { - Ok(x) => { - if unsafe { libc::kill(x as pid_t, signal_value as c_int) } != 0 { - show!(USimpleError::new(1, format!("{}", Error::last_os_error()))); - } - } - Err(e) => { - return Err(USimpleError::new( - 1, - format!("failed to parse argument {}: {}", pid.quote(), e), - )); - } - }; } Ok(()) } From 7e577476d81f36cac9f3dd64df924b3761e53b49 Mon Sep 17 00:00:00 2001 From: Smicry Date: Sun, 12 Sep 2021 23:20:05 +0800 Subject: [PATCH 03/26] 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 04/26] 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 05/26] 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 4555c8556405d18c15385c4d5fbe5dbd328f4ee4 Mon Sep 17 00:00:00 2001 From: Jan Verbeek Date: Fri, 27 Aug 2021 20:36:26 +0200 Subject: [PATCH 06/26] whoami: Cleanup - Use modern conventions - Restrict the scope of unsafe - Do not use deprecated `std::mem::unitialized()` - Do not bake unicode into design --- Cargo.lock | 1 + src/uu/whoami/Cargo.toml | 5 ++- src/uu/whoami/src/platform/unix.rs | 14 ++++---- src/uu/whoami/src/platform/windows.rs | 31 +++++++++-------- src/uu/whoami/src/whoami.rs | 49 +++++++-------------------- 5 files changed, 41 insertions(+), 59 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 91e2f1762..d767526ab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3222,6 +3222,7 @@ name = "uu_whoami" version = "0.0.7" dependencies = [ "clap", + "libc", "uucore", "uucore_procs", "winapi 0.3.9", diff --git a/src/uu/whoami/Cargo.toml b/src/uu/whoami/Cargo.toml index 919aab2e5..d12ea1aea 100644 --- a/src/uu/whoami/Cargo.toml +++ b/src/uu/whoami/Cargo.toml @@ -16,12 +16,15 @@ path = "src/whoami.rs" [dependencies] clap = { version = "2.33", features = ["wrap_help"] } -uucore = { version=">=0.0.9", package="uucore", path="../../uucore", features=["entries", "wide"] } +uucore = { version=">=0.0.9", package="uucore", path="../../uucore", features=["entries"] } uucore_procs = { version=">=0.0.6", package="uucore_procs", path="../../uucore_procs" } [target.'cfg(target_os = "windows")'.dependencies] winapi = { version = "0.3", features = ["lmcons"] } +[target.'cfg(unix)'.dependencies] +libc = "0.2.42" + [[bin]] name = "whoami" path = "src/main.rs" diff --git a/src/uu/whoami/src/platform/unix.rs b/src/uu/whoami/src/platform/unix.rs index 3d5fc6f54..1c0ea15d5 100644 --- a/src/uu/whoami/src/platform/unix.rs +++ b/src/uu/whoami/src/platform/unix.rs @@ -8,14 +8,14 @@ * file that was distributed with this source code. */ -// spell-checker:ignore (ToDO) getusername +use std::ffi::OsString; +use std::io; -use std::io::Result; use uucore::entries::uid2usr; -use uucore::libc::geteuid; -pub unsafe fn get_username() -> Result { - // Get effective user id - let uid = geteuid(); - uid2usr(uid) +pub fn get_username() -> io::Result { + // SAFETY: getuid() does nothing with memory and is always successful. + let uid = unsafe { libc::geteuid() }; + // uid2usr should arguably return an OsString but currently doesn't + uid2usr(uid).map(Into::into) } diff --git a/src/uu/whoami/src/platform/windows.rs b/src/uu/whoami/src/platform/windows.rs index 3fe8eb1e7..8afa18b63 100644 --- a/src/uu/whoami/src/platform/windows.rs +++ b/src/uu/whoami/src/platform/windows.rs @@ -7,22 +7,23 @@ * file that was distributed with this source code. */ -extern crate winapi; +use std::ffi::OsString; +use std::io; +use std::os::windows::ffi::OsStringExt; -use self::winapi::shared::lmcons; -use self::winapi::shared::minwindef; -use self::winapi::um::{winbase, winnt}; -use std::io::{Error, Result}; -use std::mem; -use uucore::wide::FromWide; +use winapi::shared::lmcons; +use winapi::shared::minwindef::DWORD; +use winapi::um::winbase; -pub unsafe fn get_username() -> Result { - #[allow(deprecated)] - let mut buffer: [winnt::WCHAR; lmcons::UNLEN as usize + 1] = mem::uninitialized(); - let mut len = buffer.len() as minwindef::DWORD; - if winbase::GetUserNameW(buffer.as_mut_ptr(), &mut len) == 0 { - return Err(Error::last_os_error()); +pub fn get_username() -> io::Result { + const BUF_LEN: DWORD = lmcons::UNLEN + 1; + let mut buffer = [0_u16; BUF_LEN as usize]; + let mut len = BUF_LEN; + // SAFETY: buffer.len() == len + unsafe { + if winbase::GetUserNameW(buffer.as_mut_ptr(), &mut len) == 0 { + return Err(io::Error::last_os_error()); + } } - let username = String::from_wide(&buffer[..len as usize - 1]); - Ok(username) + Ok(OsString::from_wide(&buffer[..len as usize - 1])) } diff --git a/src/uu/whoami/src/whoami.rs b/src/uu/whoami/src/whoami.rs index 3033a078a..830b86e63 100644 --- a/src/uu/whoami/src/whoami.rs +++ b/src/uu/whoami/src/whoami.rs @@ -1,5 +1,3 @@ -use clap::App; - // * This file is part of the uutils coreutils package. // * // * (c) Jordi Boggiano @@ -14,46 +12,25 @@ extern crate clap; #[macro_use] extern crate uucore; -use uucore::error::{UResult, USimpleError}; +use clap::App; + +use uucore::display::println_verbatim; +use uucore::error::{FromIo, UResult}; mod platform; +static ABOUT: &str = "Print the current username."; + #[uucore_procs::gen_uumain] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - let app = uu_app(); - - if let Err(err) = app.get_matches_from_safe(args) { - if err.kind == clap::ErrorKind::HelpDisplayed - || err.kind == clap::ErrorKind::VersionDisplayed - { - println!("{}", err); - Ok(()) - } else { - return Err(USimpleError::new(1, format!("{}", err))); - } - } else { - exec() - } + uu_app().get_matches_from(args); + let username = platform::get_username().map_err_context(|| "failed to get username".into())?; + println_verbatim(&username).map_err_context(|| "failed to print username".into())?; + Ok(()) } pub fn uu_app() -> App<'static, 'static> { - app_from_crate!() -} - -pub fn exec() -> UResult<()> { - unsafe { - match platform::get_username() { - Ok(username) => { - println!("{}", username); - Ok(()) - } - Err(err) => match err.raw_os_error() { - Some(0) | None => Err(USimpleError::new(1, "failed to get username")), - Some(_) => Err(USimpleError::new( - 1, - format!("failed to get username: {}", err), - )), - }, - } - } + App::new(uucore::util_name()) + .version(crate_version!()) + .about(ABOUT) } From 0a3785bf8411be00e1032b51eb9b599852ccb979 Mon Sep 17 00:00:00 2001 From: Jan Verbeek Date: Tue, 14 Sep 2021 12:25:17 +0200 Subject: [PATCH 07/26] whoami: Run tests on Windows --- tests/by-util/test_whoami.rs | 2 -- tests/common/util.rs | 10 ++++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/by-util/test_whoami.rs b/tests/by-util/test_whoami.rs index 3e8d5afa6..340c1434f 100644 --- a/tests/by-util/test_whoami.rs +++ b/tests/by-util/test_whoami.rs @@ -3,7 +3,6 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -#[cfg(unix)] use crate::common::util::*; #[test] @@ -34,7 +33,6 @@ fn test_normal_compare_id() { } #[test] -#[cfg(unix)] fn test_normal_compare_env() { let whoami = whoami(); if whoami == "nobody" { diff --git a/tests/common/util.rs b/tests/common/util.rs index 704e9b163..f3cdec010 100644 --- a/tests/common/util.rs +++ b/tests/common/util.rs @@ -1069,10 +1069,12 @@ pub fn whoami() -> String { // Use environment variable to get current user instead of // invoking `whoami` and fall back to user "nobody" on error. - std::env::var("USER").unwrap_or_else(|e| { - println!("{}: {}, using \"nobody\" instead", UUTILS_WARNING, e); - "nobody".to_string() - }) + std::env::var("USER") + .or_else(|_| std::env::var("USERNAME")) + .unwrap_or_else(|e| { + println!("{}: {}, using \"nobody\" instead", UUTILS_WARNING, e); + "nobody".to_string() + }) } /// Add prefix 'g' for `util_name` if not on linux From 89428a77f31c26024980be35ed89e4b7de7983d7 Mon Sep 17 00:00:00 2001 From: Smicry Date: Tue, 14 Sep 2021 23:56:08 +0800 Subject: [PATCH 08/26] 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 09/26] 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 10/26] 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 11/26] 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 12/26] 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 519c0d16b3128cd7d5df7be3b6ea4ae289dcc69f Mon Sep 17 00:00:00 2001 From: Jan Verbeek Date: Tue, 14 Sep 2021 21:14:49 +0200 Subject: [PATCH 13/26] uucore::utmpx: Make thread-safe --- src/uu/users/src/users.rs | 15 ++-- src/uu/who/src/who.rs | 5 +- src/uucore/src/lib/features/utmpx.rs | 102 +++++++++++++++++++++------ 3 files changed, 92 insertions(+), 30 deletions(-) diff --git a/src/uu/users/src/users.rs b/src/uu/users/src/users.rs index 866093189..d374df181 100644 --- a/src/uu/users/src/users.rs +++ b/src/uu/users/src/users.rs @@ -8,6 +8,8 @@ // spell-checker:ignore (paths) wtmp +use std::path::Path; + use clap::{crate_version, App, Arg}; use uucore::utmpx::{self, Utmpx}; @@ -36,19 +38,18 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .after_help(&after_help[..]) .get_matches_from(args); - let files: Vec = matches - .values_of(ARG_FILES) - .map(|v| v.map(ToString::to_string).collect()) + let files: Vec<&Path> = matches + .values_of_os(ARG_FILES) + .map(|v| v.map(AsRef::as_ref).collect()) .unwrap_or_default(); let filename = if !files.is_empty() { - files[0].as_ref() + files[0] } else { - utmpx::DEFAULT_FILE + utmpx::DEFAULT_FILE.as_ref() }; - let mut users = Utmpx::iter_all_records() - .read_from(filename) + let mut users = Utmpx::iter_all_records_from(filename) .filter(Utmpx::is_user_process) .map(|ut| ut.user()) .collect::>(); diff --git a/src/uu/who/src/who.rs b/src/uu/who/src/who.rs index d61747127..a975c82ba 100644 --- a/src/uu/who/src/who.rs +++ b/src/uu/who/src/who.rs @@ -341,15 +341,14 @@ impl Who { utmpx::DEFAULT_FILE }; if self.short_list { - let users = Utmpx::iter_all_records() - .read_from(f) + let users = Utmpx::iter_all_records_from(f) .filter(Utmpx::is_user_process) .map(|ut| ut.user()) .collect::>(); println!("{}", users.join(" ")); println!("# users={}", users.len()); } else { - let records = Utmpx::iter_all_records().read_from(f).peekable(); + let records = Utmpx::iter_all_records_from(f).peekable(); if self.include_heading { self.print_heading() diff --git a/src/uucore/src/lib/features/utmpx.rs b/src/uucore/src/lib/features/utmpx.rs index 8f43ba769..a96c3f48c 100644 --- a/src/uucore/src/lib/features/utmpx.rs +++ b/src/uucore/src/lib/features/utmpx.rs @@ -24,7 +24,7 @@ //! //! ``` //! use uucore::utmpx::Utmpx; -//! for ut in Utmpx::iter_all_records().read_from("/some/where/else") { +//! for ut in Utmpx::iter_all_records_from("/some/where/else") { //! if ut.is_user_process() { //! println!("{}: {}", ut.host(), ut.user()) //! } @@ -35,9 +35,12 @@ pub extern crate time; use self::time::{Timespec, Tm}; use std::ffi::CString; -use std::io::Error as IOError; use std::io::Result as IOResult; +use std::marker::PhantomData; +use std::os::unix::ffi::OsStrExt; +use std::path::Path; use std::ptr; +use std::sync::{Mutex, MutexGuard}; pub use self::ut::*; use libc::utmpx; @@ -54,7 +57,9 @@ pub unsafe extern "C" fn utmpxname(_file: *const libc::c_char) -> libc::c_int { 0 } -pub use crate::*; // import macros from `../../macros.rs` +use once_cell::sync::Lazy; + +use crate::*; // import macros from `../../macros.rs` // In case the c_char array doesn't end with NULL macro_rules! chars2string { @@ -248,30 +253,76 @@ impl Utmpx { Ok(host.to_string()) } + /// Iterate through all the utmp records. + /// + /// This will use the default location, or the path [`Utmpx::iter_all_records_from`] + /// was most recently called with. + /// + /// Only one instance of [`UtmpxIter`] may be active at a time. This + /// function will block as long as one is still active. Beware! pub fn iter_all_records() -> UtmpxIter { - UtmpxIter + let iter = UtmpxIter::new(); + unsafe { + // This can technically fail, and it would be nice to detect that, + // but it doesn't return anything so we'd have to do nasty things + // with errno. + setutxent(); + } + iter + } + + /// Iterate through all the utmp records from a specific file. + /// + /// No failure is reported or detected. + /// + /// This function affects subsequent calls to [`Utmpx::iter_all_records`]. + /// + /// The same caveats as for [`Utmpx::iter_all_records`] apply. + pub fn iter_all_records_from>(path: P) -> UtmpxIter { + let iter = UtmpxIter::new(); + let path = CString::new(path.as_ref().as_os_str().as_bytes()).unwrap(); + unsafe { + // In glibc, utmpxname() only fails if there's not enough memory + // to copy the string. + // Solaris returns 1 on success instead of 0. Supposedly there also + // exist systems where it returns void. + // GNU who on Debian seems to output nothing if an invalid filename + // is specified, no warning or anything. + // So this function is pretty crazy and we don't try to detect errors. + // Not much we can do besides pray. + utmpxname(path.as_ptr()); + setutxent(); + } + iter } } +// On some systems these functions are not thread-safe. On others they're +// thread-local. Therefore we use a mutex to allow only one guard to exist at +// a time, and make sure UtmpxIter cannot be sent across threads. +// +// I believe the only technical memory unsafety that could happen is a data +// race while copying the data out of the pointer returned by getutxent(), but +// ordinary race conditions are also very much possible. +static LOCK: Lazy> = Lazy::new(|| Mutex::new(())); + /// Iterator of login records -pub struct UtmpxIter; +pub struct UtmpxIter { + #[allow(dead_code)] + guard: MutexGuard<'static, ()>, + /// Ensure UtmpxIter is !Send. Technically redundant because MutexGuard + /// is also !Send. + phantom: PhantomData>, +} impl UtmpxIter { - /// Sets the name of the utmpx-format file for the other utmpx functions to access. - /// - /// If not set, default record file will be used(file path depends on the target OS) - pub fn read_from(self, f: &str) -> Self { - let res = unsafe { - let cstring = CString::new(f).unwrap(); - utmpxname(cstring.as_ptr()) - }; - if res != 0 { - show_warning!("utmpxname: {}", IOError::last_os_error()); + fn new() -> Self { + // PoisonErrors can safely be ignored + let guard = LOCK.lock().unwrap_or_else(|err| err.into_inner()); + UtmpxIter { + guard, + phantom: PhantomData, } - unsafe { - setutxent(); - } - self } } @@ -281,13 +332,24 @@ impl Iterator for UtmpxIter { unsafe { let res = getutxent(); if !res.is_null() { + // The data behind this pointer will be replaced by the next + // call to getutxent(), so we have to read it now. + // All the strings live inline in the struct as arrays, which + // makes things easier. Some(Utmpx { inner: ptr::read(res as *const _), }) } else { - endutxent(); None } } } } + +impl Drop for UtmpxIter { + fn drop(&mut self) { + unsafe { + endutxent(); + } + } +} From 53a91be2dfe11a87c05c5de7d085e81291e8909d Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Tue, 14 Sep 2021 20:54:31 -0400 Subject: [PATCH 14/26] seq: add is_first_iteration to avoid comparisons Add the `is_first_iteration` Boolean variable to the `print_seq()` function in order to avoid unnecessary comparisons. Specifically, before this change, the `done_printing()` function was called twice on each iteration of the main loop. After this change, it is only called once per iteration. Furthermore, this change makes the `print_seq()` function similar in structure to the `print_seq_integers()` function. Co-authored-by: Jan Verbeek --- src/uu/seq/src/seq.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/uu/seq/src/seq.rs b/src/uu/seq/src/seq.rs index 8aef226b5..0cea90838 100644 --- a/src/uu/seq/src/seq.rs +++ b/src/uu/seq/src/seq.rs @@ -287,7 +287,12 @@ fn print_seq( let (first, increment, last) = range; let mut i = 0isize; let mut value = first + i as f64 * increment; + let mut is_first_iteration = true; while !done_printing(&value, &increment, &last) { + if !is_first_iteration { + write!(stdout, "{}", separator)?; + } + is_first_iteration = false; let istr = format!("{:.*}", largest_dec, value); let ilen = istr.len(); let before_dec = istr.find('.').unwrap_or(ilen); @@ -299,11 +304,8 @@ fn print_seq( write!(stdout, "{}", istr)?; i += 1; value = first + i as f64 * increment; - if !done_printing(&value, &increment, &last) { - write!(stdout, "{}", separator)?; - } } - if (first >= last && increment < 0f64) || (first <= last && increment > 0f64) { + if !is_first_iteration { write!(stdout, "{}", terminator)?; } stdout.flush()?; From f95ab2f43caee95fe05b9f28deabd28bb34c92b5 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Tue, 14 Sep 2021 21:26:50 -0400 Subject: [PATCH 15/26] uucore(panic): guard against "Broken pipe" panics Add "Broken pipe" to the set of panic messages used to determine whether a panic is caused by a broken pipe error. --- src/uucore/src/lib/mods/panic.rs | 41 +++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/src/uucore/src/lib/mods/panic.rs b/src/uucore/src/lib/mods/panic.rs index ba0ecdf12..ebba10429 100644 --- a/src/uucore/src/lib/mods/panic.rs +++ b/src/uucore/src/lib/mods/panic.rs @@ -1,17 +1,42 @@ +//! Custom panic hooks that allow silencing certain types of errors. +//! +//! Use the [`mute_sigpipe_panic`] function to silence panics caused by +//! broken pipe errors. This can happen when a process is still +//! producing data when the consuming process terminates and closes the +//! pipe. For example, +//! +//! ```sh +//! $ seq inf | head -n 1 +//! ``` +//! use std::panic; +use std::panic::PanicInfo; -//## SIGPIPE handling background/discussions ... -//* `uutils` ~ , -//* rust and `rg` ~ , , +/// Decide whether a panic was caused by a broken pipe (SIGPIPE) error. +fn is_broken_pipe(info: &PanicInfo) -> bool { + if let Some(res) = info.payload().downcast_ref::() { + if res.contains("BrokenPipe") || res.contains("Broken pipe") { + return true; + } + } + false +} +/// Terminate without error on panics that occur due to broken pipe errors. +/// +/// For background discussions on `SIGPIPE` handling, see +/// +/// * https://github.com/uutils/coreutils/issues/374 +/// * https://github.com/uutils/coreutils/pull/1106 +/// * https://github.com/rust-lang/rust/issues/62569 +/// * https://github.com/BurntSushi/ripgrep/issues/200 +/// * https://github.com/crev-dev/cargo-crev/issues/287 +/// pub fn mute_sigpipe_panic() { let hook = panic::take_hook(); panic::set_hook(Box::new(move |info| { - if let Some(res) = info.payload().downcast_ref::() { - if res.contains("BrokenPipe") { - return; - } + if !is_broken_pipe(info) { + hook(info) } - hook(info) })); } From 3f37ddbd22803a991ece4fa3fc78fda02e776697 Mon Sep 17 00:00:00 2001 From: Jan Verbeek Date: Sat, 28 Aug 2021 13:47:31 +0200 Subject: [PATCH 16/26] hostname: Cleanup - Attach context to I/O errors - Make flags override each other - Support invalid unicode as argument - Call WsaCleanup() even on panic - Do not use deprecated std::mem::uninitialized() --- .../workspace.wordlist.txt | 2 + src/uu/hostname/src/hostname.rs | 136 +++++++++--------- 2 files changed, 73 insertions(+), 65 deletions(-) diff --git a/.vscode/cspell.dictionaries/workspace.wordlist.txt b/.vscode/cspell.dictionaries/workspace.wordlist.txt index 29957fb12..d37a59465 100644 --- a/.vscode/cspell.dictionaries/workspace.wordlist.txt +++ b/.vscode/cspell.dictionaries/workspace.wordlist.txt @@ -68,6 +68,7 @@ structs substr splitn trunc +uninit # * uutils basenc @@ -277,6 +278,7 @@ ULONG ULONGLONG UNLEN WCHAR +WSADATA errhandlingapi fileapi handleapi diff --git a/src/uu/hostname/src/hostname.rs b/src/uu/hostname/src/hostname.rs index 8852e0f43..2de6627e8 100644 --- a/src/uu/hostname/src/hostname.rs +++ b/src/uu/hostname/src/hostname.rs @@ -10,18 +10,13 @@ #[macro_use] extern crate uucore; -use clap::{crate_version, App, Arg, ArgMatches}; use std::collections::hash_set::HashSet; use std::net::ToSocketAddrs; use std::str; -#[cfg(windows)] -use uucore::error::UUsageError; -use uucore::error::{UResult, USimpleError}; -#[cfg(windows)] -use winapi::shared::minwindef::MAKEWORD; -#[cfg(windows)] -use winapi::um::winsock2::{WSACleanup, WSAStartup}; +use clap::{crate_version, App, Arg, ArgMatches}; + +use uucore::error::{FromIo, UResult}; static ABOUT: &str = "Display or set the system's host name."; @@ -31,45 +26,52 @@ static OPT_FQDN: &str = "fqdn"; static OPT_SHORT: &str = "short"; static OPT_HOST: &str = "host"; -#[uucore_procs::gen_uumain] -pub fn uumain(args: impl uucore::Args) -> UResult<()> { - #![allow(clippy::let_and_return)] - #[cfg(windows)] - unsafe { - #[allow(deprecated)] - let mut data = std::mem::uninitialized(); - if WSAStartup(MAKEWORD(2, 2), &mut data as *mut _) != 0 { - return Err(UUsageError::new( - 1, - "Failed to start Winsock 2.2".to_string(), - )); +#[cfg(windows)] +mod wsa { + use std::io; + + use winapi::shared::minwindef::MAKEWORD; + use winapi::um::winsock2::{WSACleanup, WSAStartup, WSADATA}; + + pub(super) struct WsaHandle(()); + + pub(super) fn start() -> io::Result { + let err = unsafe { + let mut data = std::mem::MaybeUninit::::uninit(); + WSAStartup(MAKEWORD(2, 2), data.as_mut_ptr()) + }; + if err != 0 { + Err(io::Error::from_raw_os_error(err)) + } else { + Ok(WsaHandle(())) } } - let result = execute(args); - #[cfg(windows)] - unsafe { - WSACleanup(); + + impl Drop for WsaHandle { + fn drop(&mut self) { + unsafe { + // This possibly returns an error but we can't handle it + let _err = WSACleanup(); + } + } } - result } fn usage() -> String { format!("{0} [OPTION]... [HOSTNAME]", uucore::execution_phrase()) } -fn execute(args: impl uucore::Args) -> UResult<()> { +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { let usage = usage(); let matches = uu_app().usage(&usage[..]).get_matches_from(args); - match matches.value_of(OPT_HOST) { + #[cfg(windows)] + let _handle = wsa::start().map_err_context(|| "failed to start Winsock".to_owned())?; + + match matches.value_of_os(OPT_HOST) { None => display_hostname(&matches), - Some(host) => { - if let Err(err) = hostname::set(host) { - return Err(USimpleError::new(1, format!("{}", err))); - } else { - Ok(()) - } - } + Some(host) => hostname::set(host).map_err_context(|| "failed to set hostname".to_owned()), } } @@ -81,64 +83,68 @@ pub fn uu_app() -> App<'static, 'static> { Arg::with_name(OPT_DOMAIN) .short("d") .long("domain") + .overrides_with_all(&[OPT_DOMAIN, OPT_IP_ADDRESS, OPT_FQDN, OPT_SHORT]) .help("Display the name of the DNS domain if possible"), ) .arg( Arg::with_name(OPT_IP_ADDRESS) .short("i") .long("ip-address") + .overrides_with_all(&[OPT_DOMAIN, OPT_IP_ADDRESS, OPT_FQDN, OPT_SHORT]) .help("Display the network address(es) of the host"), ) - // TODO: support --long .arg( Arg::with_name(OPT_FQDN) .short("f") .long("fqdn") + .overrides_with_all(&[OPT_DOMAIN, OPT_IP_ADDRESS, OPT_FQDN, OPT_SHORT]) .help("Display the FQDN (Fully Qualified Domain Name) (default)"), ) - .arg(Arg::with_name(OPT_SHORT).short("s").long("short").help( - "Display the short hostname (the portion before the first dot) if \ - possible", - )) + .arg( + Arg::with_name(OPT_SHORT) + .short("s") + .long("short") + .overrides_with_all(&[OPT_DOMAIN, OPT_IP_ADDRESS, OPT_FQDN, OPT_SHORT]) + .help("Display the short hostname (the portion before the first dot) if possible"), + ) .arg(Arg::with_name(OPT_HOST)) } fn display_hostname(matches: &ArgMatches) -> UResult<()> { - let hostname = hostname::get().unwrap().into_string().unwrap(); + let hostname = hostname::get() + .map_err_context(|| "failed to get hostname".to_owned())? + .to_string_lossy() + .into_owned(); if matches.is_present(OPT_IP_ADDRESS) { // XXX: to_socket_addrs needs hostname:port so append a dummy port and remove it later. // This was originally supposed to use std::net::lookup_host, but that seems to be // deprecated. Perhaps we should use the dns-lookup crate? let hostname = hostname + ":1"; - match hostname.to_socket_addrs() { - Ok(addresses) => { - let mut hashset = HashSet::new(); - let mut output = String::new(); - for addr in addresses { - // XXX: not sure why this is necessary... - if !hashset.contains(&addr) { - let mut ip = format!("{}", addr); - if ip.ends_with(":1") { - let len = ip.len(); - ip.truncate(len - 2); - } - output.push_str(&ip); - output.push(' '); - hashset.insert(addr); - } + let addresses = hostname + .to_socket_addrs() + .map_err_context(|| "failed to resolve socket addresses".to_owned())?; + let mut hashset = HashSet::new(); + let mut output = String::new(); + for addr in addresses { + // XXX: not sure why this is necessary... + if !hashset.contains(&addr) { + let mut ip = addr.to_string(); + if ip.ends_with(":1") { + let len = ip.len(); + ip.truncate(len - 2); } - let len = output.len(); - if len > 0 { - println!("{}", &output[0..len - 1]); - } - - Ok(()) - } - Err(f) => { - return Err(USimpleError::new(1, format!("{}", f))); + output.push_str(&ip); + output.push(' '); + hashset.insert(addr); } } + let len = output.len(); + if len > 0 { + println!("{}", &output[0..len - 1]); + } + + Ok(()) } else { if matches.is_present(OPT_SHORT) || matches.is_present(OPT_DOMAIN) { let mut it = hostname.char_indices().filter(|&ci| ci.1 == '.'); From 5bb56ec528f4ed29a021777a4a36ecd4e765fd27 Mon Sep 17 00:00:00 2001 From: Jan Verbeek Date: Wed, 15 Sep 2021 15:37:15 +0200 Subject: [PATCH 17/26] whoami: Restrict scope of unsafe Co-authored-by: Jan Scheer --- src/uu/whoami/src/platform/windows.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/uu/whoami/src/platform/windows.rs b/src/uu/whoami/src/platform/windows.rs index 8afa18b63..a627bed8e 100644 --- a/src/uu/whoami/src/platform/windows.rs +++ b/src/uu/whoami/src/platform/windows.rs @@ -20,10 +20,8 @@ pub fn get_username() -> io::Result { let mut buffer = [0_u16; BUF_LEN as usize]; let mut len = BUF_LEN; // SAFETY: buffer.len() == len - unsafe { - if winbase::GetUserNameW(buffer.as_mut_ptr(), &mut len) == 0 { - return Err(io::Error::last_os_error()); - } + if unsafe { winbase::GetUserNameW(buffer.as_mut_ptr(), &mut len) } == 0 { + return Err(io::Error::last_os_error()); } Ok(OsString::from_wide(&buffer[..len as usize - 1])) } From cd5f676903606f8623fbc921489ddab7419bbadf Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Thu, 16 Sep 2021 22:33:04 -0400 Subject: [PATCH 18/26] hashsum: add tests for Windows text mode --- tests/by-util/test_hashsum.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/by-util/test_hashsum.rs b/tests/by-util/test_hashsum.rs index f2cf91d45..545b4ee78 100644 --- a/tests/by-util/test_hashsum.rs +++ b/tests/by-util/test_hashsum.rs @@ -38,6 +38,30 @@ macro_rules! test_digest { .no_stderr() .stdout_is("input.txt: OK\n"); } + + #[cfg(windows)] + #[test] + fn test_text_mode() { + // TODO Replace this with hard-coded files that store the + // expected output of text mode on an input file that has + // "\r\n" line endings. + let result = new_ucmd!() + .args(&[DIGEST_ARG, BITS_ARG, "-b"]) + .pipe_in("a\nb\nc\n") + .succeeds(); + let expected = result.no_stderr().stdout(); + // Replace the "*-\n" at the end of the output with " -\n". + // The asterisk indicates that the digest was computed in + // binary mode. + let n = expected.len(); + let expected = [&expected[..n - 3], &[b' ', b'-', b'\n']].concat(); + new_ucmd!() + .args(&[DIGEST_ARG, BITS_ARG, "-t"]) + .pipe_in("a\r\nb\r\nc\r\n") + .succeeds() + .no_stderr() + .stdout_is(std::str::from_utf8(&expected).unwrap()); + } } )*) } From 2ac5dc0a70b03d68faba89486b9fc15490affee0 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sat, 11 Sep 2021 12:49:12 -0400 Subject: [PATCH 19/26] seq: compute correct width for scientific notation Change the way `seq` computes the number of digits needed to print a number so that it works for inputs given in scientific notation. Specifically, this commit parses the input string to determine whether it is an integer, a float in decimal notation, or a float in scientific notation, and then computes the number of integral digits and the number of fractional digits based on that. This also supports floating point negative zero, expressed in both decimal and scientific notation. --- src/uu/seq/src/digits.rs | 190 ++++++++++++++++++++++++++++++++++++++ src/uu/seq/src/seq.rs | 72 ++++++++++++--- tests/by-util/test_seq.rs | 134 +++++++++++++++++++++++++++ 3 files changed, 381 insertions(+), 15 deletions(-) create mode 100644 src/uu/seq/src/digits.rs diff --git a/src/uu/seq/src/digits.rs b/src/uu/seq/src/digits.rs new file mode 100644 index 000000000..bde933978 --- /dev/null +++ b/src/uu/seq/src/digits.rs @@ -0,0 +1,190 @@ +//! Counting number of digits needed to represent a number. +//! +//! The [`num_integral_digits`] and [`num_fractional_digits`] functions +//! count the number of digits needed to represent a number in decimal +//! notation (like "123.456"). +use std::convert::TryInto; +use std::num::ParseIntError; + +use uucore::display::Quotable; + +/// The number of digits after the decimal point in a given number. +/// +/// The input `s` is a string representing a number, either an integer +/// or a floating point number in either decimal notation or scientific +/// notation. This function returns the number of digits after the +/// decimal point needed to print the number in decimal notation. +/// +/// # Examples +/// +/// ```rust,ignore +/// assert_eq!(num_fractional_digits("123.45e-1").unwrap(), 3); +/// ``` +pub fn num_fractional_digits(s: &str) -> Result { + match (s.find('.'), s.find('e')) { + // For example, "123456". + (None, None) => Ok(0), + + // For example, "123e456". + (None, Some(j)) => { + let exponent: i64 = s[j + 1..].parse()?; + if exponent < 0 { + Ok(-exponent as usize) + } else { + Ok(0) + } + } + + // For example, "123.456". + (Some(i), None) => Ok(s.len() - (i + 1)), + + // For example, "123.456e789". + (Some(i), Some(j)) if i < j => { + // Because of the match guard, this subtraction will not underflow. + let num_digits_between_decimal_point_and_e = (j - (i + 1)) as i64; + let exponent: i64 = s[j + 1..].parse()?; + if num_digits_between_decimal_point_and_e < exponent { + Ok(0) + } else { + Ok((num_digits_between_decimal_point_and_e - exponent) + .try_into() + .unwrap()) + } + } + _ => crash!( + 1, + "invalid floating point argument: {}\n Try '{} --help' for more information.", + s.quote(), + uucore::execution_phrase() + ), + } +} + +/// The number of digits before the decimal point in a given number. +/// +/// The input `s` is a string representing a number, either an integer +/// or a floating point number in either decimal notation or scientific +/// notation. This function returns the number of digits before the +/// decimal point needed to print the number in decimal notation. +/// +/// # Examples +/// +/// ```rust,ignore +/// assert_eq!(num_fractional_digits("123.45e-1").unwrap(), 2); +/// ``` +pub fn num_integral_digits(s: &str) -> Result { + match (s.find('.'), s.find('e')) { + // For example, "123456". + (None, None) => Ok(s.len()), + + // For example, "123e456". + (None, Some(j)) => { + let exponent: i64 = s[j + 1..].parse()?; + let total = j as i64 + exponent; + if total < 1 { + Ok(1) + } else { + Ok(total.try_into().unwrap()) + } + } + + // For example, "123.456". + (Some(i), None) => Ok(i), + + // For example, "123.456e789". + (Some(i), Some(j)) => { + let exponent: i64 = s[j + 1..].parse()?; + let minimum: usize = { + let integral_part: f64 = crash_if_err!(1, s[..j].parse()); + if integral_part == -0.0 && integral_part.is_sign_negative() { + 2 + } else { + 1 + } + }; + + let total = i as i64 + exponent; + if total < minimum as i64 { + Ok(minimum) + } else { + Ok(total.try_into().unwrap()) + } + } + } +} + +#[cfg(test)] +mod tests { + + mod test_num_integral_digits { + use crate::num_integral_digits; + + #[test] + fn test_integer() { + assert_eq!(num_integral_digits("123").unwrap(), 3); + } + + #[test] + fn test_decimal() { + assert_eq!(num_integral_digits("123.45").unwrap(), 3); + } + + #[test] + fn test_scientific_no_decimal_positive_exponent() { + assert_eq!(num_integral_digits("123e4").unwrap(), 3 + 4); + } + + #[test] + fn test_scientific_with_decimal_positive_exponent() { + assert_eq!(num_integral_digits("123.45e6").unwrap(), 3 + 6); + } + + #[test] + fn test_scientific_no_decimal_negative_exponent() { + assert_eq!(num_integral_digits("123e-4").unwrap(), 1); + } + + #[test] + fn test_scientific_with_decimal_negative_exponent() { + assert_eq!(num_integral_digits("123.45e-6").unwrap(), 1); + assert_eq!(num_integral_digits("123.45e-1").unwrap(), 2); + } + } + + mod test_num_fractional_digits { + use crate::num_fractional_digits; + + #[test] + fn test_integer() { + assert_eq!(num_fractional_digits("123").unwrap(), 0); + } + + #[test] + fn test_decimal() { + assert_eq!(num_fractional_digits("123.45").unwrap(), 2); + } + + #[test] + fn test_scientific_no_decimal_positive_exponent() { + assert_eq!(num_fractional_digits("123e4").unwrap(), 0); + } + + #[test] + fn test_scientific_with_decimal_positive_exponent() { + assert_eq!(num_fractional_digits("123.45e6").unwrap(), 0); + assert_eq!(num_fractional_digits("123.45e1").unwrap(), 1); + } + + #[test] + fn test_scientific_no_decimal_negative_exponent() { + assert_eq!(num_fractional_digits("123e-4").unwrap(), 4); + assert_eq!(num_fractional_digits("123e-1").unwrap(), 1); + } + + #[test] + fn test_scientific_with_decimal_negative_exponent() { + assert_eq!(num_fractional_digits("123.45e-6").unwrap(), 8); + assert_eq!(num_fractional_digits("123.45e-1").unwrap(), 3); + } + } +} diff --git a/src/uu/seq/src/seq.rs b/src/uu/seq/src/seq.rs index 0cea90838..18f181aa8 100644 --- a/src/uu/seq/src/seq.rs +++ b/src/uu/seq/src/seq.rs @@ -14,6 +14,11 @@ use num_traits::{Num, ToPrimitive}; use std::cmp; use std::io::{stdout, ErrorKind, Write}; use std::str::FromStr; + +mod digits; +use crate::digits::num_fractional_digits; +use crate::digits::num_integral_digits; + use uucore::display::Quotable; static ABOUT: &str = "Display numbers from FIRST to LAST, in steps of INCREMENT."; @@ -155,20 +160,49 @@ pub fn uumain(args: impl uucore::Args) -> i32 { }; let mut largest_dec = 0; + let mut padding = 0; let first = if numbers.len() > 1 { let slice = numbers[0]; - let len = slice.len(); - let dec = slice.find('.').unwrap_or(len); - largest_dec = len - dec; + largest_dec = num_fractional_digits(slice).unwrap_or_else(|_| { + crash!( + 1, + "invalid floating point argument: {}\n Try '{} --help' for more information.", + slice.quote(), + uucore::execution_phrase() + ) + }); + padding = num_integral_digits(slice).unwrap_or_else(|_| { + crash!( + 1, + "invalid floating point argument: {}\n Try '{} --help' for more information.", + slice.quote(), + uucore::execution_phrase() + ) + }); crash_if_err!(1, slice.parse()) } else { Number::BigInt(BigInt::one()) }; let increment = if numbers.len() > 2 { let slice = numbers[1]; - let len = slice.len(); - let dec = slice.find('.').unwrap_or(len); - largest_dec = cmp::max(largest_dec, len - dec); + let dec = num_fractional_digits(slice).unwrap_or_else(|_| { + crash!( + 1, + "invalid floating point argument: {}\n Try '{} --help' for more information.", + slice.quote(), + uucore::execution_phrase() + ) + }); + let int_digits = num_integral_digits(slice).unwrap_or_else(|_| { + crash!( + 1, + "invalid floating point argument: {}\n Try '{} --help' for more information.", + slice.quote(), + uucore::execution_phrase() + ) + }); + largest_dec = cmp::max(largest_dec, dec); + padding = cmp::max(padding, int_digits); crash_if_err!(1, slice.parse()) } else { Number::BigInt(BigInt::one()) @@ -183,16 +217,18 @@ pub fn uumain(args: impl uucore::Args) -> i32 { } let last: Number = { let slice = numbers[numbers.len() - 1]; + let int_digits = num_integral_digits(slice).unwrap_or_else(|_| { + crash!( + 1, + "invalid floating point argument: {}\n Try '{} --help' for more information.", + slice.quote(), + uucore::execution_phrase() + ) + }); + padding = cmp::max(padding, int_digits); crash_if_err!(1, slice.parse()) }; - if largest_dec > 0 { - largest_dec -= 1; - } - let padding = first - .num_digits() - .max(increment.num_digits()) - .max(last.num_digits()); let result = match (first, last, increment) { (Number::MinusZero, Number::BigInt(last), Number::BigInt(increment)) => print_seq_integers( (BigInt::zero(), increment, last), @@ -286,18 +322,24 @@ fn print_seq( let mut stdout = stdout.lock(); let (first, increment, last) = range; let mut i = 0isize; + let is_first_minus_zero = first == -0.0 && first.is_sign_negative(); let mut value = first + i as f64 * increment; let mut is_first_iteration = true; while !done_printing(&value, &increment, &last) { if !is_first_iteration { write!(stdout, "{}", separator)?; } + let mut width = padding; + if is_first_iteration && is_first_minus_zero { + write!(stdout, "-")?; + width -= 1; + } is_first_iteration = false; let istr = format!("{:.*}", largest_dec, value); let ilen = istr.len(); let before_dec = istr.find('.').unwrap_or(ilen); - if pad && before_dec < padding { - for _ in 0..(padding - before_dec) { + if pad && before_dec < width { + for _ in 0..(width - before_dec) { write!(stdout, "0")?; } } diff --git a/tests/by-util/test_seq.rs b/tests/by-util/test_seq.rs index 65f2451f0..7a58a950a 100644 --- a/tests/by-util/test_seq.rs +++ b/tests/by-util/test_seq.rs @@ -23,6 +23,56 @@ fn test_rejects_non_floats() { )); } +#[test] +fn test_invalid_float() { + new_ucmd!() + .args(&["1e2.3"]) + .fails() + .no_stdout() + .stderr_contains("invalid floating point argument: '1e2.3'") + .stderr_contains("for more information."); + new_ucmd!() + .args(&["1e2.3", "2"]) + .fails() + .no_stdout() + .stderr_contains("invalid floating point argument: '1e2.3'") + .stderr_contains("for more information."); + new_ucmd!() + .args(&["1", "1e2.3"]) + .fails() + .no_stdout() + .stderr_contains("invalid floating point argument: '1e2.3'") + .stderr_contains("for more information."); + new_ucmd!() + .args(&["1e2.3", "2", "3"]) + .fails() + .no_stdout() + .stderr_contains("invalid floating point argument: '1e2.3'") + .stderr_contains("for more information."); + new_ucmd!() + .args(&["1", "1e2.3", "3"]) + .fails() + .no_stdout() + .stderr_contains("invalid floating point argument: '1e2.3'") + .stderr_contains("for more information."); + new_ucmd!() + .args(&["1", "2", "1e2.3"]) + .fails() + .no_stdout() + .stderr_contains("invalid floating point argument: '1e2.3'") + .stderr_contains("for more information."); +} + +#[test] +fn test_width_invalid_float() { + new_ucmd!() + .args(&["-w", "1e2.3"]) + .fails() + .no_stdout() + .stderr_contains("invalid floating point argument: '1e2.3'") + .stderr_contains("for more information."); +} + // ---- Tests for the big integer based path ---- #[test] @@ -178,6 +228,90 @@ fn test_width_negative_zero() { .no_stderr(); } +#[test] +fn test_width_negative_zero_scientific_notation() { + new_ucmd!() + .args(&["-w", "-0e0", "1"]) + .succeeds() + .stdout_is("-0\n01\n") + .no_stderr(); + + new_ucmd!() + .args(&["-w", "-0e+1", "1"]) + .succeeds() + .stdout_is("-00\n001\n") + .no_stderr(); + + new_ucmd!() + .args(&["-w", "-0.000e0", "1"]) + .succeeds() + .stdout_is("-0.000\n01.000\n") + .no_stderr(); + + new_ucmd!() + .args(&["-w", "-0.000e-2", "1"]) + .succeeds() + .stdout_is("-0.00000\n01.00000\n") + .no_stderr(); + + new_ucmd!() + .args(&["-w", "-0.000e5", "1"]) + .succeeds() + .stdout_is("-000000\n0000001\n") + .no_stderr(); + + new_ucmd!() + .args(&["-w", "-0.000e5", "1"]) + .succeeds() + .stdout_is("-000000\n0000001\n") + .no_stderr(); +} + +#[test] +fn test_width_decimal_scientific_notation_increment() { + new_ucmd!() + .args(&["-w", ".1", "1e-2", ".11"]) + .succeeds() + .stdout_is("0.10\n0.11\n") + .no_stderr(); + + new_ucmd!() + .args(&["-w", ".0", "1.500e-1", ".2"]) + .succeeds() + .stdout_is("0.0000\n0.1500\n") + .no_stderr(); +} + +/// Test that trailing zeros in the start argument contribute to precision. +#[test] +fn test_width_decimal_scientific_notation_trailing_zeros_start() { + new_ucmd!() + .args(&["-w", ".1000", "1e-2", ".11"]) + .succeeds() + .stdout_is("0.1000\n0.1100\n") + .no_stderr(); +} + +/// Test that trailing zeros in the increment argument contribute to precision. +#[test] +fn test_width_decimal_scientific_notation_trailing_zeros_increment() { + new_ucmd!() + .args(&["-w", "1e-1", "0.0100", ".11"]) + .succeeds() + .stdout_is("0.1000\n0.1100\n") + .no_stderr(); +} + +/// Test that trailing zeros in the end argument do not contribute to width. +#[test] +fn test_width_decimal_scientific_notation_trailing_zeros_end() { + new_ucmd!() + .args(&["-w", "1e-1", "1e-2", ".1100"]) + .succeeds() + .stdout_is("0.10\n0.11\n") + .no_stderr(); +} + // TODO This is duplicated from `test_yes.rs`; refactor them. /// Run `seq`, capture some of the output, close the pipe, and verify it. fn run(args: &[&str], expected: &[u8]) { From 3854a97749cd6f5fd00a2be8ae2cde2b2bd3a010 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sat, 11 Sep 2021 12:57:14 -0400 Subject: [PATCH 20/26] seq: remove unused Number::num_digits() function Remove the `Number::num_digits()` function in favor of the `digits::num_integral_digits()` functions. --- src/uu/seq/src/seq.rs | 52 ------------------------------------------- 1 file changed, 52 deletions(-) diff --git a/src/uu/seq/src/seq.rs b/src/uu/seq/src/seq.rs index 18f181aa8..8bf6cb008 100644 --- a/src/uu/seq/src/seq.rs +++ b/src/uu/seq/src/seq.rs @@ -67,38 +67,6 @@ impl Number { Number::F64(n) => n, } } - - /// Number of characters needed to print the integral part of the number. - /// - /// The number of characters includes one character to represent the - /// minus sign ("-") if this number is negative. - /// - /// # Examples - /// - /// ```rust,ignore - /// use num_bigint::{BigInt, Sign}; - /// - /// assert_eq!( - /// Number::BigInt(BigInt::new(Sign::Plus, vec![123])).num_digits(), - /// 3 - /// ); - /// assert_eq!( - /// Number::BigInt(BigInt::new(Sign::Minus, vec![123])).num_digits(), - /// 4 - /// ); - /// assert_eq!(Number::F64(123.45).num_digits(), 3); - /// assert_eq!(Number::MinusZero.num_digits(), 2); - /// ``` - fn num_digits(&self) -> usize { - match self { - Number::MinusZero => 2, - Number::BigInt(n) => n.to_string().len(), - Number::F64(n) => { - let s = n.to_string(); - s.find('.').unwrap_or_else(|| s.len()) - } - } - } } impl FromStr for Number { @@ -404,23 +372,3 @@ fn print_seq_integers( } Ok(()) } - -#[cfg(test)] -mod tests { - use crate::Number; - use num_bigint::{BigInt, Sign}; - - #[test] - fn test_number_num_digits() { - assert_eq!( - Number::BigInt(BigInt::new(Sign::Plus, vec![123])).num_digits(), - 3 - ); - assert_eq!( - Number::BigInt(BigInt::new(Sign::Minus, vec![123])).num_digits(), - 4 - ); - assert_eq!(Number::F64(123.45).num_digits(), 3); - assert_eq!(Number::MinusZero.num_digits(), 2); - } -} From 7fea771f32c0ab68e63a75de8dd4245188908e91 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Tue, 24 Aug 2021 21:34:30 -0400 Subject: [PATCH 21/26] hashsum: use std::io::copy() to simplify digest Create a `DigestWriter` struct that implements `Write` by passing bytes directly to `Digest::input()`, so that `hashsum` can use `std::io::copy()`. Using `std::io::copy()` eliminates some boilerplate code around reading and writing bytes. And defining `DigestWriter` makes it easier to add a `#[cfg(windows)]` guard around the Windows-specific replacement of "\r\n" with "\n". --- src/uu/hashsum/src/digest.rs | 85 +++++++++++++++++++++++++++++++++++ src/uu/hashsum/src/hashsum.rs | 53 ++++++---------------- 2 files changed, 98 insertions(+), 40 deletions(-) diff --git a/src/uu/hashsum/src/digest.rs b/src/uu/hashsum/src/digest.rs index 9093d94a7..531dc7e4f 100644 --- a/src/uu/hashsum/src/digest.rs +++ b/src/uu/hashsum/src/digest.rs @@ -1,10 +1,22 @@ +// spell-checker:ignore memmem +//! Implementations of digest functions, like md5 and sha1. +//! +//! The [`Digest`] trait represents the interface for providing inputs +//! to these digest functions and accessing the resulting hash. The +//! [`DigestWriter`] struct provides a wrapper around [`Digest`] that +//! implements the [`Write`] trait, for use in situations where calling +//! [`write`] would be useful. extern crate digest; extern crate md5; extern crate sha1; extern crate sha2; extern crate sha3; +use std::io::Write; + use hex::ToHex; +#[cfg(windows)] +use memchr::memmem; use crate::digest::digest::{ExtendableOutput, Input, XofReader}; @@ -158,3 +170,76 @@ impl_digest_sha!(sha3::Sha3_384, 384); impl_digest_sha!(sha3::Sha3_512, 512); impl_digest_shake!(sha3::Shake128); impl_digest_shake!(sha3::Shake256); + +/// A struct that writes to a digest. +/// +/// This struct wraps a [`Digest`] and provides a [`Write`] +/// implementation that passes input bytes directly to the +/// [`Digest::input`]. +/// +/// On Windows, if `binary` is `false`, then the [`write`] +/// implementation replaces instances of "\r\n" with "\n" before passing +/// the input bytes to the [`digest`]. +pub struct DigestWriter<'a> { + digest: &'a mut Box, + + /// Whether to write to the digest in binary mode or text mode on Windows. + /// + /// If this is `false`, then instances of "\r\n" are replaced with + /// "\n" before passing input bytes to the [`digest`]. + #[allow(dead_code)] + binary: bool, + // TODO This is dead code only on non-Windows operating systems. It + // might be better to use a `#[cfg(windows)]` guard here. +} + +impl<'a> DigestWriter<'a> { + pub fn new(digest: &'a mut Box, binary: bool) -> DigestWriter { + DigestWriter { digest, binary } + } +} + +impl<'a> Write for DigestWriter<'a> { + #[cfg(not(windows))] + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.digest.input(buf); + Ok(buf.len()) + } + + #[cfg(windows)] + fn write(&mut self, buf: &[u8]) -> std::io::Result { + if self.binary { + self.digest.input(buf); + return Ok(buf.len()); + } + + // In Windows text mode, replace each occurrence of "\r\n" + // with "\n". + // + // Find all occurrences of "\r\n", inputting the slice just + // before the "\n" in the previous instance of "\r\n" and + // the beginning of this "\r\n". + // + // FIXME This fails if one call to `write()` ends with the + // "\r" and the next call to `write()` begins with the "\n". + let n = buf.len(); + let mut i_prev = 0; + for i in memmem::find_iter(buf, b"\r\n") { + self.digest.input(&buf[i_prev..i]); + i_prev = i + 1; + } + self.digest.input(&buf[i_prev..n]); + + // Even though we dropped a "\r" for each "\r\n" we found, we + // still report the number of bytes written as `n`. This is + // because the meaning of the returned number is supposed to be + // the number of bytes consumed by the writer, so that if the + // calling code were calling `write()` in a loop, it would know + // where the next contiguous slice of the buffer starts. + Ok(n) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} diff --git a/src/uu/hashsum/src/hashsum.rs b/src/uu/hashsum/src/hashsum.rs index f820b083e..4186043f5 100644 --- a/src/uu/hashsum/src/hashsum.rs +++ b/src/uu/hashsum/src/hashsum.rs @@ -7,7 +7,7 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -// spell-checker:ignore (ToDO) algo, algoname, regexes, nread memmem +// spell-checker:ignore (ToDO) algo, algoname, regexes, nread #[macro_use] extern crate clap; @@ -18,11 +18,11 @@ extern crate uucore; mod digest; use self::digest::Digest; +use self::digest::DigestWriter; use clap::{App, Arg, ArgMatches}; use hex::ToHex; use md5::Context as Md5; -use memchr::memmem; use regex::Regex; use sha1::Sha1; use sha2::{Sha224, Sha256, Sha384, Sha512}; @@ -540,7 +540,7 @@ where let real_sum = crash_if_err!( 1, digest_reader( - &mut *options.digest, + &mut options.digest, &mut ckf, binary_check, options.output_bits @@ -571,7 +571,7 @@ where let sum = crash_if_err!( 1, digest_reader( - &mut *options.digest, + &mut options.digest, &mut file, options.binary, options.output_bits @@ -598,48 +598,21 @@ where Ok(()) } -fn digest_reader<'a, T: Read>( - digest: &mut (dyn Digest + 'a), +fn digest_reader( + digest: &mut Box, reader: &mut BufReader, binary: bool, output_bits: usize, ) -> io::Result { digest.reset(); - // Digest file, do not hold too much in memory at any given moment - let windows = cfg!(windows); - let mut buffer = Vec::with_capacity(524_288); - loop { - match reader.read_to_end(&mut buffer) { - Ok(0) => { - break; - } - Ok(nread) => { - if windows && !binary { - // In Windows text mode, replace each occurrence of - // "\r\n" with "\n". - // - // Find all occurrences of "\r\n", inputting the - // slice just before the "\n" in the previous - // instance of "\r\n" and the beginning of this - // "\r\n". - // - // FIXME This fails if one call to `read()` ends - // with the "\r" and the next call to `read()` - // begins with the "\n". - let mut i_prev = 0; - for i in memmem::find_iter(&buffer[0..nread], b"\r\n") { - digest.input(&buffer[i_prev..i]); - i_prev = i + 1; - } - digest.input(&buffer[i_prev..nread]); - } else { - digest.input(&buffer[..nread]); - } - } - Err(e) => return Err(e), - } - } + // Read bytes from `reader` and write those bytes to `digest`. + // + // If `binary` is `false` and the operating system is Windows, then + // `DigestWriter` replaces "\r\n" with "\n" before it writes the + // bytes into `digest`. Otherwise, it just inserts the bytes as-is. + let mut digest_writer = DigestWriter::new(digest, binary); + std::io::copy(reader, &mut digest_writer)?; if digest.output_bits() > 0 { Ok(digest.result_str()) From bfb1327ad4f8dd325e120f6f6a2914fc4035f598 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Thu, 16 Sep 2021 23:02:02 -0400 Subject: [PATCH 22/26] seq: use print_seq_integers() regardless of last Ensure that the `print_seq_integers()` function is called when the first number and the increment are integers, regardless of the type of the last value specified. --- src/uu/seq/src/seq.rs | 35 +++++++++-- tests/by-util/test_seq.rs | 120 +++++++++++++++++++++++++++++++++++++- 2 files changed, 147 insertions(+), 8 deletions(-) diff --git a/src/uu/seq/src/seq.rs b/src/uu/seq/src/seq.rs index 8bf6cb008..c4380fd3d 100644 --- a/src/uu/seq/src/seq.rs +++ b/src/uu/seq/src/seq.rs @@ -67,6 +67,18 @@ impl Number { Number::F64(n) => n, } } + + /// Convert this number into a bigint, consuming it. + /// + /// For floats, this returns the [`BigInt`] corresponding to the + /// floor of the number. + fn into_bigint(self) -> BigInt { + match self { + Number::MinusZero => BigInt::zero(), + Number::F64(x) => BigInt::from(x.floor() as i64), + Number::BigInt(n) => n, + } + } } impl FromStr for Number { @@ -197,25 +209,38 @@ pub fn uumain(args: impl uucore::Args) -> i32 { crash_if_err!(1, slice.parse()) }; + let is_negative_zero_f64 = |x: f64| x == -0.0 && x.is_sign_negative() && largest_dec == 0; let result = match (first, last, increment) { - (Number::MinusZero, Number::BigInt(last), Number::BigInt(increment)) => print_seq_integers( - (BigInt::zero(), increment, last), + // For example, `seq -0 1 2` or `seq -0 1 2.0`. + (Number::MinusZero, last, Number::BigInt(increment)) => print_seq_integers( + (BigInt::zero(), increment, last.into_bigint()), options.separator, options.terminator, options.widths, padding, true, ), - (Number::BigInt(first), Number::BigInt(last), Number::BigInt(increment)) => { + // For example, `seq -0e0 1 2` or `seq -0e0 1 2.0`. + (Number::F64(x), last, Number::BigInt(increment)) if is_negative_zero_f64(x) => { print_seq_integers( - (first, increment, last), + (BigInt::zero(), increment, last.into_bigint()), options.separator, options.terminator, options.widths, padding, - false, + true, ) } + // For example, `seq 0 1 2` or `seq 0 1 2.0`. + (Number::BigInt(first), last, Number::BigInt(increment)) => print_seq_integers( + (first, increment, last.into_bigint()), + options.separator, + options.terminator, + options.widths, + padding, + false, + ), + // For example, `seq 0 0.5 1` or `seq 0.0 0.5 1` or `seq 0.0 0.5 1.0`. (first, last, increment) => print_seq( (first.into_f64(), increment.into_f64(), last.into_f64()), largest_dec, diff --git a/tests/by-util/test_seq.rs b/tests/by-util/test_seq.rs index 7a58a950a..51262ff23 100644 --- a/tests/by-util/test_seq.rs +++ b/tests/by-util/test_seq.rs @@ -199,6 +199,16 @@ fn test_preserve_negative_zero_start() { .succeeds() .stdout_is("-0\n1\n") .no_stderr(); + new_ucmd!() + .args(&["-0", "1", "2"]) + .succeeds() + .stdout_is("-0\n1\n2\n") + .no_stderr(); + new_ucmd!() + .args(&["-0", "1", "2.0"]) + .succeeds() + .stdout_is("-0\n1\n2\n") + .no_stderr(); } #[test] @@ -226,6 +236,50 @@ fn test_width_negative_zero() { .succeeds() .stdout_is("-0\n01\n") .no_stderr(); + new_ucmd!() + .args(&["-w", "-0", "1", "2"]) + .succeeds() + .stdout_is("-0\n01\n02\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0", "1", "2.0"]) + .succeeds() + .stdout_is("-0\n01\n02\n") + .no_stderr(); +} + +#[test] +fn test_width_negative_zero_decimal_notation() { + new_ucmd!() + .args(&["-w", "-0.0", "1"]) + .succeeds() + .stdout_is("-0.0\n01.0\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.0", "1.0"]) + .succeeds() + .stdout_is("-0.0\n01.0\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.0", "1", "2"]) + .succeeds() + .stdout_is("-0.0\n01.0\n02.0\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.0", "1", "2.0"]) + .succeeds() + .stdout_is("-0.0\n01.0\n02.0\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.0", "1.0", "2"]) + .succeeds() + .stdout_is("-0.0\n01.0\n02.0\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.0", "1.0", "2.0"]) + .succeeds() + .stdout_is("-0.0\n01.0\n02.0\n") + .no_stderr(); } #[test] @@ -235,29 +289,63 @@ fn test_width_negative_zero_scientific_notation() { .succeeds() .stdout_is("-0\n01\n") .no_stderr(); + new_ucmd!() + .args(&["-w", "-0e0", "1", "2"]) + .succeeds() + .stdout_is("-0\n01\n02\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0e0", "1", "2.0"]) + .succeeds() + .stdout_is("-0\n01\n02\n") + .no_stderr(); new_ucmd!() .args(&["-w", "-0e+1", "1"]) .succeeds() .stdout_is("-00\n001\n") .no_stderr(); + new_ucmd!() + .args(&["-w", "-0e+1", "1", "2"]) + .succeeds() + .stdout_is("-00\n001\n002\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0e+1", "1", "2.0"]) + .succeeds() + .stdout_is("-00\n001\n002\n") + .no_stderr(); new_ucmd!() .args(&["-w", "-0.000e0", "1"]) .succeeds() .stdout_is("-0.000\n01.000\n") .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.000e0", "1", "2"]) + .succeeds() + .stdout_is("-0.000\n01.000\n02.000\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.000e0", "1", "2.0"]) + .succeeds() + .stdout_is("-0.000\n01.000\n02.000\n") + .no_stderr(); new_ucmd!() .args(&["-w", "-0.000e-2", "1"]) .succeeds() .stdout_is("-0.00000\n01.00000\n") .no_stderr(); - new_ucmd!() - .args(&["-w", "-0.000e5", "1"]) + .args(&["-w", "-0.000e-2", "1", "2"]) .succeeds() - .stdout_is("-000000\n0000001\n") + .stdout_is("-0.00000\n01.00000\n02.00000\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.000e-2", "1", "2.0"]) + .succeeds() + .stdout_is("-0.00000\n01.00000\n02.00000\n") .no_stderr(); new_ucmd!() @@ -265,6 +353,32 @@ fn test_width_negative_zero_scientific_notation() { .succeeds() .stdout_is("-000000\n0000001\n") .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.000e5", "1", "2"]) + .succeeds() + .stdout_is("-000000\n0000001\n0000002\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.000e5", "1", "2.0"]) + .succeeds() + .stdout_is("-000000\n0000001\n0000002\n") + .no_stderr(); + + new_ucmd!() + .args(&["-w", "-0.000e5", "1"]) + .succeeds() + .stdout_is("-000000\n0000001\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.000e5", "1", "2"]) + .succeeds() + .stdout_is("-000000\n0000001\n0000002\n") + .no_stderr(); + new_ucmd!() + .args(&["-w", "-0.000e5", "1", "2.0"]) + .succeeds() + .stdout_is("-000000\n0000001\n0000002\n") + .no_stderr(); } #[test] From 7ea2bfbe2680e65e5a6774905a8f380888ca7a25 Mon Sep 17 00:00:00 2001 From: Jan Verbeek Date: Mon, 13 Sep 2021 21:19:49 -0400 Subject: [PATCH 23/26] seq: replace loops with a single format string Replace two loops that print leading and trailing 0s when printing a number in fixed-width mode with a single call to `write!()` with the appropriate formatting parameters. --- src/uu/seq/src/seq.rs | 17 ++++++++--------- tests/by-util/test_seq.rs | 9 +++++++++ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/uu/seq/src/seq.rs b/src/uu/seq/src/seq.rs index c4380fd3d..aac8f2280 100644 --- a/src/uu/seq/src/seq.rs +++ b/src/uu/seq/src/seq.rs @@ -317,6 +317,7 @@ fn print_seq( let mut i = 0isize; let is_first_minus_zero = first == -0.0 && first.is_sign_negative(); let mut value = first + i as f64 * increment; + let padding = if pad { padding + 1 + largest_dec } else { 0 }; let mut is_first_iteration = true; while !done_printing(&value, &increment, &last) { if !is_first_iteration { @@ -328,15 +329,13 @@ fn print_seq( width -= 1; } is_first_iteration = false; - let istr = format!("{:.*}", largest_dec, value); - let ilen = istr.len(); - let before_dec = istr.find('.').unwrap_or(ilen); - if pad && before_dec < width { - for _ in 0..(width - before_dec) { - write!(stdout, "0")?; - } - } - write!(stdout, "{}", istr)?; + write!( + stdout, + "{value:>0width$.precision$}", + value = value, + width = width, + precision = largest_dec, + )?; i += 1; value = first + i as f64 * increment; } diff --git a/tests/by-util/test_seq.rs b/tests/by-util/test_seq.rs index 51262ff23..27b5f99bc 100644 --- a/tests/by-util/test_seq.rs +++ b/tests/by-util/test_seq.rs @@ -426,6 +426,15 @@ fn test_width_decimal_scientific_notation_trailing_zeros_end() { .no_stderr(); } +#[test] +fn test_width_floats() { + new_ucmd!() + .args(&["-w", "9.0", "10.0"]) + .succeeds() + .stdout_is("09.0\n10.0\n") + .no_stderr(); +} + // TODO This is duplicated from `test_yes.rs`; refactor them. /// Run `seq`, capture some of the output, close the pipe, and verify it. fn run(args: &[&str], expected: &[u8]) { 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 24/26] 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 25/26] 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 26/26] 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() {