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

Remove @ types from Sha1 implementation, create Digest trait, and implement most of the SHA-2 functions #7207

Merged
merged 4 commits into from Jun 24, 2013

Conversation

Projects
None yet
5 participants
@DaGenix
Copy link

DaGenix commented Jun 18, 2013

This pull request contains an unoptimized implementation of most of the SHA-2 functions (everything but the variable output size versions). I also created a common trait (Digest) for all of the interesting methods for working with digests and updated the existing SHA-1 code to use it. Finally, while working with the SHA-1 code, I got rid of the use of @ types and type objects.

I've tested all functions against the Wikipedia test vectors. Additionally, I tested the SHA-512, 384, and 256 variants against the Java implementations of those digests.

I did my best to try to follow Rust conventions, but, there are so many different conventions in the code base right now that I'm not sure if I'm following the correct one or not. Anyway, I'm happy to rework if I didn't get the coding convention right (or if there are bugs!).

@DaGenix

This comment has been minimized.

Copy link

DaGenix commented Jun 18, 2013

Bah! I tested with make check-stage1-extra NO_REBUILD=1 which doesn't enforce documentation like the full build does, apparently. Running with the full build failed with complaints about missing documentation. I will re-open once I address this.

@DaGenix DaGenix closed this Jun 18, 2013

@DaGenix

This comment has been minimized.

Copy link

DaGenix commented Jun 20, 2013

Ok, I've fixed up the issues with missing documentation. Reopening the pull request.

@DaGenix DaGenix reopened this Jun 20, 2013

@cmr

This comment has been minimized.

Copy link
Member

cmr commented Jun 21, 2013

@DaGenix this PR is stale, so you'll need to rebase against master (it was probably the vector work)

@huonw

This comment has been minimized.

Copy link
Member

huonw commented Jun 21, 2013

This looks really good! ... But I've got a few nitpicks:

