Merge pull request #9063 from RenjiSann/cksum-fix-tests

cksum: Fix GNU `cksum-c.sh` and `cksum-sha3.sh`

src/uu/cksum/src/cksum.rs

@@ -6,7 +6,7 @@
 // spell-checker:ignore (ToDO) fname, algo
 
 use clap::builder::ValueParser;
-use clap::{Arg, ArgAction, Command, value_parser};
+use clap::{Arg, ArgAction, Command};
 use std::ffi::{OsStr, OsString};
 use std::fs::File;
 use std::io::{BufReader, Read, Write, stdin, stdout};
@@ -16,8 +16,8 @@ use uucore::checksum::{
     ALGORITHM_OPTIONS_BLAKE2B, ALGORITHM_OPTIONS_BSD, ALGORITHM_OPTIONS_CRC,
     ALGORITHM_OPTIONS_CRC32B, ALGORITHM_OPTIONS_SHA2, ALGORITHM_OPTIONS_SHA3,
     ALGORITHM_OPTIONS_SYSV, ChecksumError, ChecksumOptions, ChecksumVerbose, HashAlgorithm,
-    LEGACY_ALGORITHMS, SUPPORTED_ALGORITHMS, calculate_blake2b_length, detect_algo, digest_reader,
-    perform_checksum_validation,
+    LEGACY_ALGORITHMS, SUPPORTED_ALGORITHMS, calculate_blake2b_length_str, detect_algo,
+    digest_reader, perform_checksum_validation, sanitize_sha2_sha3_length_str,
 };
 use uucore::translate;
 
