Skip to content

Fix Issue 18217: Don't repeatedly call unpredictableSeed to initialize rndGen#6021

Merged
dlang-bot merged 1 commit intodlang:masterfrom
n8sh:rndGen-seeding
Jan 9, 2018
Merged

Fix Issue 18217: Don't repeatedly call unpredictableSeed to initialize rndGen#6021
dlang-bot merged 1 commit intodlang:masterfrom
n8sh:rndGen-seeding

Conversation

@n8sh
Copy link
Member

@n8sh n8sh commented Jan 9, 2018

Having this code in Phobos leads others to mistakenly copy it since Phobos is generally a model of good D programming practices. Calling unpredictableSeed just once would be faster and would give a better result.

Explanation from #5788 (comment):

This method of seeding calls unpredictableSeed 624 times to initialize the MersenneTwisterEngine's internal array. Currently this just means multiplying a number 624 times modulo 2^^31-1 and XOR-ing the result against the clock each time. This is pointless but fairly harmless, although it means that initially each number will probably have the same high bit. It would be faster and give a better result to just seed MersenneTwisterEngine with a single number. Also, if a proposal like #5230 to make unpedictableSeed more "unpredictable" is implemented, this mode of seeding could become pathological.

@n8sh n8sh requested a review from wilzbach as a code owner January 9, 2018 20:15
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @n8sh! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close Bugzilla Description
18217 Don't repeatedly call unpredictableSeed to initialize rndGen

@n8sh n8sh changed the title Fix Issue 18217: Don't repeatedly call unpredictableSeed when initializing rndGen Fix Issue 18217: Don't repeatedly call unpredictableSeed to initialize rndGen Jan 9, 2018
Copy link
Member

@quickfur quickfur left a comment

Choose a reason for hiding this comment

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

Thanks! This is a good change, because it removes the illusion that we're getting "more randomness" by explicitly initializing everything in the RNG state. That's a false sense of security, because unpredictableSeed is not supposed to be a secure source of randomness, and calling repeatedly does not make the RNG any more random. Seeding is supposed to be with a single value anyway. This change makes it clear that the single value is the only real randomness we have here.

Only thing I'd add here is a comment on line 1360 to explain why we should only initialize a single state value rather than trying to fill it up.

And yes, if unpredictableSeed were ever to start reading from /dev/urandom, code like this could easily completely drain the entropy from the OS's entropy pool, thus causing potential security issues with other processes that may need a source of entropy.

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Thanks a lot. Really appreciated! FWIW we should start porting the good stuff from mir-random to Phobos, for example, we could begin with with unpredictableSeed.

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.

4 participants