Permalink
Browse files

Revert "Fix #71152: mt_rand() returns the different values from origi…

…nal mt19937ar.c"

This reverts commit 6f6bd8c.

`mt_rand()` is seedable with `mt_srand()` which means it can be used to (re)produce specific streams of numbers. All code (no matter how few instances that may be) that previously depended on this behaviour will no longer produce the same results.

This kind of change needs to be discussed before being committed.
  • Loading branch information...
lt committed Feb 18, 2016
1 parent aa383d6 commit a0724d30817600540946b41e40f4cfc2a0c30f80
Showing with 1 addition and 54 deletions.
  1. +1 −1 ext/standard/rand.c
  2. +0 −53 ext/standard/tests/math/mt_rand_value.phpt
View
@@ -146,7 +146,7 @@ PHPAPI zend_long php_rand(void)
#define loBits(u) ((u) & 0x7FFFFFFFU) /* mask the highest bit of u */
#define mixBits(u, v) (hiBit(u)|loBits(v)) /* move hi bit of u to hi bit of v */
#define twist(m,u,v) (m ^ (mixBits(u,v)>>1) ^ ((php_uint32)(-(php_int32)(loBit(v))) & 0x9908b0dfU))
#define twist(m,u,v) (m ^ (mixBits(u,v)>>1) ^ ((php_uint32)(-(php_int32)(loBit(u))) & 0x9908b0dfU))
/* {{{ php_mt_initialize
*/

This file was deleted.

Oops, something went wrong.

39 comments on commit a0724d3

@andreas23

This comment has been minimized.

Show comment
Hide comment
@andreas23

andreas23 Feb 18, 2016

"Sure it's broken, but at least it's consistently broken!"

andreas23 replied Feb 18, 2016

"Sure it's broken, but at least it's consistently broken!"

@lt

This comment has been minimized.

Show comment
Hide comment
@lt

lt Feb 18, 2016

Contributor

There are appropriate channels to discuss whether or not it is fundamentally broken or simply non-standard. You don't break userland code without those discussions.

Contributor

lt replied Feb 18, 2016

There are appropriate channels to discuss whether or not it is fundamentally broken or simply non-standard. You don't break userland code without those discussions.

@lolphp876

This comment has been minimized.

Show comment
Hide comment
@lolphp876

lolphp876 Feb 18, 2016

It is fundamentally broken if you claim to implement an specific algorithm in a "non-standard" way that produces different of output. If you want to implement a different algorithm, call it by another name. This is why the PHP development team isn't taken seriously; they disregard proper knowledge of their profession in a cavalier way, lacking basic reading comprehension, due diligence and sanity.

lolphp876 replied Feb 18, 2016

It is fundamentally broken if you claim to implement an specific algorithm in a "non-standard" way that produces different of output. If you want to implement a different algorithm, call it by another name. This is why the PHP development team isn't taken seriously; they disregard proper knowledge of their profession in a cavalier way, lacking basic reading comprehension, due diligence and sanity.

@salathe

This comment has been minimized.

Show comment
Hide comment
@salathe

salathe Feb 18, 2016

Contributor

It is broken, since the values don't match what is in mt19937ar.c. That's not in question.
Changing the (mis)behaviour without discussion: that is what's questionable here.

Contributor

salathe replied Feb 18, 2016

It is broken, since the values don't match what is in mt19937ar.c. That's not in question.
Changing the (mis)behaviour without discussion: that is what's questionable here.

@laruence

This comment has been minimized.

Show comment
Hide comment
@laruence

laruence Feb 18, 2016

Member

@it @salathe the pull request has been there for monthes.. I don't understand why you don't give the idea then, now complaint it was committed without discussion? (with whom? with you?)

Member

laruence replied Feb 18, 2016

@it @salathe the pull request has been there for monthes.. I don't understand why you don't give the idea then, now complaint it was committed without discussion? (with whom? with you?)

@lt

This comment has been minimized.

Show comment
Hide comment
@lt

lt Feb 18, 2016

Contributor

@laruence I don't normally look through commits for breaking changes because normally they hit the mailing list. I just happened to see this yesterday.

It's a breaking change. It definitely does not belong in the 7.0 branch.

You may, as you state on the PR, find it weird that people might rely on a predictable stream of pseudo-random numbers, but they do have their uses.

Contributor

lt replied Feb 18, 2016