@@ -186,13 +186,14 @@ fn cksum<'a, I>(mut options: Options, files: I) -> UResult<()>
 where
     I: Iterator<Item = &'a OsStr>,
 {
-    let files: Vec<_> = files.collect();
+    let mut files = files.peekable();
 
-    if options.output_format.is_raw() && files.len() > 1 {
-        return Err(Box::new(ChecksumError::RawMultipleFiles));
-    }
+    while let Some(filename) = files.next() {
+        // Check that in raw mode, we are not provided with several files.
+        if options.output_format.is_raw() && files.peek().is_some() {
+            return Err(Box::new(ChecksumError::RawMultipleFiles));
+        }
 
-    for filename in files {
         let filepath = Path::new(filename);
         let stdin_buf;
         let file_buf;
@@ -363,42 +364,45 @@ fn figure_out_output_format(
     }
 }
 
+/// Sanitize the `--length` argument depending on `--algorithm` and `--length`.
+fn maybe_sanitize_length(
+    algo_cli: Option<&str>,
+    input_length: Option<&str>,
+) -> UResult<Option<usize>> {
+    match (algo_cli, input_length) {
+        // No provided length is not a problem so far.
+        (_, None) => Ok(None),
+
+        // For SHA2 and SHA3, if a length is provided, ensure it is correct.
+        (Some(algo @ (ALGORITHM_OPTIONS_SHA2 | ALGORITHM_OPTIONS_SHA3)), Some(s_len)) => {
+            sanitize_sha2_sha3_length_str(algo, s_len).map(Some)
+        }
+
+        // For BLAKE2b, if a length is provided, validate it.
+        (Some(ALGORITHM_OPTIONS_BLAKE2B), Some(len)) => calculate_blake2b_length_str(len),
+
+        // For any other provided algorithm, check if length is 0.
+        // Otherwise, this is an error.
+        (_, Some(len)) if len.parse::<u32>() == Ok(0_u32) => Ok(None),
+        (_, Some(_)) => Err(ChecksumError::LengthOnlyForBlake2bSha2Sha3.into()),
+    }
+}
+
 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let matches = uucore::clap_localization::handle_clap_result(uu_app(), args)?;
 
     let check = matches.get_flag(options::CHECK);
 
-    let algo_name: &str = match matches.get_one::<String>(options::ALGORITHM) {
-        Some(v) => v,
-        None => {
-            if check {
-                // if we are doing a --check, we should not default to crc
-                ""
-            } else {
-                ALGORITHM_OPTIONS_CRC
-            }
-        }
-    };
+    let algo_cli = matches
+        .get_one::<String>(options::ALGORITHM)
+        .map(String::as_str);
 
-    let input_length = matches.get_one::<usize>(options::LENGTH);
+    let input_length = matches
+        .get_one::<String>(options::LENGTH)
+        .map(String::as_str);
 
-    let length = match (input_length, algo_name) {
-        // Length for sha2 and sha3 should be saved, it will be validated
-        // afterwards if necessary.
-        (Some(len), ALGORITHM_OPTIONS_SHA2 | ALGORITHM_OPTIONS_SHA3) => Some(*len),
-        (None | Some(0), _) => None,
-        // Length for Blake2b if saved only if it's not zero.
-        (Some(len), ALGORITHM_OPTIONS_BLAKE2B) => calculate_blake2b_length(*len)?,
-        // a --length flag set with any other algorithm is an error.
-        _ => {
-            return Err(ChecksumError::LengthOnlyForBlake2bSha2Sha3.into());
-        }
-    };
-
-    if LEGACY_ALGORITHMS.contains(&algo_name) && check {
-        return Err(ChecksumError::AlgorithmNotSupportedWithCheck.into());
-    }
+    let length = maybe_sanitize_length(algo_cli, input_length)?;
 
     let files = matches.get_many::<OsString>(options::FILE).map_or_else(
         // No files given, read from stdin.
@@ -408,6 +412,11 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     );
 
     if check {
+        // cksum does not support '--check'ing legacy algorithms
+        if algo_cli.is_some_and(|algo_name| LEGACY_ALGORITHMS.contains(&algo_name)) {
+            return Err(ChecksumError::AlgorithmNotSupportedWithCheck.into());
+        }
+
         let text_flag = matches.get_flag(options::TEXT);
         let binary_flag = matches.get_flag(options::BINARY);
         let strict = matches.get_flag(options::STRICT);
@@ -421,13 +430,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
             return Err(ChecksumError::BinaryTextConflict.into());
         }
 
-        // Determine the appropriate algorithm option to pass
-        let algo_option = if algo_name.is_empty() {
-            None
-        } else {
-            Some(algo_name)
-        };
-
         // Execute the checksum validation based on the presence of files or the use of stdin
 
         let verbose = ChecksumVerbose::new(status, quiet, warn);
@@ -438,9 +440,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
             verbose,
         };
 
-        return perform_checksum_validation(files, algo_option, length, opts);
+        return perform_checksum_validation(files, algo_cli, length, opts);
     }
 
+    // Not --check
+
+    // Set the default algorithm to CRC when not '--check'ing.
+    let algo_name = algo_cli.unwrap_or(ALGORITHM_OPTIONS_CRC);
+
     let (tag, binary) = handle_tag_text_binary_flags(std::env::args_os())?;
 
     let algo = detect_algo(algo_name, length)?;
@@ -508,7 +515,6 @@ pub fn uu_app() -> Command {
         .arg(
             Arg::new(options::LENGTH)
                 .long(options::LENGTH)
-                .value_parser(value_parser!(usize))
                 .short('l')
                 .help(translate!("cksum-help-length"))
                 .action(ArgAction::Set),

src/uucore/src/lib/features/checksum.rs

@@ -12,6 +12,7 @@ use std::{
     fmt::Display,
     fs::File,
     io::{self, BufReader, Read, Write, stdin},
+    num::IntErrorKind,
     path::Path,
     str,
 };
@@ -220,12 +221,12 @@ pub enum ChecksumError {
     QuietNotCheck,
     #[error("--length required for {}", .0.quote())]
     LengthRequired(String),
-    #[error("unknown algorithm: {0}: clap should have prevented this case")]
-    UnknownAlgorithm(String),
-    #[error("length is not a multiple of 8")]
-    InvalidLength,
+    #[error("invalid length: {}", .0.quote())]
+    InvalidLength(String),
     #[error("digest length for {} must be 224, 256, 384, or 512", .0.quote())]
-    InvalidLengthFor(String),
+    InvalidLengthForSha(String),
+    #[error("--algorithm={0} requires specifying --length 224, 256, 384, or 512")]
+    LengthRequiredForSha(String),
     #[error("--length is only supported with --algorithm blake2b, sha2, or sha3")]
     LengthOnlyForBlake2bSha2Sha3,
     #[error("the --binary and --text options are meaningless when verifying checksums")]
@@ -238,6 +239,8 @@ pub enum ChecksumError {
     CombineMultipleAlgorithms,
     #[error("Needs an algorithm to hash with.\nUse --help for more information.")]
     NeedAlgorithmToHash,
+    #[error("unknown algorithm: {0}: clap should have prevented this case")]
+    UnknownAlgorithm(String),
     #[error("")]
     Io(#[from] io::Error),
 }
@@ -277,7 +280,7 @@ pub fn create_sha3(bits: usize) -> UResult<HashAlgorithm> {
             bits: 512,
         }),
 
-        _ => Err(ChecksumError::InvalidLengthFor("SHA3".into()).into()),
+        _ => Err(ChecksumError::InvalidLengthForSha("SHA3".into()).into()),
     }
 }
 
@@ -304,7 +307,7 @@ pub fn create_sha2(bits: usize) -> UResult<HashAlgorithm> {
             bits: 512,
         }),
 
-        _ => Err(ChecksumError::InvalidLengthFor("SHA2".into()).into()),
+        _ => Err(ChecksumError::InvalidLengthForSha("SHA2".into()).into()),
     }
 }
 
@@ -837,25 +840,41 @@ fn identify_algo_name_and_length(
     last_algo: &mut Option<String>,
 ) -> Result<(String, Option<usize>), LineCheckError> {
     let algo_from_line = line_info.algo_name.clone().unwrap_or_default();
-    let algorithm = algo_from_line.to_lowercase();
+    let line_algo = algo_from_line.to_lowercase();
     *last_algo = Some(algo_from_line);
 
-    // check if we are called with XXXsum (example: md5sum) but we detected a different algo parsing the file
-    // (for example SHA1 (f) = d...)
+    // check if we are called with XXXsum (example: md5sum) but we detected a
+    // different algo parsing the file (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 Err(LineCheckError::ImproperlyFormatted);
+    if let Some(algo_name_input) = algo_name_input {
+        match (algo_name_input, line_algo.as_str()) {
+            (l, r) if l == r => (),
+            // Edge case for SHA2, which matches SHA(224|256|384|512)
+            (
+                ALGORITHM_OPTIONS_SHA2,
+                ALGORITHM_OPTIONS_SHA224
+                | ALGORITHM_OPTIONS_SHA256
+                | ALGORITHM_OPTIONS_SHA384
+                | ALGORITHM_OPTIONS_SHA512,
+            ) => (),
+            _ => return Err(LineCheckError::ImproperlyFormatted),
+        }
     }
 
-    if !SUPPORTED_ALGORITHMS.contains(&algorithm.as_str()) {
+    if !SUPPORTED_ALGORITHMS.contains(&line_algo.as_str()) {
         // Not supported algo, leave early
         return Err(LineCheckError::ImproperlyFormatted);
     }
 
     let bytes = if let Some(bitlen) = line_info.algo_bit_len {
-        match algorithm.as_str() {
+        match line_algo.as_str() {
             ALGORITHM_OPTIONS_BLAKE2B if bitlen % 8 == 0 => Some(bitlen / 8),
-            ALGORITHM_OPTIONS_SHA3 if [224, 256, 384, 512].contains(&bitlen) => Some(bitlen),
+            ALGORITHM_OPTIONS_SHA2 | ALGORITHM_OPTIONS_SHA3
+                if [224, 256, 384, 512].contains(&bitlen) =>
+            {
+                Some(bitlen)
+            }
             // Either
             //  the algo based line is provided with a bit length
             //  with an algorithm that does not support it (only Blake2B does).
@@ -866,14 +885,14 @@ fn identify_algo_name_and_length(
             //  the given length is wrong because it's not a multiple of 8.
             _ => return Err(LineCheckError::ImproperlyFormatted),
         }
-    } else if algorithm == ALGORITHM_OPTIONS_BLAKE2B {
+    } else if line_algo == ALGORITHM_OPTIONS_BLAKE2B {
         // Default length with BLAKE2b,
         Some(64)
     } else {
         None
     };
 
-    Ok((algorithm, bytes))
+    Ok((line_algo, bytes))
 }
 
 /// Given a filename and an algorithm, compute the digest and compare it with
@@ -1219,21 +1238,29 @@ pub fn digest_reader<T: Read>(
 
 /// Calculates the length of the digest.
 pub fn calculate_blake2b_length(length: usize) -> UResult<Option<usize>> {
-    match length {
-        0 => Ok(None),
-        n if n % 8 != 0 => {
-            show_error!("invalid length: \u{2018}{length}\u{2019}");
+    calculate_blake2b_length_str(length.to_string().as_str())
+}
+
+/// Calculates the length of the digest.
+pub fn calculate_blake2b_length_str(length: &str) -> UResult<Option<usize>> {
+    match length.parse() {
+        Ok(0) => Ok(None),
+        Ok(n) if n % 8 != 0 => {
+            show_error!("{}", ChecksumError::InvalidLength(length.into()));
             Err(io::Error::new(io::ErrorKind::InvalidInput, "length is not a multiple of 8").into())
         }
-        n if n > 512 => {
-            show_error!("invalid length: \u{2018}{length}\u{2019}");
+        Ok(n) if n > 512 => {
+            show_error!("{}", ChecksumError::InvalidLength(length.into()));
             Err(io::Error::new(
                 io::ErrorKind::InvalidInput,
-                "maximum digest length for \u{2018}BLAKE2b\u{2019} is 512 bits",
+                format!(
+                    "maximum digest length for {} is 512 bits",
+                    "BLAKE2b".quote()
+                ),
             )
             .into())
         }
-        n => {
+        Ok(n) => {
             // Divide by 8, as our blake2b implementation expects bytes instead of bits.
             if n == 512 {
                 // When length is 512, it is blake2b's default.
@@ -1243,6 +1270,7 @@ pub fn calculate_blake2b_length(length: usize) -> UResult<Option<usize>> {
                 Ok(Some(n / 8))
             }
         }
+        Err(_) => Err(ChecksumError::InvalidLength(length.into()).into()),
     }
 }
 
@@ -1250,10 +1278,35 @@ pub fn validate_sha2_sha3_length(algo_name: &str, length: Option<usize>) -> URes
     match length {
         Some(len @ (224 | 256 | 384 | 512)) => Ok(len),
         Some(len) => {
-            show_error!("invalid length: '{len}'");
-            Err(ChecksumError::InvalidLengthFor(algo_name.to_ascii_uppercase()).into())
+            show_error!("{}", ChecksumError::InvalidLength(len.to_string()));
+            Err(ChecksumError::InvalidLengthForSha(algo_name.to_ascii_uppercase()).into())
         }
-        None => Err(ChecksumError::LengthRequired(algo_name.to_ascii_uppercase()).into()),
+        None => Err(ChecksumError::LengthRequiredForSha(algo_name.into()).into()),
+    }
+}
+
+pub fn sanitize_sha2_sha3_length_str(algo_name: &str, length: &str) -> UResult<usize> {
+    // There is a difference in the errors sent when the length is not a number
+    // vs. its an invalid number.
+    //
+    // When inputting an invalid number, an extra error message it printed to
+    // remind of the accepted inputs.
+    let len = match length.parse::<usize>() {
+        Ok(l) => l,
+        // Note: Positive overflow while parsing counts as an invalid number,
+        // but a number still.
+        Err(e) if *e.kind() == IntErrorKind::PosOverflow => {
+            show_error!("{}", ChecksumError::InvalidLength(length.into()));
+            return Err(ChecksumError::InvalidLengthForSha(algo_name.to_ascii_uppercase()).into());
+        }
+        Err(_) => return Err(ChecksumError::InvalidLength(length.into()).into()),
+    };
+
+    if [224, 256, 384, 512].contains(&len) {
+        Ok(len)
+    } else {
+        show_error!("{}", ChecksumError::InvalidLength(length.into()));
+        Err(ChecksumError::InvalidLengthForSha(algo_name.to_ascii_uppercase()).into())
     }
 }
 

tests/by-util/test_cksum.rs

@@ -7,6 +7,7 @@
 use uutests::at_and_ucmd;
 use uutests::new_ucmd;
 use uutests::util::TestScenario;
+use uutests::util::log_info;
 use uutests::util_name;
 
 const ALGOS: [&str; 11] = [
@@ -294,15 +295,79 @@ fn test_untagged_algorithm_stdin() {
 }
 
 #[test]
-fn test_sha2_wrong_length() {
-    for l in [0, 13, 819_111_123] {
+fn test_sha_length_invalid() {
+    for algo in ["sha2", "sha3"] {
+        for l in ["0", "00", "13", "56", "99999999999999999999999999"] {
+            new_ucmd!()
+                .arg("--algorithm")
+                .arg(algo)
+                .arg("--length")
+                .arg(l)
+                .arg("/dev/null")
+                .fails_with_code(1)
+                .no_stdout()
+                .stderr_contains(format!("invalid length: '{l}'"))
+                .stderr_contains(format!(
+                    "digest length for '{}' must be 224, 256, 384, or 512",
+                    algo.to_ascii_uppercase()
+                ));
+
+            // Also fails with --check
+            new_ucmd!()
+                .arg("--algorithm")
+                .arg(algo)
+                .arg("--length")
+                .arg(l)
+                .arg("/dev/null")
+                .arg("--check")
+                .fails_with_code(1)
+                .no_stdout()
+                .stderr_contains(format!("invalid length: '{l}'"))
+                .stderr_contains(format!(
+                    "digest length for '{}' must be 224, 256, 384, or 512",
+                    algo.to_ascii_uppercase()
+                ));
+        }
+
+        // Different error for NaNs
+        for l in ["512x", "x512", "512x512"] {
+            new_ucmd!()
+                .arg("--algorithm")
+                .arg(algo)
+                .arg("--length")
+                .arg(l)
+                .arg("/dev/null")
+                .fails_with_code(1)
+                .no_stdout()
+                .stderr_contains(format!("invalid length: '{l}'"));
+
+            // Also fails with --check
+            new_ucmd!()
+                .arg("--algorithm")
+                .arg(algo)
+                .arg("--length")
+                .arg(l)
+                .arg("/dev/null")
+                .arg("--check")
+                .fails_with_code(1)
+                .no_stdout()
+                .stderr_contains(format!("invalid length: '{l}'"));
+        }
+    }
+}
+
+#[test]
+fn test_sha_missing_length() {
+    for algo in ["sha2", "sha3"] {
         new_ucmd!()
-            .arg("--algorithm=sha2")
-            .arg(format!("--length={l}"))
+            .arg("--algorithm")
+            .arg(algo)
             .arg("lorem_ipsum.txt")
             .fails_with_code(1)
             .no_stdout()
-            .stderr_contains(format!("invalid length: '{l}'"));
+            .stderr_contains(format!(
+                "--algorithm={algo} requires specifying --length 224, 256, 384, or 512"
+            ));
     }
 }
 
@@ -434,15 +499,56 @@ fn test_check_untagged_sha2_multiple_files() {
 }
 
 #[test]
-fn test_sha3_wrong_length() {
-    for l in [0, 13, 819_111_123] {
-        new_ucmd!()
-            .arg("--algorithm=sha3")
-            .arg(format!("--length={l}"))
-            .arg("lorem_ipsum.txt")
-            .fails_with_code(1)
-            .no_stdout()
-            .stderr_contains(format!("invalid length: '{l}'"));
+fn test_check_sha2_tagged_variant() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+    at.touch("f");
+
+    // SHA2-xxx is an alias to SHAxxx we don't output but we still recognize.
+    let checksum_lines = [
+        (
+            "SHA224",
+            "SHA2-224",
+            "(f) = d14a028c2a3a2bc9476102bb288234c415a2b01f828ea62ac5b3e42f",
+        ),
+        (
+            "SHA256",
+            "SHA2-256",
+            "(f) = e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+        ),
+        (
+            "SHA384",
+            "SHA2-384",
+            "(f) = 38b060a751ac96384cd9327eb1b1e36a21fdb71114be07434c0cc7bf63f6e1da274edebfe76f65fbd51ad2f14898b95b",
+        ),
+        (
+            "SHA512",
+            "SHA2-512",
+            "(f) = cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e",
+        ),
+    ];
+
+    for (basic, variant, digest) in checksum_lines {
+        let stdin = format!("{basic} {digest}");
+        log_info("stdin is: ", &stdin);
+        scene
+            .ucmd()
+            .arg("--check")
+            .arg("--algorithm=sha2")
+            .pipe_in(stdin)
+            .succeeds()
+            .stdout_is("f: OK\n");
+
+        // Check that the variant works the same
+        let stdin = format!("{variant} {digest}");
+        log_info("stdin is: ", &stdin);
+        scene
+            .ucmd()
+            .arg("--check")
+            .arg("--algorithm=sha2")
+            .pipe_in(stdin)
+            .succeeds()
+            .stdout_is("f: OK\n");
     }
 }
 
@@ -653,7 +759,7 @@ fn test_length_not_supported() {
 }
 
 #[test]
-fn test_length() {
+fn test_blake2b_length() {
     new_ucmd!()
         .arg("--length=16")
         .arg("--algorithm=blake2b")
@@ -666,7 +772,7 @@ fn test_length() {
 }
 
 #[test]
-fn test_length_greater_than_512() {
+fn test_blake2b_length_greater_than_512() {
     new_ucmd!()
         .arg("--length=1024")
         .arg("--algorithm=blake2b")
@@ -678,7 +784,7 @@ fn test_length_greater_than_512() {
 }
 
 #[test]
-fn test_length_is_zero() {
+fn test_blake2b_length_is_zero() {
     new_ucmd!()
         .arg("--length=0")
         .arg("--algorithm=blake2b")
@@ -690,7 +796,7 @@ fn test_length_is_zero() {
 }
 
 #[test]
-fn test_length_repeated() {
+fn test_blake2b_length_repeated() {
     new_ucmd!()
         .arg("--length=10")
         .arg("--length=123456")
@@ -703,6 +809,23 @@ fn test_length_repeated() {
         .stdout_is_fixture("length_is_zero.expected");
 }
 
+#[test]
+fn test_blake2b_length_invalid() {
+    for len in [
+        "1", "01", // Odd
+        "",
+    ] {
+        new_ucmd!()
+            .arg("--length")
+            .arg(len)
+            .arg("--algorithm=blake2b")
+            .arg("lorem_ipsum.txt")
+            .arg("alice_in_wonderland.txt")
+            .fails_with_code(1)
+            .stderr_contains(format!("invalid length: '{len}'"));
+    }
+}
+
 #[test]
 fn test_raw_single_file() {
     for algo in ALGOS {

tests/fixtures/cksum/length_larger_than_512.expected

@@ -1,2 +1,2 @@
-cksum: invalid length: ‘1024’
-cksum: maximum digest length for ‘BLAKE2b’ is 512 bits
+cksum: invalid length: '1024'
+cksum: maximum digest length for 'BLAKE2b' is 512 bits