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

Fixes initialization of ISAAC PRNG without explicit seed #4882

Merged
merged 1 commit into from Aug 28, 2017

Conversation

konovod
Copy link
Contributor

@konovod konovod commented Aug 24, 2017

#4789 introduced a problem - there is no spec ensuring that PRNG can be actually created without seed. So "seeding" it added for ISAAC was a piece of code that don't even compiles.

This PR fixes it and adds spec that ensures that initialization at least compiles. I don't know what else can be checked in spec (i think we can compare that next_u of two created instances aren't equal, but this means that there will be 1/2^32 chance that they will match and spec fails).


private def random_seeds
result = uninitialized Seeds
SecureRandom.random_bytes(result.to_unsafe.as(UInt8*).to_slice(sizeof(Seeds)))
Copy link
Contributor

@RX14 RX14 Aug 24, 2017

Choose a reason for hiding this comment

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

Would unsafe_as(StaticArray(UInt8, 32)).to_slice work?

Copy link
Contributor Author

@konovod konovod Aug 24, 2017

Choose a reason for hiding this comment

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

Yeah, this way it looks cleaner 😄 . Is StaticArray(UInt8, sizeof(Seeds)) ok?

Copy link
Contributor

@RX14 RX14 Aug 24, 2017

Choose a reason for hiding this comment

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

If it complies it's great.


private def random_seeds
result = uninitialized Seeds
SecureRandom.random_bytes(result.to_unsafe.as(UInt8*).to_slice(sizeof(Seeds)))
Copy link
Contributor

@RX14 RX14 Aug 24, 2017

Choose a reason for hiding this comment

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

If it complies it's great.


private def random_seeds
result = uninitialized Seeds
SecureRandom.random_bytes(result.unsafe_as(StaticArray(UInt8, sizeof(Seeds))).to_slice)
Copy link
Contributor

@RX14 RX14 Aug 24, 2017

Choose a reason for hiding this comment

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

Please extract result.unsafe_as(StaticArray(UInt8, sizeof(Seeds))).to_slice to a result_slice variable. The line's getting quite messy.

Copy link
Contributor Author

@konovod konovod Aug 24, 2017

Choose a reason for hiding this comment

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

Done.

RX14
RX14 approved these changes Aug 24, 2017
@ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Aug 25, 2017

Why requiring SecureRandom when Crystal::Random::System can be used directly in stdlib?

@konovod
Copy link
Contributor Author

@konovod konovod commented Aug 25, 2017

I don't understand what is the difference (I thought that SecureRandom was renamed to Random::System but then discovered that it's still here).
SecureRandom is a safe random source, while Crystal::Random::System is it's platform independent implementation?

@luislavena
Copy link
Contributor

@luislavena luislavena commented Aug 25, 2017

I don't understand what is the difference (I thought that SecureRandom was renamed to Random::System but then discovered that it's still here).

That definitely is worth a blogpost or similar that describes what the stdlib should look like moving forward. /cc @mverzilli @matiasgarciaisaia @bcardiff

@luislavena
Copy link
Contributor

@luislavena luislavena commented Aug 25, 2017

@konovod a bit more of background (until someone can produce a condensed and concise response here):

Cheers

@konovod
Copy link
Contributor Author

@konovod konovod commented Aug 25, 2017

@luislavena i think i already read them (maybe not thoroughly). But when reading i thought that it was replacing secure_random and i was confused to see it's still here (and just calls Crystal::Random::System). What to use in any particular case - Crystal::Random::System.random_bytes or SecureRandom.random_bytes?

So, if I understand correctly, the difference is that SecureRandom is an interface exposed to end users, while Crystal::Random::System is a low-level and internal part of stdlib, so former should be used in external shards, while latter should be used inside stdlib?

I've updated PR.

@RX14
Copy link
Contributor

@RX14 RX14 commented Aug 25, 2017

This weird phobia against including SecureRandom in the prelude is just crazy.

The system random situation in crystal is also just crazy. There should be one class in crystal which uses Crystal::System::Random: Random::System. And it should provide a random_bytes method.

And you know how I feel about SecureRandom, it should be unified with Random.

All the current mess just serves to confuse people, as this thread continues to prove.

@luislavena
Copy link
Contributor

@luislavena luislavena commented Aug 25, 2017

All the current mess just serves to confuse people, as this thread continues to prove.

I have no personal position on that, so I will suggest a RFC for that be reviewed by core and other contributors.

Please do, so the RFC can be used as guidance and modifications be introduced to simply current approach.

@mverzilli mverzilli merged commit 98af739 into crystal-lang:master Aug 28, 2017
2 checks passed
@mverzilli
Copy link

@mverzilli mverzilli commented Aug 28, 2017

Thank you @konovod! I was about to say you're steadily becoming "Crystal's Random Guy" but realized it didn't sound so positive :P.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants