On Wed, Oct 19, 2016 at 8:01 AM, Anatol Belski <anatol....@belski.net> wrote: >> -----Original Message----- >> From: Yasuo Ohgaki [mailto:yohg...@ohgaki.net] >> Sent: Tuesday, October 18, 2016 9:53 PM >> To: Anatol Belski <anatol....@belski.net> >> Cc: Joe Watkins <pthre...@pthreads.org>; Niklas Keller <m...@kelunik.com>; >> Leigh <lei...@gmail.com>; PHP Internals <internals@lists.php.net> >> Subject: Re: [PHP-DEV] Re: [RFC][DISCUSSION] Improve uniqid() uniqueness >> >> Hi Anatol, >> >> On Wed, Oct 19, 2016 at 1:41 AM, Anatol Belski <anatol....@belski.net> wrote: >> > AFM the patch is not acceptable for 7.0. It is true that some place was >> > moved >> to the new random int functionality (in password AFAIR). But, it is done at >> the >> place and the way that a BC breach is unlikely. Using the throwing variant >> is for >> sure a BC breach, but also the way pushing while being explicitly asked to go >> through an RFC, is inappropriate. As the new random_* functions are available >> and allow to implement the best possible uniqueness in user land, changing >> the >> algorithm of the existing uniqid() doesn't look to have a valid base. >> > >> >> Any additional error could be BC. It's the fact. >> >> However, your sentence does not make sense at all. >> Do we revert any error emitting bug fix? No, not at all. >> > As far as I remember, uniqid() was never meant to be cryptographically safe. > It is documented. Indeed systems might be too fast for microseconds based > function nowadays. In 7.0, my simple exercise - substr(md5(random_bytes(8)), > 0, 13) which does same in the way you want it. We are talking about a > oneliner of new code vs. an old function that is guaranteed in use at any > possible places.
The original draft RFC proposal aimed to be cryptographically safe unique ID as much as it can, but the pushed patch is not. > >> We do add errors as normal bug fix process. Many of them are w/o RFC, even >> w/o discussion. >> >> Example: https://bugs.php.net/bug.php?id=73238 >> This bug fix caused WordPress caused 3 additional E_WARNING displayed that >> can be remove by php.ini or code fix. >> > As a reminder - there's no global rule about functions throwing exceptions, > so it is not done by default. Except a couple of new places, no function > throws an exception. The place in password salt code, that was migrated to > the new randomness, did already depend on /dev/urandom and others. However, > even it's based on the new functionality, the old behavior is kept and it is > done intentionally. I agree that this apply to cases such as rand(). We do had to keep rand() behavior even if it produces very bad random on Windows, as you know well. Replacing bad entropy that should be "really random" is different story. Current uniqid('', true)'s result is: unique id (time stamp) + entropy (timestamp based entropy) Isn't this a shame of us providing the result as "uniqid()" call? (I'm not saying original design is bad. Original design was inevitable due to technical limitation, historical reason, just like Windows rand()) Entropy is entropy. As long as format is kept, it does not matter if we use better entropy. > >> Which is important? >> - uniqid() is not unique >> - Really broken system that shouldn't be used may emit error >> >> "/dev/urandom cannot read discussion" is FUD and irrelevant to this >> discussion. >> Issues with user script random_bytes() implementation or like does not apply >> to >> uniqid() fix. >> > But your implementation indeed uses another API that has other impacts. > Php_random_bytes is crossplatform, there can be various errors on various > platforms. That's the concern as I'd understand it. > >> >> Anyway, are we going to revert anything emit new errors from now on because >> it's BC? >> Are we going to require RFC for this kind of very simple and reasonable fix? >> I hope not. >> >> IMHO my discussion is logical. Please consider revert the revert. >> Otherwise, we cannot fix even simple bugs. >> > No, IMHO you overdo it a bit. Of course it is acceptable with errors, > warning, etc. where it makes sense. But it needs a base and a balance also in > other areas for usability, performance, BC, language consistency, etc. If one > were telling, it's impossible to do it in PHP - but there are functions in > PHP 7, that provide the functionality aimed. Yes, there is also some legacy > functionality, so should everything be moved to cryptographically safe? The > answer is obviously - no. For crypto there are dedicated functions and > extensions there. Besides that, you see many other people opposing this > change. An RFC were the way to target the PHP version you want, even 7.0. As > for me, I'd likely vote yes for master, if the throwing part were replaced. I think you and Joe could not follow the discussion. It's okay, reading them all is waste of your time. I read all, but I'm not sure if I understand/remember all of them well. IMHO Oppositions for the patch is based on _wrong_ assumption that "new uniqid() causes common enough errors to be an issue". This wrong assumption is the reason why my commit became an issue, I presume. Could you reconsider decision based on _wrong_ assumption? Thank you. P.S. I'm a bit tired of uniqid() discussion because I expected this is easy one. This - unique id (time stamp) + entropy (timestamp based entropy) - is obviously wrong for today's PHP. I won't have time to write RFC for this, probably. I have many other things that I would like to improve, like session error status handling improvement that I recently proposed. -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
