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

sha1-checked: ubc_check is not used #594

Closed
newpavlov opened this issue Jun 16, 2024 · 4 comments · Fixed by #599
Closed

sha1-checked: ubc_check is not used #594

newpavlov opened this issue Jun 16, 2024 · 4 comments · Fixed by #599

Comments

@newpavlov
Copy link
Member

It looks like due to an oversight the ubc_check module is not used. Calling the Builder::use_ubc has no effect on hash computation.

This results in CI failures.

cc @dignifiedquire

@dignifiedquire
Copy link
Member

Ack. Will take a look in the next days

@dignifiedquire
Copy link
Member

It looks like due to an oversight the ubc_check module is not used.

As far as I can tell it is very much used

Calling the Builder::use_ubc has no effect on hash computation.

It does, calling use_ubc, sets use_ubc to either true or false, resulting in this branch being triggered or not:

https://github.com/RustCrypto/hashes/blob/master/sha1-checked/src/compress.rs#L691

The actual dead code warnings I am seeing are the following:

   Compiling sha1-checked v0.11.0-pre (/Users/dignifiedquire/opensource/rustcrypto/hashes/sha1-checked)
warning: fields `dv_type`, `dv_k`, `dv_b`, and `maski` are never read
  --> sha1-checked/src/ubc_check.rs:43:9
   |
42 | pub struct Info {
   |            ---- fields in this struct
43 |     pub dv_type: u32,
   |         ^^^^^^^
44 |     pub dv_k: u32,
   |         ^^^^
45 |     pub dv_b: u32,
   |         ^^^^
46 |     pub testt: Testt,
47 |     pub maski: i32,
   |         ^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

Which go back to the original C code, and I filed an issue there to better understand the reason for this: cr-marcstevens/sha1collisiondetection#90

dignifiedquire added a commit to dignifiedquire/hashes that referenced this issue Jul 3, 2024
@cr-marcstevens
Copy link

cr-marcstevens commented Jul 3, 2024

The last commit removes dvtype, dvK and dvB from the internal tables entirely.
While that is fine, these are also the actual identifiers for the tested disturbance vector.
For documentation purposes might I suggest that in the place of the removed code lines, comments are added of the form
/// disturbance vector: type= K= B=

@dignifiedquire
Copy link
Member

@cr-marcstevens you are right, made #600 to improve the situation

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 a pull request may close this issue.

3 participants