Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC][VOTE] RNG fixes »

[RFC][VOTE] RNG fixes

php.internals

Leigh

10 years ago
As the discussion thread has been quiet for a while, moving this RFC to voting. https://wiki.php.net/rfc/rng_fixes https://github.com/php/php-src/pull/1986

Pierre Joye

10 years ago
Hi Looks good but missing an option to choose the default mode. I would choose BC as default at least for one release (7.1). I tend to vote against fixing mt_rand because of that. On Jul 7, 2016 5:39 PM, "Leigh" <leight@gmail.com> wrote:

Leigh

10 years ago
On 7 July 2016 at 13:39, Pierre Joye <pierre.php@gmail.com> wrote:
> Hi > > Looks good but missing an option to choose the default mode. > > I would choose BC as default at least for one release (7.1).
It's been that way since 5.2.1, I think it's had enough releases already :)

Pierre Joye

10 years ago
On Jul 7, 2016 7:53 PM, "Leigh" <leight@gmail.com> wrote:
> > On 7 July 2016 at 13:39, Pierre Joye <pierre.php@gmail.com> wrote: > > Hi > > > > Looks good but missing an option to choose the default mode. > > > > I would choose BC as default at least for one release (7.1). > > It's been that way since 5.2.1, I think it's had enough releases already
:) You mean we changed it, announce it and give a chance to adapt the code? Given than the majority of users does not run 5.2 but 5.3+.

Pierre Joye

10 years ago
On Thu, Jul 7, 2016 at 7:56 PM, Pierre Joye <pierre.php@gmail.com> wrote:
> > On Jul 7, 2016 7:53 PM, "Leigh" <leight@gmail.com> wrote: >> >> On 7 July 2016 at 13:39, Pierre Joye <pierre.php@gmail.com> wrote: >> > Hi >> > >> > Looks good but missing an option to choose the default mode. >> > >> > I would choose BC as default at least for one release (7.1). >> >> It's been that way since 5.2.1, I think it's had enough releases already >> :) > > You mean we changed it, announce it and give a chance to adapt the code? > Given than the majority of users does not run 5.2 but 5.3+.
So, I voted no then as it is clear that you do not see a problem to break codes without a single warning or time to adapt before. The other sections are fine and voted yes. I would still strongly recommend to take a less harsh way. Having to change code for such things from 7.0 to 7.1 sounds bad to me, this is the kind of behaviors one won't expect to change without a previous warning. Cheers,
-- Pierre @pierrejoye | http://www.libgd.org

Leigh