@laruence I don't normally look through commits for breaking changes because normally they hit the mailing list. I just happened to see this yesterday.

It's a breaking change. It definitely does not belong in the 7.0 branch.

You may, as you state on the PR, find it weird that people might rely on a predictable stream of pseudo-random numbers, but they do have their uses.

@laruence

This comment has been minimized.

Show comment
Hide comment
@laruence

laruence Feb 18, 2016

Member

@it okey, thanks, then may ,as this is really typo, leave the fix in master, and add a note in changlog, it is much better then only reverting this, thanks

Member

laruence replied Feb 18, 2016

@it okey, thanks, then may ,as this is really typo, leave the fix in master, and add a note in changlog, it is much better then only reverting this, thanks

@salathe

This comment has been minimized.

Show comment
Hide comment
@salathe

salathe Feb 18, 2016

Contributor

@laruence I want to see this change (not the revert), don't get me wrong. With my documentation hat on, a heads up wouldn't go amiss.

As for why comment now? I don't watch the php-src PRs, so missed it there and only came now as it was linked in a place I do watch. Sorry if that's not good enough for you. That said, I did see the original commit (not the revert) email, and inwardly applauded the change.

Contributor

salathe replied Feb 18, 2016

@laruence I want to see this change (not the revert), don't get me wrong. With my documentation hat on, a heads up wouldn't go amiss.

As for why comment now? I don't watch the php-src PRs, so missed it there and only came now as it was linked in a place I do watch. Sorry if that's not good enough for you. That said, I did see the original commit (not the revert) email, and inwardly applauded the change.

@Hypfer

This comment has been minimized.

Show comment
Hide comment
@Hypfer

Hypfer Feb 18, 2016

@lt "You may, as you state on the PR, find it weird that people might rely on a predictable stream of pseudo-random numbers, but they do have their uses."

I'd like to know a use case in which someone would rely on that. Just out of curiosity.

Hypfer replied Feb 18, 2016

@lt "You may, as you state on the PR, find it weird that people might rely on a predictable stream of pseudo-random numbers, but they do have their uses."

I'd like to know a use case in which someone would rely on that. Just out of curiosity.

@Two9A

This comment has been minimized.

Show comment
Hide comment
@Two9A

Two9A Feb 18, 2016

I'd like to know a use case in which someone would rely on that. Just out of curiosity.

Maze generation is one example: if you want "levels" of maze, a simple way to do that is to seed the RNG with a particular number that generates a subjectively "easy" or "difficult" maze. The seeds then correspond to the level numbers, reducing storage of the level information by orders of magnitude.

Two9A replied Feb 18, 2016

I'd like to know a use case in which someone would rely on that. Just out of curiosity.

Maze generation is one example: if you want "levels" of maze, a simple way to do that is to seed the RNG with a particular number that generates a subjectively "easy" or "difficult" maze. The seeds then correspond to the level numbers, reducing storage of the level information by orders of magnitude.

@WolleTD

This comment has been minimized.

Show comment
Hide comment
@WolleTD

WolleTD Feb 18, 2016

In case your code needs a reproducable sequence of pseudo-random numbers, why not just hash your seed? If you need more numbers, hash the resulting hash (and so on). They will never change, not even when changing the environment/language/whatsoever.

Relying on the predictability of a pseudo-random number generator looks like bad style for me.

WolleTD replied Feb 18, 2016

In case your code needs a reproducable sequence of pseudo-random numbers, why not just hash your seed? If you need more numbers, hash the resulting hash (and so on). They will never change, not even when changing the environment/language/whatsoever.

Relying on the predictability of a pseudo-random number generator looks like bad style for me.

@koraa

This comment has been minimized.

Show comment
Hide comment
@koraa

koraa Feb 18, 2016

In case your code needs a reproducable sequence of pseudo-random numbers, why not just hash your seed? If you need more numbers, hash the resulting hash (and so on). They will never change, not even when changing the environment/language/whatsoever.

Wouldn't you then just write your own hash-based PRNG?
PRNGS are standardized, just as hash functions are. Why would the latter be less prone to typos than the former?

koraa replied Feb 18, 2016

In case your code needs a reproducable sequence of pseudo-random numbers, why not just hash your seed? If you need more numbers, hash the resulting hash (and so on). They will never change, not even when changing the environment/language/whatsoever.

