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

Random::ISAAC: fix initial seed #7977

Merged
merged 3 commits into from
Jul 21, 2019

Conversation

asterite
Copy link
Member

Fixes #7976

In the old code unsafe_as returned a copy of the static array and so it was initializing something else other than what was returned.

@asterite asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib labels Jul 19, 2019
it "different instances generate different numbers (#7976)" do
isaacs = Array.new(1000) { Random::ISAAC.new }
values = isaacs.map(&.rand(10_000_000))
values.uniq.size.should be > 2
Copy link
Member Author

Choose a reason for hiding this comment

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

This spec might fail, but it's veeeeery unlikely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any other way to test this? Otherwise I can remove the test altogether.

src/random/isaac.cr Outdated Show resolved Hide resolved
src/random/isaac.cr Outdated Show resolved Hide resolved
@ysbaddaden
Copy link
Contributor

What about an uninitialized UInt8[32], fill it in one call to random_bytes instead of 8 calls to rand, then cast it to a StaticArray(UInt32, 8)?

That would spare 7 unbuffered reads from urandom (or 7 calls to getrandom, arc4random, ...) yet fix the issue, wouldn't it?

@asterite
Copy link
Member Author

I think so, yes, but to do that we need to use "unsafe_as". I wanted to avoid unsafe code. But I'll try it anyway.

Another idea is to have such code in Secure::Random, somehow, so that it's unsafe but in a single place.

@asterite
Copy link
Member Author

Updated!

I added a general Random#rand(StaticArray(T, N)) that, for now, only works with integer types. Its implementation uses unsafe_as but it's not really unsafe (or it shouldn't be).

Then I used that method in the initialization of Random::ISSAC seed.

In the future I'd also like to be able to do rand(Int32), rand(UInt64) and so on.

@asterite
Copy link
Member Author

I'll do it differently.

@asterite
Copy link
Member Author

Added more things and implemented it in a way that's easier to read in docs (now there's one overload per static array type).

Also added the ability to do rand(Int32), rand(Int8) and so on, to get a value in that type's range. For that I use random_bytes to fill all bytes and then cast it to that type (same thing as with static arrays). I don't know if that's sound but we could eventually change the implementation if needed.

src/random.cr Show resolved Hide resolved
src/random.cr Outdated Show resolved Hide resolved
@asterite asterite merged commit 1cd278d into crystal-lang:master Jul 21, 2019
@asterite asterite deleted the bug/random-issac branch July 21, 2019 09:56
@bcardiff bcardiff added this to the 0.30.0 milestone Jul 24, 2019
# rand({{type}}) # => {{values[0].id}}
# ```
def rand(type : {{type}}.class) : {{type}}
rand_type_from_bytes(type)
Copy link
Member

@oprypin oprypin Sep 24, 2019

Choose a reason for hiding this comment

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

It's strange to have a separate implementation for this when rand(type::MIN..type::MAX) works fine.

Also kind of funny that random_bytes unrolls integers into bytes (and inverts for endinanness) and then this rolls them back into an integer (without inverting for endianness - though double-inverting would also be weird).

So yes, this implementation is endianness-dependent, unlike everything else.

And then, just by coincidence,
Random.new(x).rand(Int32::MIN..Int32::MAX) == Random.new(x).rand(Int32)
but
Random.new(x).rand(Int64::MIN..Int64::MAX) != Random.new(x).rand(Int64)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random::ISAAC does not randomize the outcome
6 participants