Make new and default implementation not depend on random state #160
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This oddly fixes issue bodil#148. I tried to compose a set in a set. Which failed miserably, because the HashSet would not have the same hash, because of the random state hasher implementation. This was done to prevent hand crafted key DOS attacks. To get things working, you can build hash maps with exactly the same deterministic hasher. This becomes error prone and inconvenient quite quickly. Sprinkeling ::with_hasher() everywhere. So this commit generalizes the ::new() and Default implementation over the hasher (S) implementing Default and BuildHasher traits. This makes it possible to instantiate the RandomState specific implementations in the root lib module, while keeping the default(), new() and unit() implementations general for each hasher. So if I want a MyHashmap<K,V> with a SpecificHasher implementing Default: type MyHashMap<K,V> = im::hashmap::HashMap<K,V,SpecificHasher>; Then all functions still work for the different hasher. I would then also have to reimplement the macros. But that is fine.
dusdanig
force-pushed
the
generalize-default-new-and-unit-implementations
branch
from
80b6b47
to
645a1ae
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Great library, I use it often.
I have found a small composition problem related to #148
I was unable to compose a set as a key. Looking further it was due to the random state hashing which is a security concern. I think that is a reasonable security concern if the data structures are exposed directly. But it does preclude the hash map and hash set to be composed with itself if the default hasher is used.
So basically it is not a problem the RandomState is the default, but rather swapping it with another hasher which is deterministic. The intended goal of this commit is keeping that use case readable.
To make it somewhat more convenient to have hash sets and maps with a different hasher, I have generalized the implementations of new() and unit() such that they work with hashers which implement default. RandomState implements that trait as well, so it fits nicely.
In that case a specific version of a hashmap can be instantiated by a type definition, while keeping the Default trait and new and unit implementations:
type MyHashMap<K,V> = im::hashmap::HashMap<K,V,MySpecificHasher>;
Let me know what you think.