-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
cksum: implement check (Closes: #5705) #6390
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
Conversation
GNU testsuite comparison:
|
GNU testsuite comparison:
|
should fail but getting close |
GNU testsuite comparison:
|
GNU testsuite comparison:
|
coreutils/coreutils@b5ce9fb I contributed this upstream as they were missing |
GNU testsuite comparison:
|
GNU testsuite comparison:
|
1 similar comment
GNU testsuite comparison:
|
GNU testsuite comparison:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a few places where we produce the wrong result, and even a panic :)
I'm weirdly proud of finding so much. Clearly it's already good code, it passes the GNU tests and even some that you created for them, and yet there are a lot of interesting edge cases! :D
src/uu/cksum/src/cksum.rs
Outdated
io::ErrorKind::Other, | ||
"no properly formatted checksum lines found", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, this does not always trigger for me:
$ ../gnu/src/cksum -a blake2b README.md | tee foo.sums
BLAKE2b (README.md) = c0a230cfdbbb686f91071119c915a19df28cb2e4fc575f19d5c6b81bf4e3ca2e83e6df49f14d8b89ece14eb4090be435a7af6fed2c981774e076caa8fe62b04c
$ ../gnu/src/cksum -a sha1 -c foo.sums
../gnu/src/cksum: foo.sums: no properly formatted checksum lines found
[$? = 1]
$ cargo run -q cksum -a sha1 -c foo.sums
cksum: WARNING: 1 line is improperly formatted
I suspect that this has to do with -a sha1
being interpreted as "every line MUST use the specified algorithm, even if tagged otherwise" or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bug still exists. We definitely shouldn't accept files if they aren't correct.
GNU testsuite comparison:
|
GNU testsuite comparison:
|
GNU testsuite comparison:
|
@BenWiederhake I added tests on our side (ignored) for stuff that we don't support yet (and probably quite niche) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the problems I addressed are indeed fixed, and I mostly agree that it's a good idea to move most of the remainder for later. Hooray!
There are two exceptions that I think should be fixed in this PR:
- The new-ish issue
SHA4 (README.md) = 00000000
, see above - The bit-rounding-down issue, which causes us to accept dubious and obviously-wrong hashes. This should be just a simple div-mod, right?
Things that should be fixed eventually, but perhaps not right now:
- "accept files with explicit algo but algos mismatch" details
- inconsistent behavior caused by the inclusion of
b
in the algo-name regex:
$ echo 'bSD (README.md) = 0000' > foo.sums; cargo run -q cksum -c foo.sums
README.md: FAILED
cksum: WARNING: 1 computed checksum did NOT match
[$? = 1]
$ echo 'BsD (README.md) = 0000' > foo.sums; cargo run -q cksum -c foo.sums
cksum: foo.sums: no properly formatted checksum lines found
cksum: WARNING: 1 line is improperly formatted
[$? = 1]
Done :)
|
GNU testsuite comparison:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No more crashes, no more false-positives in the checking, and I couldn't find any new edge cases in the new code. Yay! This is ready to merge in my eyes.
No description provided.