From 9b29ac98a59c5b8bc82a4a15177ad75053fbb067 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Tue, 1 Jun 2021 18:12:37 +0200 Subject: [PATCH 1/4] seq: reject NaN arguments Move the validation logic to an argument validator. --- src/uu/seq/src/seq.rs | 68 ++++++++++++++------------------------- tests/by-util/test_seq.rs | 16 +++++++++ 2 files changed, 40 insertions(+), 44 deletions(-) diff --git a/src/uu/seq/src/seq.rs b/src/uu/seq/src/seq.rs index bdab044c5..60aa7e654 100644 --- a/src/uu/seq/src/seq.rs +++ b/src/uu/seq/src/seq.rs @@ -13,7 +13,6 @@ use num_traits::Zero; use num_traits::{Num, ToPrimitive}; use std::cmp; use std::io::{stdout, Write}; -use std::str::FromStr; static VERSION: &str = env!("CARGO_PKG_VERSION"); static ABOUT: &str = "Display numbers from FIRST to LAST, in steps of INCREMENT."; @@ -44,6 +43,18 @@ enum Number { } impl Number { + fn new(mut s: &str) -> Self { + if s.starts_with('+') { + s = &s[1..]; + } + + match s.parse::() { + Ok(n) => Number::BigInt(n), + // The argument validator made sure this is a valid float. + Err(_) => Number::F64(s.parse::().unwrap()), + } + } + fn is_zero(&self) -> bool { match self { Number::BigInt(n) => n.is_zero(), @@ -60,27 +71,6 @@ impl Number { } } -impl FromStr for Number { - type Err = String; - /// Tries to parse this string as a BigInt, or if that fails as an f64. - fn from_str(mut s: &str) -> Result { - if s.starts_with('+') { - s = &s[1..]; - } - - match s.parse::() { - Ok(n) => Ok(Number::BigInt(n)), - Err(_) => match s.parse::() { - Ok(n) => Ok(Number::F64(n)), - Err(e) => Err(format!( - "seq: invalid floating point argument `{}`: {}", - s, e - )), - }, - } - } -} - pub fn uumain(args: impl uucore::Args) -> i32 { let usage = get_usage(); let matches = App::new(executable!()) @@ -116,7 +106,15 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .takes_value(true) .allow_hyphen_values(true) .max_values(3) - .required(true), + .required(true) + .validator(|value| { + match value.parse::() { + Ok(value) if value.is_nan() => Err("can not be NaN".to_string()), + Ok(_) => Ok(()), + Err(e) => Err(e.to_string()), + } + .map_err(|e| format!("invalid floating point argument `{}`: {}", value, e)) + }), ) .get_matches_from(args); @@ -136,13 +134,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let dec = slice.find('.').unwrap_or(len); largest_dec = len - dec; padding = dec; - match slice.parse() { - Ok(n) => n, - Err(s) => { - show_error!("{}", s); - return 1; - } - } + Number::new(slice) } else { Number::BigInt(BigInt::one()) }; @@ -152,13 +144,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let dec = slice.find('.').unwrap_or(len); largest_dec = cmp::max(largest_dec, len - dec); padding = cmp::max(padding, dec); - match slice.parse() { - Ok(n) => n, - Err(s) => { - show_error!("{}", s); - return 1; - } - } + Number::new(slice) } else { Number::BigInt(BigInt::one()) }; @@ -169,13 +155,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let last = { let slice = numbers[numbers.len() - 1]; padding = cmp::max(padding, slice.find('.').unwrap_or_else(|| slice.len())); - match slice.parse::() { - Ok(n) => n, - Err(s) => { - show_error!("{}", s); - return 1; - } - } + Number::new(slice) }; if largest_dec > 0 { largest_dec -= 1; diff --git a/tests/by-util/test_seq.rs b/tests/by-util/test_seq.rs index 98eb23598..7545ebde7 100644 --- a/tests/by-util/test_seq.rs +++ b/tests/by-util/test_seq.rs @@ -1,5 +1,21 @@ use crate::common::util::*; +#[test] +fn test_rejects_nan() { + new_ucmd!() + .args(&["NaN"]) + .fails() + .stderr_only("error: Invalid value for '...': invalid floating point argument `NaN`: can not be NaN"); +} + +#[test] +fn test_rejects_non_floats() { + new_ucmd!() + .args(&["foo"]) + .fails() + .stderr_only("error: Invalid value for '...': invalid floating point argument `foo`: invalid float literal"); +} + // ---- Tests for the big integer based path ---- #[test] From 5329d77cc2f928a4b59a6e1563761217789d47e8 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Tue, 1 Jun 2021 20:35:18 +0200 Subject: [PATCH 2/4] seq: adapt output to GNU seq --- src/uu/seq/src/seq.rs | 61 +++++++++++++++++++++++---------------- tests/by-util/test_seq.rs | 4 +-- 2 files changed, 38 insertions(+), 27 deletions(-) diff --git a/src/uu/seq/src/seq.rs b/src/uu/seq/src/seq.rs index 60aa7e654..8cf6513cb 100644 --- a/src/uu/seq/src/seq.rs +++ b/src/uu/seq/src/seq.rs @@ -13,6 +13,7 @@ use num_traits::Zero; use num_traits::{Num, ToPrimitive}; use std::cmp; use std::io::{stdout, Write}; +use std::str::FromStr; static VERSION: &str = env!("CARGO_PKG_VERSION"); static ABOUT: &str = "Display numbers from FIRST to LAST, in steps of INCREMENT."; @@ -43,18 +44,6 @@ enum Number { } impl Number { - fn new(mut s: &str) -> Self { - if s.starts_with('+') { - s = &s[1..]; - } - - match s.parse::() { - Ok(n) => Number::BigInt(n), - // The argument validator made sure this is a valid float. - Err(_) => Number::F64(s.parse::().unwrap()), - } - } - fn is_zero(&self) -> bool { match self { Number::BigInt(n) => n.is_zero(), @@ -71,6 +60,32 @@ impl Number { } } +impl FromStr for Number { + type Err = String; + fn from_str(mut s: &str) -> Result { + if s.starts_with('+') { + s = &s[1..]; + } + + match s.parse::() { + Ok(n) => Ok(Number::BigInt(n)), + Err(_) => match s.parse::() { + Ok(value) if value.is_nan() => Err(format!( + "invalid 'not-a-number' argument: '{}'\nTry '{} --help' for more information.", + s, + executable!(), + )), + Ok(value) => Ok(Number::F64(value)), + Err(_) => Err(format!( + "invalid floating point argument: '{}'\nTry '{} --help' for more information.", + s, + executable!(), + )), + }, + } + } +} + pub fn uumain(args: impl uucore::Args) -> i32 { let usage = get_usage(); let matches = App::new(executable!()) @@ -106,15 +121,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .takes_value(true) .allow_hyphen_values(true) .max_values(3) - .required(true) - .validator(|value| { - match value.parse::() { - Ok(value) if value.is_nan() => Err("can not be NaN".to_string()), - Ok(_) => Ok(()), - Err(e) => Err(e.to_string()), - } - .map_err(|e| format!("invalid floating point argument `{}`: {}", value, e)) - }), + .required(true), ) .get_matches_from(args); @@ -134,7 +141,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let dec = slice.find('.').unwrap_or(len); largest_dec = len - dec; padding = dec; - Number::new(slice) + return_if_err!(1, slice.parse()) } else { Number::BigInt(BigInt::one()) }; @@ -144,18 +151,22 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let dec = slice.find('.').unwrap_or(len); largest_dec = cmp::max(largest_dec, len - dec); padding = cmp::max(padding, dec); - Number::new(slice) + return_if_err!(1, slice.parse()) } else { Number::BigInt(BigInt::one()) }; if increment.is_zero() { - show_error!("increment value: '{}'", numbers[1]); + show_error!( + "invalid Zero increment value: '{}'\nTry '{} --help' for more information.", + numbers[1], + executable!() + ); return 1; } let last = { let slice = numbers[numbers.len() - 1]; padding = cmp::max(padding, slice.find('.').unwrap_or_else(|| slice.len())); - Number::new(slice) + return_if_err!(1, slice.parse()) }; if largest_dec > 0 { largest_dec -= 1; diff --git a/tests/by-util/test_seq.rs b/tests/by-util/test_seq.rs index 7545ebde7..459eeeb3a 100644 --- a/tests/by-util/test_seq.rs +++ b/tests/by-util/test_seq.rs @@ -5,7 +5,7 @@ fn test_rejects_nan() { new_ucmd!() .args(&["NaN"]) .fails() - .stderr_only("error: Invalid value for '...': invalid floating point argument `NaN`: can not be NaN"); + .stderr_only("seq: invalid 'not-a-number' argument: 'NaN'\nTry 'seq --help' for more information."); } #[test] @@ -13,7 +13,7 @@ fn test_rejects_non_floats() { new_ucmd!() .args(&["foo"]) .fails() - .stderr_only("error: Invalid value for '...': invalid floating point argument `foo`: invalid float literal"); + .stderr_only("seq: invalid floating point argument: 'foo'\nTry 'seq --help' for more information."); } // ---- Tests for the big integer based path ---- From a323e9cda17c51849bd62d75e47303fb2a47a3d8 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Tue, 1 Jun 2021 23:06:38 +0200 Subject: [PATCH 3/4] cp: show errors in cow on linux --- src/uu/cp/src/cp.rs | 6 ++++-- tests/by-util/test_cp.rs | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index b7c64928f..40266689c 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1261,13 +1261,15 @@ fn copy_helper(source: &Path, dest: &Path, options: &Options) -> CopyResult<()> fn copy_on_write_linux(source: &Path, dest: &Path, mode: ReflinkMode) -> CopyResult<()> { debug_assert!(mode != ReflinkMode::Never); - let src_file = File::open(source).unwrap().into_raw_fd(); + let src_file = File::open(source) + .context(&*context_for(source, dest))? + .into_raw_fd(); let dst_file = OpenOptions::new() .write(true) .truncate(false) .create(true) .open(dest) - .unwrap() + .context(&*context_for(source, dest))? .into_raw_fd(); match mode { ReflinkMode::Always => unsafe { diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index e995cc56c..c56e1ca57 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -7,6 +7,8 @@ use std::fs::set_permissions; #[cfg(not(windows))] use std::os::unix::fs; +#[cfg(target_os = "linux")] +use std::os::unix::fs::PermissionsExt; #[cfg(windows)] use std::os::windows::fs::symlink_file; @@ -1257,3 +1259,20 @@ fn test_cp_reflink_bad() { .fails() .stderr_contains("invalid argument"); } + +#[test] +#[cfg(target_os = "linux")] +fn test_cp_reflink_insufficient_permission() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.make_file("unreadable") + .set_permissions(PermissionsExt::from_mode(0o000)) + .unwrap(); + + ucmd.arg("-r") + .arg("--reflink=auto") + .arg("unreadable") + .arg(TEST_EXISTING_FILE) + .fails() + .stderr_only("cp: 'unreadable' -> 'existing_file.txt': Permission denied (os error 13)"); +} From fc2b61eb9623c855817e87ffef0dd672885f89a0 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Tue, 1 Jun 2021 23:06:51 +0200 Subject: [PATCH 4/4] tests: typo --- tests/common/macros.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/common/macros.rs b/tests/common/macros.rs index 81878bf1b..03d2051d0 100644 --- a/tests/common/macros.rs +++ b/tests/common/macros.rs @@ -50,14 +50,14 @@ macro_rules! new_ucmd { /// Convenience macro for acquiring a [`UCommand`] builder and a test path. /// /// Returns a tuple containing the following: -/// - an [`AsPath`] that points to a unique temporary test directory +/// - an [`AtPath`] that points to a unique temporary test directory /// - a [`UCommand`] builder for invoking the binary to be tested /// /// This macro is intended for quick, single-call tests. For more complex tests /// that require multiple invocations of the tested binary, see [`TestScenario`] /// /// [`UCommand`]: crate::tests::common::util::UCommand -/// [`AsPath`]: crate::tests::common::util::AsPath +/// [`AtPath`]: crate::tests::common::util::AtPath /// [`TestScenario]: crate::tests::common::util::TestScenario #[macro_export] macro_rules! at_and_ucmd {