dd: stop at a zero factor in a multiplier expression - #14299
Open
oliver-kopcik wants to merge 2 commits into
Open
dd: stop at a zero factor in a multiplier expression#14299oliver-kopcik wants to merge 2 commits into
oliver-kopcik wants to merge 2 commits into
Conversation
A zero factor makes the whole multiplier expression zero, so the factors
after it are never looked at. We parsed every factor up front instead, so
a later one that does not fit in a u64 failed the whole argument:
$ dd count=00x9999999999999999999999999999999999999999999999999999999999999
dd: invalid number: '00x999…': Value too large for defined data type
That should copy zero blocks and exit successfully, which is what it did
before 7f9b9a6 ("dd: reject a number that does not fit in u64") turned
ParseSizeError::SizeTooBig into an error rather than u64::MAX. The zero
factor case was not covered there.
Return as soon as a factor parses to zero, so the rest is never parsed. A
number that does not fit in a u64 on its own stays an error.
The zero-multiplier warning is only a check on the literal text, so it is
kept in its own pass and still reported once per "0" factor.
Fixes uutils#14160
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
GNU testsuite comparison: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #14160.
The bug
A zero factor makes a whole multiplier expression zero, so the factors after it are never looked at.
parse_bytes_with_opt_multiplierparsed every factor up front instead, so a later factor that does not fit in au64failed the whole argument:It should copy zero blocks and exit successfully, which it did before 7f9b9a6 ("dd: reject a number that does not fit in u64") turned
ParseSizeError::SizeTooBiginto an error rather thanu64::MAX. The zero factor case was not covered there.After this change:
The change
Return as soon as a factor parses to zero, so the remaining factors are never parsed. A number that does not fit in a
u64on its own stays an error, so the behaviour 7f9b9a6 added is kept.The zero-multiplier warning is only a check on the literal text and needs no parsing, so it moved into its own pass over the factors. That keeps it reported once per
"0"factor —count=0x0x1still warns twice, astest_zero_multiplier_warningrequires. Folding it into the short-circuiting loop would have silently dropped the second warning.Tests
Added a unit test in
parseargs.rsand an end-to-end test intests/by-util/test_dd.rs. Both fail onmainand pass with the change — verified by reverting just theif num == 0block and re-running:The unit test covers a zero factor leading, in the middle, and trailing, with and without an oversized factor after it.
Run locally on Windows (x86_64-pc-windows-msvc):
cargo test -p uu_dd --lib— 84 passed, 0 failedcargo test --features dd --test tests -- test_dd— 111 passed, 0 failedcargo fmt --all -- --check— cleancargo clippy -p uu_dd --all-targets— no new warningsOn
tests/dd/misc#14160 expects this fix alone to leave GNU's
tests/dd/miscstill failing, on a second difference(
dd: failed to seek in output file: Illegal seek). That turns out not to hold — the GNU test jobon this PR reports:
So the whole test passes now, and the second difference either does not reproduce in CI or has been
resolved since the issue was filed. (An earlier version of this description repeated the issue's
expectation as though it still applied.)