Wouldn't you then just write your own hash-based PRNG?
PRNGS are standardized, just as hash functions are. Why would the latter be less prone to typos than the former?

@WolleTD

This comment has been minimized.

Show comment
Hide comment
@WolleTD

WolleTD Feb 18, 2016

@koraa You're right, you would.
But if PRNGs are standardized, shouldn't we get PHPs PRNG to that standard?
Ok, well, we should keep providing the old one, that's right.

WolleTD replied Feb 18, 2016

@koraa You're right, you would.
But if PRNGs are standardized, shouldn't we get PHPs PRNG to that standard?
Ok, well, we should keep providing the old one, that's right.

@Profpatsch

This comment has been minimized.

Show comment
Hide comment
@Profpatsch

Profpatsch Feb 18, 2016

This is pure internet gold. Don’t ever change, PHP team. We need some fun in our lives.

Profpatsch replied Feb 18, 2016

This is pure internet gold. Don’t ever change, PHP team. We need some fun in our lives.

@yetzt

This comment has been minimized.

Show comment
Hide comment
@yetzt

yetzt Feb 18, 2016

please leave it broken, so i can continue pissing on about how bad php is.

yetzt replied Feb 18, 2016

please leave it broken, so i can continue pissing on about how bad php is.

@gries

This comment has been minimized.

Show comment
Hide comment
@gries

gries Feb 18, 2016

Please don't use this procedure for the next security fixes if somebody comes up with an obscure use case that relies on a security bug. :D

Also:
The public api does not change,
the method is still producing the results as expected and as described in the documentation,
and
http://php.net/manual/en/function.mt-srand.php even mentions that its NOT SAFE to rely on the sequences.

gries replied Feb 18, 2016

Please don't use this procedure for the next security fixes if somebody comes up with an obscure use case that relies on a security bug. :D

Also:
The public api does not change,
the method is still producing the results as expected and as described in the documentation,
and
http://php.net/manual/en/function.mt-srand.php even mentions that its NOT SAFE to rely on the sequences.

@tehplague

This comment has been minimized.

Show comment
Hide comment
@tehplague

tehplague Feb 18, 2016

< irony >
This was by far the best idea ever - to further encourage people to count on the predictability of so-called "random numbers"
< /irony >

Feels like re-inventing register_globals because that's what people were used to.

tehplague replied Feb 18, 2016

< irony >
This was by far the best idea ever - to further encourage people to count on the predictability of so-called "random numbers"
< /irony >

Feels like re-inventing register_globals because that's what people were used to.

@hikari-no-yume

This comment has been minimized.

Show comment
Hide comment
@hikari-no-yume

hikari-no-yume Feb 18, 2016

Contributor

I've backported the mt_rand_value.phpt test so that we actually have tests checking what mt_rand outputs. However, the backported version tests that it always produces the wrong result:

a50c31d

Contributor

hikari-no-yume replied Feb 18, 2016

I've backported the mt_rand_value.phpt test so that we actually have tests checking what mt_rand outputs. However, the backported version tests that it always produces the wrong result:

a50c31d

@FSMaxB

This comment has been minimized.

Show comment
Hide comment
@FSMaxB

FSMaxB Feb 19, 2016

This discussion makes it seem like some people think that this is a random number generator for cryptographic purposes, but it's not. So stay calm.

FSMaxB replied Feb 19, 2016

This discussion makes it seem like some people think that this is a random number generator for cryptographic purposes, but it's not. So stay calm.

@KiNaudiz

This comment has been minimized.

Show comment
Hide comment
@KiNaudiz

KiNaudiz Feb 19, 2016

This is why I don't use PHP.

...

Well, it's part of the reason.

KiNaudiz replied Feb 19, 2016

This is why I don't use PHP.

...

Well, it's part of the reason.

@MercSec

This comment has been minimized.

Show comment
Hide comment
@MercSec

MercSec Feb 19, 2016

Andreas ist right and there no "other Channels".. it's fucked up.
"There are appropriate channels to discuss " : No... it's brocken. Thanks...

MercSec replied Feb 19, 2016

Andreas ist right and there no "other Channels".. it's fucked up.
"There are appropriate channels to discuss " : No... it's brocken. Thanks...

@torvitas

This comment has been minimized.

Show comment
Hide comment
@torvitas

torvitas Feb 19, 2016

Well, at least there are laughs all throughout germany now: http://blog.fefe.de/?ts=a83b3c21

