From 2fadd253f78755b938c909ccb863b8a1e3a4f316 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Mon, 3 Feb 2025 01:23:28 +0100 Subject: [PATCH 1/3] cksum: fix --binary reset (issue #6375) --- src/uu/cksum/src/cksum.rs | 136 ++++++------------------------------ tests/by-util/test_cksum.rs | 3 +- 2 files changed, 21 insertions(+), 118 deletions(-) diff --git a/src/uu/cksum/src/cksum.rs b/src/uu/cksum/src/cksum.rs index cf95d1bd2..ff27478d6 100644 --- a/src/uu/cksum/src/cksum.rs +++ b/src/uu/cksum/src/cksum.rs @@ -205,61 +205,33 @@ mod options { pub const ZERO: &str = "zero"; } -/// Determines whether to prompt an asterisk (*) in the output. -/// -/// This function checks the `tag`, `binary`, and `had_reset` flags and returns a boolean -/// indicating whether to prompt an asterisk (*) in the output. -/// It relies on the overrides provided by clap (i.e., `--binary` overrides `--text` and vice versa). -/// Same for `--tag` and `--untagged`. -fn prompt_asterisk(tag: bool, binary: bool, had_reset: bool) -> bool { - if tag { - return false; - } - if had_reset { - return false; - } - binary -} - -/** - * Determine if we had a reset. - * This is basically a hack to support the behavior of cksum - * when we have the following arguments: - * --binary --tag --untagged - * Don't do it with clap because if it struggling with the --overrides_with - * marking the value as set even if not present - */ -fn had_reset(args: &[OsString]) -> bool { - // Indices where "--binary" or "-b", "--tag", and "--untagged" are found - let binary_index = args.iter().position(|x| x == "--binary" || x == "-b"); - let tag_index = args.iter().position(|x| x == "--tag"); - let untagged_index = args.iter().position(|x| x == "--untagged"); - - // Check if all arguments are present and in the correct order - match (binary_index, tag_index, untagged_index) { - (Some(b), Some(t), Some(u)) => b < t && t < u, - _ => false, - } -} - /*** * cksum has a bunch of legacy behavior. * We handle this in this function to make sure they are self contained * and "easier" to understand */ -fn handle_tag_text_binary_flags(matches: &clap::ArgMatches) -> UResult<(bool, bool)> { - let untagged = matches.get_flag(options::UNTAGGED); - let tag = matches.get_flag(options::TAG); - let tag = tag || !untagged; +fn handle_tag_text_binary_flags>( + args: impl Iterator, +) -> UResult<(bool, bool)> { + let mut tag = true; + let mut binary = false; - let binary_flag = matches.get_flag(options::BINARY); + // --binary, --tag and --untagged are tight together: none of them + // conflicts with each other but --tag will reset "binary" and set "tag". - let args: Vec = std::env::args_os().collect(); - let had_reset = had_reset(&args); + for arg in args { + let arg = arg.as_ref(); + if arg == "-b" || arg == "--binary" { + binary = true; + } else if arg == "--tag" { + tag = true; + binary = false; + } else if arg == "--untagged" { + tag = false; + } + } - let asterisk = prompt_asterisk(tag, binary_flag, had_reset); - - Ok((tag, asterisk)) + Ok((tag, !tag && binary)) } #[uucore::main] @@ -336,7 +308,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { return perform_checksum_validation(files.iter().copied(), algo_option, length, opts); } - let (tag, asterisk) = handle_tag_text_binary_flags(&matches)?; + let (tag, asterisk) = handle_tag_text_binary_flags(std::env::args_os())?; let algo = detect_algo(algo_name, length)?; let line_ending = LineEnding::from_zero_flag(matches.get_flag(options::ZERO)); @@ -501,75 +473,7 @@ pub fn uu_app() -> Command { #[cfg(test)] mod tests { - use super::had_reset; use crate::calculate_blake2b_length; - use crate::prompt_asterisk; - use std::ffi::OsString; - - #[test] - fn test_had_reset() { - let args = ["--binary", "--tag", "--untagged"] - .iter() - .map(|&s| s.into()) - .collect::>(); - assert!(had_reset(&args)); - - let args = ["-b", "--tag", "--untagged"] - .iter() - .map(|&s| s.into()) - .collect::>(); - assert!(had_reset(&args)); - - let args = ["-b", "--binary", "--tag", "--untagged"] - .iter() - .map(|&s| s.into()) - .collect::>(); - assert!(had_reset(&args)); - - let args = ["--untagged", "--tag", "--binary"] - .iter() - .map(|&s| s.into()) - .collect::>(); - assert!(!had_reset(&args)); - - let args = ["--untagged", "--tag", "-b"] - .iter() - .map(|&s| s.into()) - .collect::>(); - assert!(!had_reset(&args)); - - let args = ["--binary", "--tag"] - .iter() - .map(|&s| s.into()) - .collect::>(); - assert!(!had_reset(&args)); - - let args = ["--tag", "--untagged"] - .iter() - .map(|&s| s.into()) - .collect::>(); - assert!(!had_reset(&args)); - - let args = ["--text", "--untagged"] - .iter() - .map(|&s| s.into()) - .collect::>(); - assert!(!had_reset(&args)); - - let args = ["--binary", "--untagged"] - .iter() - .map(|&s| s.into()) - .collect::>(); - assert!(!had_reset(&args)); - } - - #[test] - fn test_prompt_asterisk() { - assert!(!prompt_asterisk(true, false, false)); - assert!(!prompt_asterisk(false, false, true)); - assert!(prompt_asterisk(false, true, false)); - assert!(!prompt_asterisk(false, false, false)); - } #[test] fn test_calculate_length() { diff --git a/tests/by-util/test_cksum.rs b/tests/by-util/test_cksum.rs index d2e8ac4c6..232793d26 100644 --- a/tests/by-util/test_cksum.rs +++ b/tests/by-util/test_cksum.rs @@ -602,7 +602,6 @@ fn test_reset_binary() { .stdout_contains("d41d8cd98f00b204e9800998ecf8427e "); } -#[ignore = "issue #6375"] #[test] fn test_reset_binary_but_set() { let scene = TestScenario::new(util_name!()); @@ -619,7 +618,7 @@ fn test_reset_binary_but_set() { .arg("--algorithm=md5") .arg(at.subdir.join("f")) .succeeds() - .stdout_contains("d41d8cd98f00b204e9800998ecf8427e *"); // currently, asterisk=false. It should be true + .stdout_contains("d41d8cd98f00b204e9800998ecf8427e *"); } #[test] From b8abebfaf967a82304b3a97ec511e778b4a50633 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Mon, 3 Feb 2025 01:33:27 +0100 Subject: [PATCH 2/3] test(cksum): un-ignore now passing test `test_blake2b_tested_with_sha1` --- tests/by-util/test_cksum.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/by-util/test_cksum.rs b/tests/by-util/test_cksum.rs index 232793d26..6120e1a18 100644 --- a/tests/by-util/test_cksum.rs +++ b/tests/by-util/test_cksum.rs @@ -1187,7 +1187,6 @@ fn test_bsd_case() { .stderr_contains("f: no properly formatted checksum lines found"); } -#[ignore = "Different output"] #[test] fn test_blake2d_tested_with_sha1() { let (at, mut ucmd) = at_and_ucmd!(); From f2cf08b4e6d116910a80ed608853283fcbcb697a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dorian=20P=C3=A9ron?= Date: Mon, 3 Feb 2025 02:09:45 +0100 Subject: [PATCH 3/3] test(cksum): fix and un-ignore `test_md5_bits` --- src/uucore/src/lib/features/checksum.rs | 24 +++++++++++++++--------- tests/by-util/test_cksum.rs | 1 - 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/uucore/src/lib/features/checksum.rs b/src/uucore/src/lib/features/checksum.rs index 9912bea74..8346f9c6d 100644 --- a/src/uucore/src/lib/features/checksum.rs +++ b/src/uucore/src/lib/features/checksum.rs @@ -692,7 +692,7 @@ fn identify_algo_name_and_length( line_info: &LineInfo, algo_name_input: Option<&str>, last_algo: &mut Option, -) -> Option<(String, Option)> { +) -> Result<(String, Option), LineCheckError> { let algo_from_line = line_info.algo_name.clone().unwrap_or_default(); let algorithm = algo_from_line.to_lowercase(); *last_algo = Some(algo_from_line); @@ -701,18 +701,25 @@ fn identify_algo_name_and_length( // (for example SHA1 (f) = d...) // Also handle the case cksum -s sm3 but the file contains other formats if algo_name_input.is_some() && algo_name_input != Some(&algorithm) { - return None; + return Err(LineCheckError::ImproperlyFormatted); } if !SUPPORTED_ALGORITHMS.contains(&algorithm.as_str()) { // Not supported algo, leave early - return None; + return Err(LineCheckError::ImproperlyFormatted); } let bytes = if let Some(bitlen) = line_info.algo_bit_len { - if bitlen % 8 != 0 { - // The given length is wrong - return None; + if algorithm != ALGORITHM_OPTIONS_BLAKE2B || bitlen % 8 != 0 { + // Either + // the algo based line is provided with a bit length + // with an algorithm that does not support it (only Blake2B does). + // + // eg: MD5-128 (foo.txt) = fffffffff + // ^ This is illegal + // OR + // the given length is wrong because it's not a multiple of 8. + return Err(LineCheckError::ImproperlyFormatted); } Some(bitlen / 8) } else if algorithm == ALGORITHM_OPTIONS_BLAKE2B { @@ -722,7 +729,7 @@ fn identify_algo_name_and_length( None }; - Some((algorithm, bytes)) + Ok((algorithm, bytes)) } /// Given a filename and an algorithm, compute the digest and compare it with @@ -773,8 +780,7 @@ fn process_algo_based_line( let filename_to_check = line_info.filename.as_slice(); let (algo_name, algo_byte_len) = - identify_algo_name_and_length(line_info, cli_algo_name, last_algo) - .ok_or(LineCheckError::ImproperlyFormatted)?; + identify_algo_name_and_length(line_info, cli_algo_name, last_algo)?; // If the digest bitlen is known, we can check the format of the expected // checksum with it. diff --git a/tests/by-util/test_cksum.rs b/tests/by-util/test_cksum.rs index 6120e1a18..9f2869b54 100644 --- a/tests/by-util/test_cksum.rs +++ b/tests/by-util/test_cksum.rs @@ -1136,7 +1136,6 @@ fn test_cksum_garbage() { .stderr_contains("check-file: no properly formatted checksum lines found"); } -#[ignore = "Should fail on bits"] #[test] fn test_md5_bits() { let (at, mut ucmd) = at_and_ucmd!();