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

Add Digest::input_hashable method #333

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

canndrew
Copy link
Contributor

A method to Digest to allow it to take data of any type that implements std::hash::Hash. Add a default impl for the method.

A method to Digest to allow it to take data of any type that implements
Hash. Add a default impl for the method.
@canndrew
Copy link
Contributor Author

I'm surprised this didn't exist already which makes me suspect I'm missing some obvious reason why it shouldn't exist. But anyway, I've found this very useful so I'm leaving this PR here as a suggestion.

@canndrew
Copy link
Contributor Author

canndrew commented Jul 2, 2016

@DaGenix Any interest in this?

@canndrew
Copy link
Contributor Author

canndrew commented Sep 1, 2016

@DaGenix Bump.

I'm using a local, patched version of rust-crypto that has this for a project that I'm thinking of publishing on crates.io. I don't really wanna have to remove it because it's quite useful.

@FredericJacobs
Copy link

FredericJacobs commented Sep 5, 2016

Would love seeing something like this merged too. (Didn't review the PR yet)

@burdges
Copy link

burdges commented Sep 5, 2016

You guys saw the other two open pull requests : #373 #294

If I understand correctly, this pull request allows one to use a cryptographic hash supplied by rust-crypto wherever one might wish to use a regular Rust hash, yes? I think that's okay for security, but..

It'll kill the performance of many tools that expect non-cryptographic hash functions, likely including Rust's HashMap. I have not examined their Robin Hood hashing strategy, but they do keep a u64 hash, so maybe not.

As an example, I'm considering writing a cuckoo hashing based alternative for HashMap that I plan to use to store secret key material indexed by their hashes. It requires a cryptographic has for the public identifier key while the value remains the secret key. It need not store this public identifier key that acts as the key into the hash table, but it does need a small fingerprint of it from which to do it's cuckoo egg thang.

@burdges
Copy link

burdges commented Sep 5, 2016

Oh wait. I just saw you have finish returning 0, so it cannot be used at all by data structures. I suppose you're only trying to consume data, not use this hash function. And you want to avoid calling the possibly expensive finish function for the cryptographic hash. I don't suppose it's possible or wise to panic on a call to finish is it?

@canndrew
Copy link
Contributor Author

canndrew commented Sep 9, 2016

I don't suppose it's possible or wise to panic on a call to finish is it?

That's probably a better idea really. The point of this Hasher impl is only to extract data out of a Hash implementer and put it into a Digest. It doesn't make sense to ever call finish. plus DigestHaser is private to the module.

@@ -11,13 +11,19 @@
use std::iter::repeat;
use std::hash::{Hash, Hasher};

/*
* The purpose of this type is to implement `Hasher` so that it can extract data from any type
* which implements `Hash` and write the dataa to a `Digest`. This type is private to this module

Choose a reason for hiding this comment

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

Typo: "dataa"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

@burdges
Copy link

burdges commented Sep 14, 2016

We could write an RFC for a std::HasherIn trait that splits off the final call, so that one does not need to mess with it. Thoughts?

@burdges
Copy link

burdges commented Sep 14, 2016

Or maybe someone need to byte the bullet and try to standardize something better than the ByteOrder create for doing that sort of thing.

Edit : I mean, I suppose the underlying issue here is the need to hash larger integer types, but that brings in endianness issues.

@burdges
Copy link

burdges commented Sep 16, 2016

Anyways this input_hashable solution looks plenty good enough for any use cases I've noticed. It's necessary for developers to call .to_le() or whatever, as libcore does not do it, but that's fine.

@Ericson2314
Copy link

I just opened rust-lang/rfcs#1768 which I believe is relevant.

@burdges
Copy link

burdges commented Nov 23, 2016

Just fyi RFC rust-lang/rfcs#1666 would break your input_hashable.

Appears impl<T: Hash> Hash for [T] includes the length so it's not a literal translation.

If T does not contain an [_] itself, then you can avoid this by calling hash_slice. And rust-lang/rfcs#1666 would create a way to patch it recursively.

Also endianness can make a mess of things if one gets sloppy here. It's easy enough to patch if you wrap the Hasher as I mentioned in rust-lang/rfcs#1666 (comment)

@burdges
Copy link

burdges commented Nov 30, 2016

Just uploaded a quick and dirty wrapper crate to address endiannes in hashing. I suppose it won't work with private Hashers like input_hashable though.

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.

4 participants