torvitas replied Feb 19, 2016

Well, at least there are laughs all throughout germany now: http://blog.fefe.de/?ts=a83b3c21

@hikari-no-yume

This comment has been minimized.

Show comment
Hide comment
@hikari-no-yume

hikari-no-yume Feb 19, 2016

Contributor

Yes, it's a buggy implementation, but we can't fix this in a patch release, there's a risk of backwards-compatibility breakage. It doesn't really matter if it doesn't match Mersenne Twister to existing apps. They just care if it works predictably. If we suddenly fix it in a patch release, we might break existing apps.

Contributor

hikari-no-yume replied Feb 19, 2016

Yes, it's a buggy implementation, but we can't fix this in a patch release, there's a risk of backwards-compatibility breakage. It doesn't really matter if it doesn't match Mersenne Twister to existing apps. They just care if it works predictably. If we suddenly fix it in a patch release, we might break existing apps.

@PurHur

This comment has been minimized.

Show comment
Hide comment
@PurHur

PurHur Feb 19, 2016

You break shitty code... I dont think this is a problem if the other code will work even better.

PurHur replied Feb 19, 2016

You break shitty code... I dont think this is a problem if the other code will work even better.

@hikari-no-yume

This comment has been minimized.

Show comment
Hide comment
@hikari-no-yume

hikari-no-yume Feb 19, 2016

Contributor

Uh, what? Expecting the RNG to work predictably is reasonable behaviour.

Contributor

hikari-no-yume replied Feb 19, 2016

Uh, what? Expecting the RNG to work predictably is reasonable behaviour.

@mhotchen

This comment has been minimized.

Show comment
Hide comment
@mhotchen

mhotchen Feb 19, 2016

I just want to show my appreciation for the language maintainers here as a very heavy user of PHP and part of a team that maintains a huge amount of legacy code. I'm glad that I can continue to allow security and other non-BC changing updates to happen automatically and I can check the change logs only when doing major version updates. I need this level of stability and maturity in a language instead of having small things subtly break in code I've never read and don't understand and me wasting days wondering why, or weeks trying to rewrite, all while the business continues to suffer.

mhotchen replied Feb 19, 2016

I just want to show my appreciation for the language maintainers here as a very heavy user of PHP and part of a team that maintains a huge amount of legacy code. I'm glad that I can continue to allow security and other non-BC changing updates to happen automatically and I can check the change logs only when doing major version updates. I need this level of stability and maturity in a language instead of having small things subtly break in code I've never read and don't understand and me wasting days wondering why, or weeks trying to rewrite, all while the business continues to suffer.

@PurHur

This comment has been minimized.

Show comment
Hide comment
@PurHur

PurHur Feb 19, 2016

I expect diffrent values...

PurHur replied Feb 19, 2016

I expect diffrent values...

@dequis

This comment has been minimized.

Show comment
Hide comment
@dequis

dequis Feb 19, 2016

I see some people in this thread don't even understand the difference between a CSPRNG and mersenne twister and are just here to repeat that meme that "php is bad" without even understanding what the issue really is about.

Hint: it's not security sensitive, and the reverted fix didn't make the generated sequences any less predictable. That's not even relevant.

dequis replied Feb 19, 2016

I see some people in this thread don't even understand the difference between a CSPRNG and mersenne twister and are just here to repeat that meme that "php is bad" without even understanding what the issue really is about.

Hint: it's not security sensitive, and the reverted fix didn't make the generated sequences any less predictable. That's not even relevant.

@m0yellow

This comment has been minimized.

Show comment
Hide comment
@m0yellow

m0yellow Feb 19, 2016

It could break legacy code.
I would rather say show that this kind of code really exists. code search, guides, whatever. This would be such a legacy code that might even break when changing platforms 32->64bit.
Otherwise, thanks for maintaining such a large codebase, and just fix it right by applying the patch.

m0yellow replied Feb 19, 2016

It could break legacy code.
I would rather say show that this kind of code really exists. code search, guides, whatever. This would be such a legacy code that might even break when changing platforms 32->64bit.
Otherwise, thanks for maintaining such a large codebase, and just fix it right by applying the patch.

@hikari-no-yume

This comment has been minimized.

Show comment
Hide comment
@hikari-no-yume

hikari-no-yume Feb 19, 2016

Contributor

