dd: exit when bs/ibs/obs/cbs isn't positive

[iburaky2]

Match GNU coreutils behavior:

$ dd bs=0
dd: invalid number: '0'
$ dd ibs=0
dd: invalid number: '0'
$ dd obs=0
dd: invalid number: '0'
$ dd cbs=0
dd: invalid number: '0'

$ dd bs=-5
dd: invalid number: '-5'
$ dd ibs=-5
dd: invalid number: '-5'
$ dd obs=-5
dd: invalid number: '-5'
$ dd cbs=-5
dd: invalid number: '-5'

Currently:

• bs/ibs/obs=0 panics, see dd bs=0 panics (attempt to divide by zero) #11580.
• cbs=0 seems to allow normal dd operation???
• bs/ibs/obs/cbs=-1 errors correctly after reaching parse_bytes_with_opt_multiplier

Another option would be to check in parse_bytes / parse_bytes_with_opt_multiplier but I think it's better to exit earlier.

[github-actions]

GNU testsuite comparison:

Note: The gnu test tests/csplit/csplit-heap is now being skipped but was previously passing.
Note: The gnu test tests/dd/no-allocate is now being skipped but was previously passing.
Note: The gnu test tests/tail/tail-n0f is now being skipped but was previously passing.

[oech3]

Our dd already rejects negative value. So this PR might have duplicated logic with it.

[iburaky2]

mb I got the order wrong parse_bytes comes before validate(). Since parse_bytes is only used with those 4 parameters and all should reject 0 we can handle the check there.

Negative numbers are handled here:

fn parse_bytes_no_x(full: &str, s: &str) -> Result<u64, ParseError> {
    let parser = SizeParser {
        capital_b_bytes: true,
        no_empty_numeric: true,
        ..Default::default()
    };
    let (num, multiplier) = match (s.find('c'), s.rfind('w'), s.rfind('b')) {
        (None, None, None) => match parser.parse_u64(s) { // -5 can't be parsed as y64
            Ok(n) => (n, 1),
            Err(ParseSizeError::SizeTooBig(_)) => (u64::MAX, 1),
            Err(_) => return Err(ParseError::InvalidNumber(full.to_string())), // error here
        },
        (Some(i), None, None) => (parse_bytes_only(s, i)?, 1),
        (None, Some(i), None) => (parse_bytes_only(s, i)?, 2),
        (None, None, Some(i)) => (parse_bytes_only(s, i)?, 512),
        _ => return Err(ParseError::MultiplierStringParseFailure(full.to_string())),
    };
    num.checked_mul(multiplier)
        .ok_or_else(|| ParseError::MultiplierStringOverflow(full.to_string()))
}

[github-actions]

GNU testsuite comparison:

Skip an intermittent issue tests/pr/bounded-memory (was skipped on 'main', now failing)

[sylvestre]

Thanks! Two things before this lands:

1. The negative case doesn't actually match GNU. bs=-5 currently errors via try_into() => BsOutOfRange, which prints bs=N cannot fit into memory, not GNU's invalid number: '-5'. Could you collapse it into a single check? like:
   let num = parse_bytes_with_opt_multiplier(val)?;
   if num <= 0 {
   return Err(ParseError::InvalidNumber(val.to_string()));
   }
2. Note val.to_string() (not num.to_string()) so inputs like 0x0 are echoed verbatim as GNU does.
3. The test only checks exit code and empty stdout, so it doesn't actually verify the error message - which is the whole point of the PR. Please add a .stderr_contains("invalid number") so we catch regressions in the wording.

[github-actions]

GNU testsuite comparison:

Skip an intermittent issue tests/pr/bounded-memory (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/rm/isatty (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)

[iburaky2]

I've fixed points 2 and 3 and also added 0x0 as a testcase

Regarding point 1, bs=-5 does match GNU's error message [see the passing test which now checks error message as proof :) ] because it's handled earlier in parse_bytes_no_x.

$ rust-gdb --args coreutils dd bs=-5
(gdb) b uu_dd::parseargs::parse_bytes_no_x
Breakpoint 1 at 0x1176ac4: file src/uu/dd/src/parseargs.rs, line 491.
(gdb) r
Breakpoint 1, uu_dd::parseargs::parse_bytes_no_x (full="-5", s="-5") at src/uu/dd/src/parseargs.rs:491
491	        ..Default::default()
(gdb) n
488	    let parser = SizeParser {
(gdb) 
493	    let (num, multiplier) = match (s.find('c'), s.rfind('w'), s.rfind('b')) {
(gdb) 
494	        (None, None, None) => match parser.parse_u64(s) {
(gdb) 
497	            Err(_) => return Err(ParseError::InvalidNumber(full.to_string())),
(gdb) 

I'm not fully sure I fully understand what the point of

        num.try_into()
            .map_err(|_| ParseError::BsOutOfRange(arg.to_string()))

is. I guess this would only trigger if we were on a 32 bit system (since the function returns usize) and num (u64) was too large?