cksum: implement check (Closes: #5705) #6390
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: |
BenWiederhake
left a comment
BenWiederhake
left a comment
There was a problem hiding this comment.
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.
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 formattedI 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.
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) |
BenWiederhake
left a comment
BenWiederhake
left a comment
There was a problem hiding this comment.
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
bin 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: |
sylvestre
changed the title
BenWiederhake
left a comment
BenWiederhake
left a comment
There was a problem hiding this comment.
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.
3 participants
No description provided.