Well, we don't know. Most code isn't open-source.

Also, another possible thing that will break is unit tests.

Though using GitHub and Open Hub Code Search, I couldn't find a single non-phpt usage of mt_srand. I could find usages of srand, but they all seem to use microtime.

Contributor

hikari-no-yume replied Feb 19, 2016

Well, we don't know. Most code isn't open-source.

Also, another possible thing that will break is unit tests.

Though using GitHub and Open Hub Code Search, I couldn't find a single non-phpt usage of mt_srand. I could find usages of srand, but they all seem to use microtime.

@yohgaki

This comment has been minimized.

Show comment
Hide comment
@yohgaki

yohgaki Feb 20, 2016

Contributor

MT rand should be "MT rand" otherwise it is not MT rand simply.
Anyway, I sent mail to internals. Let's discuss what we should do for released versions there.

Contributor

yohgaki replied Feb 20, 2016

MT rand should be "MT rand" otherwise it is not MT rand simply.
Anyway, I sent mail to internals. Let's discuss what we should do for released versions there.

@divinity76

This comment has been minimized.

Show comment
Hide comment
@divinity76

divinity76 Apr 14, 2016

at least, remember to fix it in 7.1.

inb4 "php_mt_rand()" polyfill

divinity76 replied Apr 14, 2016

at least, remember to fix it in 7.1.

inb4 "php_mt_rand()" polyfill

@DarkMukke

This comment has been minimized.

Show comment
Hide comment
@DarkMukke

DarkMukke May 10, 2016

If you are "depending" (quoting the original comment here) on 'random', you are doing it wrong. Random should be unexpected, therefore undependable.

DarkMukke replied May 10, 2016

If you are "depending" (quoting the original comment here) on 'random', you are doing it wrong. Random should be unexpected, therefore undependable.

@hikari-no-yume

This comment has been minimized.

Show comment
Hide comment
@hikari-no-yume

hikari-no-yume May 10, 2016

Contributor

For backwards compatibility purposes, generally what matters isn't whether you ought to depend on something, but whether existing code actually does.

Contributor

hikari-no-yume replied May 10, 2016

For backwards compatibility purposes, generally what matters isn't whether you ought to depend on something, but whether existing code actually does.

@DarkMukke

This comment has been minimized.

Show comment
Hide comment
@DarkMukke

DarkMukke May 10, 2016

But how much do you want to keep functionality just for BC and sacrifice fundamental fixes.
Its it worth not making progress in a better code base, because it might break your ancient code ?
Just add it to the list http://php.net/manual/en/migration70.incompatible.php

DarkMukke replied May 10, 2016

But how much do you want to keep functionality just for BC and sacrifice fundamental fixes.
Its it worth not making progress in a better code base, because it might break your ancient code ?
Just add it to the list http://php.net/manual/en/migration70.incompatible.php

@yohgaki

This comment has been minimized.

Show comment
Hide comment
@yohgaki

yohgaki May 10, 2016

Contributor

Randomness of PHP mt_rand() is verified and it's supposed to be the same quality as original one. Games need the same random sequence on occasions, for example. If we are going to "fix" this, we should have INI switch for this. BTW I'm +1 for fixing this.

Contributor

yohgaki replied May 10, 2016

Randomness of PHP mt_rand() is verified and it's supposed to be the same quality as original one. Games need the same random sequence on occasions, for example. If we are going to "fix" this, we should have INI switch for this. BTW I'm +1 for fixing this.

@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic May 10, 2016

Member

For the record, someone is already working on an RFC which will fix this issue, as well as a slew of other problems with the non-cryptographic PRNGs in PHP.

Member

nikic replied May 10, 2016

For the record, someone is already working on an RFC which will fix this issue, as well as a slew of other problems with the non-cryptographic PRNGs in PHP.

@hwkns

This comment has been minimized.

Show comment
Hide comment
@hwkns

hwkns replied May 19, 2016

There are probably children out there holding down spacebar to stay warm in the winter!  YOUR UPDATE MURDERS CHILDREN.

@tom--

This comment has been minimized.

Show comment
Hide comment
@tom--

tom-- Jun 20, 2016

I added side-by-side BigCrush test results for MT19937 and mt_rand() to this gist.

tom-- replied Jun 20, 2016

I added side-by-side BigCrush test results for MT19937 and mt_rand() to this gist.

Please sign in to comment.