10 years ago
On Sat, 9 Jul 2016 at 08:48 Pierre Joye <pierre.php@gmail.com> wrote:
> So, I voted no then as it is clear that you do not see a problem to > break codes without a single warning or time to adapt before. > > The other sections are fine and voted yes. >
For the part where you voted no. Still nobody has presented (publicly available) source that makes legitimate use of mt_srand (yes it's mt_srand that is "broken" here, not mt_rand) for deterministic streams of random numbers. I can only assume by this that almost nobody does. However, for those that do, they can still use the old algorithm. For all of the other sections where you voted yes, they are mostly bug fixes, but all change the output of mt_rand AND more functions (without a single warning, like you wanted). I'm not trying to encourage you to change any votes, but I need to make sure you understand what you're voting for.

Pierre Joye

10 years ago
On Jul 9, 2016 3:19 PM, "Leigh" <leight@gmail.com> wrote:
> > > On Sat, 9 Jul 2016 at 08:48 Pierre Joye <pierre.php@gmail.com> wrote: >> >> So, I voted no then as it is clear that you do not see a problem to >> break codes without a single warning or time to adapt before. >> >> The other sections are fine and voted yes. > > > For the part where you voted no. Still nobody has presented (publicly
available) source that makes legitimate use of mt_srand (yes it's mt_srand that is "broken" here, not mt_rand) for deterministic streams of random numbers. I can only assume by this that almost nobody does. However, for those that do, they can still use the old algorithm. I am sorry but this PR possibly breaks BC and cases have been clearly explained how and why. Asking to show production code publically is not something that you should request. I can write a sample app to show you how but given the explanations many gave already....
> For all of the other sections where you voted yes, they are mostly bug
fixes, but all change the output of mt_rand AND more functions (without a single warning, like you wanted). I'm not trying to encourage you to change any votes, but I need to make sure you understand what you're voting for.

Leigh

10 years ago
On Sat, 9 Jul 2016 at 09:49 Pierre Joye <pierre.php@gmail.com> wrote:
> > I am sorry but this PR possibly breaks BC and cases have been clearly > explained how and why. Asking to show production code publically is not > something that you should request. > > I can write a sample app to show you how but given the explanations many > gave already.... >
Just to be clear, you voted no to one BC break, but yes to other BC breaks. I don't know how you pick which ones are acceptable, and which are not. Summary BC breaks you voted for: * No to changing the output of mt_rand after calling mt_srand with a given seed (when not specifying a min/max) * Yes to changing the output of mt_rand after calling mt_srand with a given seed (when specifying a min/max) * Yes to changing the output of rand, shuffle, str_shuffle and array_rand Do you see why this looks weird to me?

Christoph Becker

10 years ago
On 09.07.2016 at 10:49, Pierre Joye wrote:
> On Jul 9, 2016 3:19 PM, "Leigh" <leight@gmail.com> wrote: >> >> On Sat, 9 Jul 2016 at 08:48 Pierre Joye <pierre.php@gmail.com> wrote: >>> >>> So, I voted no then as it is clear that you do not see a problem to >>> break codes without a single warning or time to adapt before. >>> >>> The other sections are fine and voted yes. >> >> For the part where you voted no. Still nobody has presented (publicly >> available) source that makes legitimate use of mt_srand (yes it's mt_srand >> that is "broken" here, not mt_rand) for deterministic streams of random >> numbers. I can only assume by this that almost nobody does. However, for >> those that do, they can still use the old algorithm. > > I am sorry but this PR possibly breaks BC and cases have been clearly > explained how and why. Asking to show production code publically is not > something that you should request.
ACK. However, it appears to me that it has not been sufficiently verified that the random distribution of the current mt_rand() implementation is as good as the original algorithm (apparently, there are only some demos and quick investigations available). Therefore fixing this in a minor version with the option to enforce the old behavior looks good to me.
-- Christoph M. Becker

Christoph Becker

10 years ago
On 09.07.2016 at 10:18, Leigh wrote:
> On Sat, 9 Jul 2016 at 08:48 Pierre Joye <pierre.php@gmail.com> wrote: > >> So, I voted no then as it is clear that you do not see a problem to >> break codes without a single warning or time to adapt before. >> >> The other sections are fine and voted yes. > > For the part where you voted no. Still nobody has presented (publicly > available) source that makes legitimate use of mt_srand (yes it's mt_srand > that is "broken" here, not mt_rand) for deterministic streams of random > numbers. I can only assume by this that almost nobody does. However, for > those that do, they can still use the old algorithm. > > For all of the other sections where you voted yes, they are mostly bug > fixes, but all change the output of mt_rand AND more functions (without a > single warning, like you wanted). I'm not trying to encourage you to change > any votes, but I need to make sure you understand what you're voting for.
Thanks, Leigh, for the RFC. In my opinion, all sections except "Alias rand() to mt_rand()" and maybe "Make array_rand() more efficient" are *bug* *fixes*, and even though they might cause BC breaks, deploying them with a minor release seems to be a sensible compromise. Aliasing rand() to mt_rand() should better be postponed to PHP 8.0 (if at all). I'm aware of rand()'s limitations and platform specific behavior, but I'd prefer to leave the choice to nonetheless use it to userland (at least for now). I would suggest to improve the documentation of rand() wrt. to the note in the description section and the warning in the notes section (which are mutually contradicting themselves). Assuming that "Make array_rand() more efficient" is indeed only about performance (and not about fixes for the broken implementation), it wouldn't be a bug fix, but as array_rand()'s behavior would change anyway as noted by the RFC, it makes sense to do it as well.
-- Christoph M. Becker

Leigh

10 years ago
On Sat, 9 Jul 2016 at 10:16 Christoph Becker <cmbecker69@gmx.de> wrote:
> > Assuming that "Make array_rand() more efficient" is indeed only about > performance (and not about fixes for the broken implementation), it > wouldn't be a bug fix, but as array_rand()'s behavior would change > anyway as noted by the RFC, it makes sense to do it as well. > > Possibly badly worded by me. It makes the output of array_rand less
biased.

Christoph Becker

10 years ago
On 09.07.2016 at 11:18, Leigh wrote:
> On Sat, 9 Jul 2016 at 10:16 Christoph Becker <cmbecker69@gmx.de> wrote: > >> Assuming that "Make array_rand() more efficient" is indeed only about >> performance (and not about fixes for the broken implementation), it >> wouldn't be a bug fix, but as array_rand()'s behavior would change >> anyway as noted by the RFC, it makes sense to do it as well. >> >> Possibly badly worded by me. It makes the output of array_rand less >> biased.
Thanks for the clarification. In this case I consider it a bug fix, as array_rand() is *seriously* broken on Windows.
-- Christoph M. Becker

Tom Worster

10 years ago
On 7/7/16 6:39 AM, Leigh wrote:
> As the discussion thread has been quiet for a while, moving this RFC to voting. > > https://wiki.php.net/rfc/rng_fixes > > https://github.com/php/php-src/pull/1986 >
Nice work. The discussion persuaded me (Nikita mostly) that aliasing rand() to mt_rand() is sensible. And the compromise to fix the mt_rand() bug is good enough. Everything else is pretty much uncontroversial, I would think. A more efficient RNG can wait for another day. Tom

Leigh

10 years ago
On Thu, 7 Jul 2016 at 11:39 Leigh <leight@gmail.com> wrote:
> As the discussion thread has been quiet for a while, moving this RFC to > voting. > > https://wiki.php.net/rfc/rng_fixes > > https://github.com/php/php-src/pull/1986
Votes are now closed. Results as follows: * 19-5 - Fix mt_rand() implementation * 21-4 - Alias rand() to mt_rand() * 25-0 - Fix RAND_RANGE() * 23-0 - Replace insecure uses of php_rand() with php_random_bytes() * 24-0 - Make array_rand() more efficient There are still a couple of tweaks to the implementation to be done before merging: * Make ranged output the same on 32 and 64 bit platforms where max-min is less than 32 bits * In compatibility mode use the old RAND_RANGE for mt_rand() I intend to make time for these fixes tomorrow.