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

Clean up hasher discussion on HashMap #36557

Merged
merged 2 commits into from
Sep 30, 2016
Merged

Conversation

sfackler
Copy link
Member

  • We never want to make guarantees about protecting against attacks.
  • "True randomness" is not the right terminology to be using in this
    context.
  • There is significantly more nuance to the performance of SipHash than
    "somewhat slow".

r? @steveklabnik

Follow up to discussion on #35371

* We never want to make guarantees about protecting against attacks.
* "True randomness" is not the right terminology to be using in this
    context.
* There is significantly more nuance to the performance of SipHash than
    "somewhat slow".
@brson
Copy link
Contributor

brson commented Sep 19, 2016

I'm not sure about "We never want to make guarantees about protecting against attacks." This has been a stated goal since Rust has had hash maps. If we aren't guaranteeing that then why bother doing it?

@sfackler
Copy link
Member Author

Guarantees don't tend to exist in the security space IME. We believe SipHash 1-3 is resistant to discovery of its internal key. We believe it is hard to generate collisions without knowing that key. We believe the kernel is giving us high quality random data, etc.

@sfackler
Copy link
Member Author

Ping

/// require this behavior you can create your own hashing function using
/// [BuildHasherDefault](../hash/struct.BuildHasherDefault.html).
/// By default, `HashMap` uses a hashing algorithm selected to provide
/// resistance against HashDoS attacks. The algorithm is randomly seeded, and a
Copy link
Member

Choose a reason for hiding this comment

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

Is there perhaps a wikipedia article or something we could link to here with "HashDoS attacks" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't able to find anything that seems like a particularly good source to link to, unfortunately.

/// The hashing algorithm can be replaced on a per-`HashMap` basis using the
/// `HashMap::default`, `HashMap::with_hasher`, and
/// `HashMap::with_capacity_and_hasher` methods. Many alternative algorithms
/// are available on crates.io.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this could mention "such as fnv" which seems to be the most popular alternative?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 29, 2016

📌 Commit aaf32aa has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Sep 29, 2016

⌛ Testing commit aaf32aa with merge f390451...

@alexcrichton
Copy link
Member

@bors: retry force clean

  • restarted buildbot

@bors
Copy link
Contributor

bors commented Sep 30, 2016

⌛ Testing commit aaf32aa with merge 7660bdf...

bors added a commit that referenced this pull request Sep 30, 2016
Clean up hasher discussion on HashMap

* We never want to make guarantees about protecting against attacks.
* "True randomness" is not the right terminology to be using in this
    context.
* There is significantly more nuance to the performance of SipHash than
    "somewhat slow".

r? @steveklabnik

Follow up to discussion on #35371
@bors bors merged commit aaf32aa into rust-lang:master Sep 30, 2016
@sfackler sfackler deleted the fix-hashdos-docs branch November 26, 2016 05:48
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.

5 participants