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

Added MD2 hash function #7

Merged
merged 3 commits into from
Jan 9, 2017
Merged

Added MD2 hash function #7

merged 3 commits into from
Jan 9, 2017

Conversation

felipeamp
Copy link
Contributor

I added the MD2 hash function and 3 basic tests (the ones on the Wikipedia page). I based the structure on the MD4 hash, but I'm not sure if the example code is correct.

If there are any problems, please let me know. :)

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

Thank you for you work!
Some minor changes and we are good to go!

@@ -0,0 +1,19 @@
[package]
name = "md2"
version = "0.4.0"
Copy link
Member

Choose a reason for hiding this comment

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

It's better to start from 0.1.0, as it's not required to synchronize versions across crates.

digest-buffer = "0.2"
generic-array = "0.6"
byte-tools = "0.1"
fake-simd = "0.1"
Copy link
Member

Choose a reason for hiding this comment

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

You don't use neither byte-tools, nor fake-simd, so you can remove them from here.

@@ -0,0 +1,25 @@
Copyright (c) 2016 bacher09, Artyom Pavlov
Copy link
Member

Choose a reason for hiding this comment

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

It's your work, so you can insert your own credits instead of mine and bacher09. ;)


#![no_std]
extern crate generic_array;
extern crate byte_tools;
Copy link
Member

Choose a reason for hiding this comment

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

Not used byte_tools.


fn process_block(&mut self, input: &Block) {
// Update state
for j in 0..16usize {
Copy link
Member

Choose a reason for hiding this comment

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

usize is not necessary here. Rust can infer type in this situation.

self.finalize();

let mut out = GenericArray::default();
for (x, y) in self.state.x[0..16].iter().zip(out.iter_mut()) {
Copy link
Member

Choose a reason for hiding this comment

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

The simple loop:

for i in 0..16 {
    out[i] = self.state.x[i];
}

will be more readable and should have the same efficiency. (compiler can remove bound checks in this case) Or you can use copy_memory from byte_tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree!

}
}

impl Default for Md2 {
Copy link
Member

Choose a reason for hiding this comment

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

Actually instead of implementing Default like this you can simply derive it for Md2 and Md2State. After that you will not need new() for Md2State and you will be able to simply use Default::default() in Md2::new(). (well some argue that there is no need for new without any arguments and that Default trait would be sufficient, maybe in the future I will remove new() altogether)

break;
}
}
print_result(&sh.result(), name);
Copy link
Member

Choose a reason for hiding this comment

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

Now we can write simply println!("{:x}\t{}", sh.result(), name), so print_result function is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing that, but the compiler complains that <D as md2::Digest>::OutputSize (which is generic_array::typenum::U16) does not implement std::ops::Add.
Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, implementation of LowerHex and UpperHex requires additional trait bounds to create array with double length internally. The easiest way will be to remove unnecessary generalization over Digest and simply create Md2 instance inside process:

fn process<R: Read>(reader: &mut R, name: &str) {
    let mut sh = Md2::new();
    ...
    println!("{:x}\t{}", &sh.result(), name);
}


// Update checksum
let mut l = self.checksum[15];
for j in 0..16usize {
Copy link
Member

Choose a reason for hiding this comment

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

Same for usize.

}

// #[test]
// fn md2_1million_a() {
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's better to add 1million_a test, e.g. by generating it by another tool, or remove it altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point I guess I'll just remove it. Later I'll try to create the 1million_a test file.

@felipeamp
Copy link
Contributor Author

Anything else?

@newpavlov newpavlov merged commit 8f22bae into RustCrypto:master Jan 9, 2017
@newpavlov
Copy link
Member

Everything looks good! Thank you!

@newpavlov newpavlov mentioned this pull request Jan 9, 2017
18 tasks
linkmauve pushed a commit to linkmauve/hashes that referenced this pull request Jan 4, 2020
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.

2 participants