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.

Sign up for GitHub

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

Jump to bottom

cksum: Added check for sum length #6606

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

cksum: Added check for sum length #6606

wants to merge 2 commits into from

Conversation

SarthakSingh31
Copy link

Fixes #6576

@cakebaker
Copy link
Contributor

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

@sylvestre
Copy link
Contributor

and it regressed a test (but maybe the test is incorrect)


--- TRY 3 STDERR:        coreutils::tests test_hashsum::test_check_b2sum_strict_check ---
thread 'test_hashsum::test_check_b2sum_strict_check' panicked at 'Expected stderr to be empty, but it's:
b2sum: WARNING: 7 lines are improperly formatted
', tests/by-util/test_hashsum.rs:932:10
stack backtrace:
   0: rust_begin_unwind
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/std/src/panicking.rs:578:5
   1: core::panicking::panic_fmt
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/panicking.rs:67:14
   2: tests::common::util::CmdResult::no_stderr
             at ./tests/common/util.rs:444:9
   3: tests::common::util::CmdResult::stdout_only
             at ./tests/common/util.rs:615:9
   4: tests::test_hashsum::test_check_b2sum_strict_check
             at ./tests/by-util/test_hashsum.rs:927:5
   5: tests::test_hashsum::test_check_b2sum_strict_check::{{closure}}
             at ./tests/by-util/test_hashsum.rs:908:36
   6: core::ops::function::FnOnce::call_once
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/ops/function.rs:250:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 2, 2024

GNU testsuite comparison:

GNU test failed: tests/cksum/b2sum. tests/cksum/b2sum is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/cksum/cksum-base64. tests/cksum/cksum-base64 is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/cksum/cksum-c. tests/cksum/cksum-c is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@SarthakSingh31
Copy link
Author

SarthakSingh31 commented Aug 2, 2024 •
edited
Loading

@sylvestre The tests pass now.

I think the checkrum.rs and sum.rs need to be rewritten with more typing. One of the big issues is that everything is referred to as length.
Like this is 100% wrong

coreutils/src/uucore/src/lib/features/checksum.rs

Lines 238 to 239 in e33cecd

create_fn: Box::new(move || Box::new(Blake2b::with_output_bytes(bits))),
bits,

Here bits (which should be bits) are passed to Blake2b::with_output_bytes() which expects to take in bytes. After some investigation I found that the bits are actually bytes. So the assignment to HashAlgorithm::bits is wrong.

I have found that this issue effects other parts of the code aswell.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 2, 2024

GNU testsuite comparison:

GNU test failed: tests/rm/rm2. tests/rm/rm2 is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@cakebaker cakebaker changed the title Added check for sum length cksum: Added check for sum length Aug 2, 2024
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@sylvestre
Copy link
Contributor

Looks good but could you please add a test to make sure we don't regress? thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cksum: incomplete hex checksum is treated as mismatch instead of improper format
3 participants
@SarthakSingh31 @cakebaker @sylvestre