Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dd: should terminate with error if skip argument is too large #7275

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ic3man5
Copy link
Contributor

@ic3man5 ic3man5 commented Feb 6, 2025

Fixes #7216

 cargo run dd bs=1 skip=9223372036854775808 count=0
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.11s
     Running `target/debug/coreutils dd bs=1 skip=9223372036854775808 count=0`
dd: invalid number: ‘'9223372036854775808': Value too large for defined data type’

@ic3man5
Copy link
Contributor Author

ic3man5 commented Feb 6, 2025

Notes:

  • slight output difference:
    dd: invalid number: ‘'9223372036854775808': Value too large for defined data type’
    instead of:
    dd: invalid number: '9223372036854775808': Value too large for defined data type
  • seek also had this issue. For some reason GNU coreutils uses a signed integer and checks to make sure its in range of 0 and max i64.

@ic3man5
Copy link
Contributor Author

ic3man5 commented Feb 6, 2025

reference to the check in GNU coreutils:
https://github.com/coreutils/coreutils/blob/master/src/dd.c#L1581C23-L1581C39

Copy link

github-actions bot commented Feb 6, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)

@cakebaker
Copy link
Contributor

Can you please add a test to ensure we don't regress in the future?

@ic3man5
Copy link
Contributor Author

ic3man5 commented Feb 6, 2025

Can you please add a test to ensure we don't regress in the future?

yes, do we care that the GnuTests CI step failed? I'm not sure how to tell what test exactly failed from the logs.

I ran this gnu test but I'm unsure of how to compare it to see what test actually failed.

$ bash util/run-gnu-test.sh

============================================================================
Testsuite summary for GNU coreutils 9.6.8-fbfd88-dirty
============================================================================
# TOTAL: 617
# PASS:  469
# SKIP:  85
# XFAIL: 0
# FAIL:  63
# XPASS: 0
# ERROR: 0
============================================================================
See ./tests/test-suite.log for debugging.
Some test(s) failed.  Please report this to [email protected],
together with the test-suite.log file (gzipped) and your system
information.  Thanks.
============================================================================

@cakebaker
Copy link
Contributor

do we care that the GnuTests CI step failed?

It fails because of an intermittent test and you can ignore it.

Copy link

github-actions bot commented Feb 7, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)

@ic3man5
Copy link
Contributor Author

ic3man5 commented Feb 7, 2025

@cakebaker this should be ready to go now.

Comment on lines +227 to +229
return Err(ParseError::InvalidNumber(format!(
"'{skip}': Value too large for defined data type"
)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a similar error in uucore::parse_size here:

Self::SizeTooBig(format!(
"{}: Value too large for defined data type",
s.quote()
))
Could that be reused in some way? I guess GNU probably determines that the skip and seek are too large at the time of parsing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfinkels yes, they determine it when its parsed. Honestly I feel like its a "bug" upstream. I can reuse uucore::parse_size if you'd like instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dd: should terminate with error if skip argument is too large
3 participants