There are several instances of ~[] vectors that can and should be fixed length. E.g. for Sha1, work_buf should use a fixed length vector i.e. in the type declaration: [u32, .. WORK_BUF_LEN] (without any ~'s), and when initialising it [0, .. WORK_BUF_LEN]. Similarly for msg_block and h. (As an example the IsaacRng in rand.rs).

Lastly, the new functions should return non-boxed values (i.e. Sha1 instead of ~Sha1). No need to force an allocation when the consumer doesn't necessarily want one, and if they do want one then ~Sha1::new() should be fine.

(Also, the convention is for static constants to be uppercase.)

@huonw

This comment has been minimized.

Copy link
Member

huonw commented Jun 21, 2013

And, default methods just landed (yay!), so input_str etc can be moved to the Digest trait as you suggest.

@DaGenix

This comment has been minimized.

Copy link

DaGenix commented Jun 22, 2013

Thanks for all the feedback! I'll try to get an updated version of this in the next few days.

@DaGenix DaGenix closed this Jun 22, 2013

@DaGenix

This comment has been minimized.

Copy link

DaGenix commented Jun 24, 2013

I've implemented almost all of the changes mentioned by reviewers above and also modified the Digest trait to so that its possible to calculate a digest value without any heap allocation. The only change I didn't implement was to use default methods. When I tried to do that, I ended up getting the error:

rust/src/libextra/workcache.rs:253:4: 253:37 error: internal compiler error: method not found in AST map?!
rust/src/libextra/workcache.rs:253 (sha).input_str(json_encode(t));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make: *
* [x86_64-unknown-linux-gnu/stage0/test/extratest-x86_64-unknown-linux-gnu] Error 101

which makes me think that there might still be some bugs remaining in the default methods implementation. I don't have a simple test case for that issue, but I can make a branch available that triggers it if desired.

I think there is some room for extracting out the common code for the SHA-256 and SHA-512 implementations into a some sort of generic structure. It would be even better if that generic structure could support other block based digests. However, I tried to do this a couple of times and kept running into the same issue: although the algorithms are very similar they contain enough small differences that its harder to do this than I'd thought it would be. My thinking is that it might be easier to try to handle this as part of an MD-5, SHA-3, or some other digest implementation since I don't know enough about how those digests work to feel all that confident that I'd be able to create a generic structure that would handle them right now. Anyway, as long as the Digest trait's definition is solid, changing the implementation shouldn't end up breaking any code.

@DaGenix DaGenix reopened this Jun 24, 2013

@huonw

This comment has been minimized.

Copy link
Member

huonw commented Jun 24, 2013

@DaGenix yes, default methods still seem to have some large(ish) bugs in them, so that can be done later.

(Can you file a bug with that error message (and a small testcase, if you have one), and leave a FIXME #<bug number> in the code?)

@DaGenix

This comment has been minimized.

Copy link

DaGenix commented Jun 24, 2013

I've rebased to add in the requested FIXME to the DigestUtil trait.

Hopefully I've accounted for all the requested changes; if I've missed anything, please let me know and I'll be happy to fix it up!

@huonw

View changes

src/libextra/crypto/sha2.rs Outdated
}

static K32: [u32, ..64] = [
0x428a2f98u32, 0x71374491u32, 0xb5c0fbcfu32, 0xe9b5dba5u32, 0x3956c25bu32, 0x59f111f1u32, 0x923f82a4u32, 0xab1c5ed5u32,

This comment has been minimized.

@huonw

huonw Jun 24, 2013

Member

Are these lines too long? (The limit is 100 chars, you can use make tidy (which also runs as part of make check) to double check.)

@huonw

This comment has been minimized.

Copy link
Member

huonw commented Jun 24, 2013

Other than the lines being too long (which means it'll fail the tests), this looks good!

Palmer Cox added some commits Jun 23, 2013

Palmer Cox
Improve the SHA-1 implementation
* Rename struct Sha1State to Sha1
* Remove all use of @ types
* Use fixed length vectors
* Move all of the inner functions from inside sha1() to top level, private functions
* Sha1 instances are now created via Sha1::new()
* Update all constant names to uppercase
* Remove unecessary assert_eq!s
* Remove check_vec_eq() helper function; use vec::eq() instead
Palmer Cox
Create a Digest trait for common methods on digests and convert the S…
…HA-1 implementation to use it.

The DigestUtil trait was created for helper methods since default methods still have issues.
@DaGenix

This comment has been minimized.

Copy link

DaGenix commented Jun 24, 2013

A normal make wasn't failing, but make check was complaining about some of the lines being too long. I've rebased again to fix that and also removed an unused use from sha1.rs.

@pcwalton

This comment has been minimized.

Copy link

pcwalton commented on 711273f Jun 24, 2013

r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented on 711273f Jun 24, 2013

saw approval from pcwalton
at DaGenix@711273f

This comment has been minimized.

Copy link
Contributor

bors replied Jun 24, 2013

merging DaGenix/rust/sha2 = 711273f into auto

This comment has been minimized.

Copy link
Contributor

bors replied Jun 24, 2013

DaGenix/rust/sha2 = 711273f merged ok, testing candidate = 832fe32

This comment has been minimized.

Copy link
Contributor

bors replied Jun 24, 2013

fast-forwarding master to auto = 832fe32

bors added a commit that referenced this pull request Jun 24, 2013

auto merge of #7207 : DaGenix/rust/sha2, r=pcwalton
This pull request contains an unoptimized implementation of most of the SHA-2 functions (everything but the variable output size versions). I also created a common trait (Digest) for all of the interesting methods for working with digests and updated the existing SHA-1 code to use it. Finally, while working with the SHA-1 code, I got rid of the use of @ types and type objects.

I've tested all functions against the Wikipedia test vectors. Additionally, I tested the SHA-512, 384, and 256 variants against the Java implementations of those digests.

I did my best to try to follow Rust conventions, but, there are so many different conventions in the code base right now that I'm not sure if I'm following the correct one or not. Anyway, I'm happy to rework if I didn't get the coding convention right (or if there are bugs!).

@bors bors closed this Jun 24, 2013

@bors bors merged commit 711273f into rust-lang:master Jun 24, 2013

1 check passed

default all tests passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment