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

Allow to use RandomState::new() without any features #51

Closed
a1phyr opened this issue Oct 15, 2020 · 2 comments · Fixed by #55
Closed

Allow to use RandomState::new() without any features #51

a1phyr opened this issue Oct 15, 2020 · 2 comments · Fixed by #55
Assignees

Comments

@a1phyr
Copy link
Contributor

a1phyr commented Oct 15, 2020

When building aHash without any features, the RandomState::new is disabled. I think it would be great to enable it in this case as in previous versions.

@tkaitchuck
Copy link
Owner

Yes. That would be desirable see discussion here:
rust-lang/hashbrown#207
Based on earlier discussion here:
#48

The question is how to do that "safely". Meaning that users don't make assumptions about DOS resistance that would not apply given the lack of both runtime and compile time randomization.

@tkaitchuck
Copy link
Owner

tkaitchuck commented Oct 16, 2020

getrandom works on many but not all targets. (And those unsupported targets are more likely if we're talking about no-std builds.)

Obviously re-using the generated random data is a useful property too. Currently this is done via Lazy static. Atomics + Box might be an alternative, as could thread-local. Both have issues similar in that they aren't supported universally. For reference the standard library calls sys::hashmap_random_keys() which obviously is not available without std.

Previously the code in this case was using the history / order of memory offsets where maps were constructed as a (poor) source of randomness. This is certainly better than using fixed keys, because even if it is predictable at least accidentally self-inflicted dos attacks don't happen. But it can't be labeled as secure, so it seems like maybe it should go via a different method so it's not confused.

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 a pull request may close this issue.

2 participants