Skip to content

[RFC] RNG fixes #1986

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

Merged
merged 10 commits into from
Jul 17, 2016
Merged

[RFC] RNG fixes #1986

merged 10 commits into from
Jul 17, 2016

Conversation

lt
Copy link
Contributor

@lt lt commented Jul 6, 2016

This implements all proposed changes. Some may need to be reverted depending on vote results.

https://wiki.php.net/rfc/rng_fixes

@nikic
Copy link
Member

nikic commented Jul 6, 2016

To clarify: Does the MT compatibility mode work if you specify a range? I would expect you'd still get a different sequence because the scaling is done differently. (Though I personally recommend not listening to the single loud person who wants a compatibility mode.)

Btw, we already have a bitset implementation in zend_bitset.h.

@lt
Copy link
Contributor Author

lt commented Jul 6, 2016

The mode takes effect from the next mt_srand() from userland or mt_reload() internally.

If you're not using mt_srand() then it doesn't matter what mode you're in, you wont notice at all.

And yes:

mt_srand(1234, MT_RAND_PHP);
var_dump(mt_rand(0, 1000));

Will produce different output to:

mt_srand(1234 /*, MT_RAND_MT19937 */);
var_dump(mt_rand(0, 1000));

Did you mean does ranged output produce different from pre 7.1 even in compatibility mode? Yes, it does. (fixing RAND_RANGE is a distinct option from fixing mt_rand)

PHP_MD5_CTX md5ctx;
unsigned char hash[16];

php_random_bytes_throw(&nonce, sizeof(nonce));
nonce &= 0x7fffffff;
Copy link

Choose a reason for hiding this comment

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

this suggests that sizeof(nonce) is guaranteed to be 4. the previous suggests that it is not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

php_rand() would have originally returned up to 2 ** 31

The nonce variable may be 32 or 64 bit, but I'm limiting it to the same range of values it previously had.

@tom--
Copy link

tom-- commented Jul 7, 2016

Since the only users who need the compat sequence are using a seed, why not hide the mode selector in that function?

 void mt_srand ( [ int $seed [ , int $mode ] ] )

Otherwise the 7.1 migration guide will document mt_rand_mode() as a new function. People will read about it and it's going to make no practical sense to all but the most specialized users. How many users will see that and declare, "At last, PHP has a proper MT19337 generator! I've been waiting for years for it. Now I can get that xyz work done." :þ

I know the vote already started but...

@lt
Copy link
Contributor Author

lt commented Jul 7, 2016

This feels a lot saner than my knee-jerk addition to appease some camps.

The vote is on the concept right. The concept is to provide access to the legacy RNG. The implementation can vary slightly in my opinion.

I think we can get away with changing this detail regardless of the vote, because it is wholly better than the additional function.

lt added 2 commits July 7, 2016 15:14
The mode of operation is intrinsically linked to seeding, so this makes a lot of sense
@jpauli jpauli added the RFC label Jul 8, 2016
# n' = a + n(b-a+1)/(M+1)
*
* -RL
*/
Copy link
Member

Choose a reason for hiding this comment

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

This comment no longer applies, right?

@php-pulls php-pulls merged commit 027375d into php:master Jul 17, 2016
php-pulls pushed a commit that referenced this pull request Jul 17, 2016
* rng-fixes:
  Fix legacy mode RAND_RANGE and 32/64-bit consistency
  Fix crypt salt not being converted to b64
  Make mode selection part of mt_srand()
  Use zend_bitset
  Improve array_rand distribution
  Fix some insecure usages of php_rand
  Alias rand to mt_rand
  Fix RAND_RANGE for mt_rand
  Fix mt_rand impl. Provide legacy impl. access.
  Split rand and mt_rand into separate files
@tom--
Copy link

tom-- commented Jul 27, 2016

Statistical tests of mt_rand(PHP_INT_MIN, PHP_INT_MAX) are encouraging

https://gist.github.com/tom--/7311a6fb3bfd00a9f527bb80ac27d668

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

Successfully merging this pull request may close these issues.

5 participants