Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:97589
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain giraffy.jp designates 49.212.134.110 as permitted sender)
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
Cc: Lauri =?iso-8859-1?Q?Kentt=E4?= <lauri.kentta@gmail.com>,
  "internals\@lists.php.net" <internals@lists.php.net>
In-Reply-To: <CAGa2bXaZPBYTaXzNDnA32BCBoZ_FzFUnRNYKFY8b1TXAWSwMyg@mail.gmail.com>
	(Yasuo Ohgaki's message of "Sat, 7 Jan 2017 10:15:09 +0900")
References: <CAGa2bXZ9EzWHDTBg_ZHGW4KdjiFK4tpE7aMCVTZD5n+UZ2vEWA@mail.gmail.com>
	<7500b0c6a50baf49beac70ae01e8b50d@koti.fimnet.fi>
	<CAGa2bXaZPBYTaXzNDnA32BCBoZ_FzFUnRNYKFY8b1TXAWSwMyg@mail.gmail.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
Date: Mon, 09 Jan 2017 03:15:30 +0900
Message-ID: <87o9zhju4d.fsf@lil.giraffy.jp>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PHP-DEV] Use decent entropy for uniqid($prefix, TRUE)
From: kazuo@o-ishi.jp (Kazuo Oishi)

Hi,

>>> +               zend_long rand;
>>> +               php_random_int(1000000000, 9999999999, &rand, 1);
>>> +               uniqid = strpprintf(0, "%s%08x%05x%.8F", prefix, sec,
>>> usec, (double)rand/10000000000);
>>
>> Your code is broken. It produces 0.10000000 - 0.99999999 when it should
>> produce 0.00000000 - 9.99999999. Also, you have integer overflow on 32-bit
>> systems.
>>
>> Why do you mess with oversized integers and doubles and at all? It would
>> be cleaner and simpler to use just regular 32-bit integers like this:
>>
>> +               zend_long rand;
>> +               php_random_int(0, 999999999, &rand, 1);
>> +               uniqid = strpprintf(0, "%s%08x%05x%d.%08d", prefix, sec,
>> usec, rand % 10, rand / 10);
...
> My original patch is better, then,
>
> [yohgaki@dev PHP-master]$ git show 48f1a17886d874dc90867c669481804de90
> commit 48f1a17886d874dc90867c669481804de90509e8
> Author: Yasuo Ohgaki <yohgaki@php.net>
> Date:   Tue Oct 18 09:04:57 2016 +0900
>
>     Fix bug #47890 #73215 uniqid() should use better random source
>
> diff --git a/ext/standard/uniqid.c b/ext/standard/uniqid.c
> index f429e6d..207cf01 100644
> --- a/ext/standard/uniqid.c
> +++ b/ext/standard/uniqid.c
> @@ -35,9 +35,11 @@
>  #include <sys/time.h>
>  #endif
>
> -#include "php_lcg.h"
> +#include "php_random.h"
>  #include "uniqid.h"
>
> +#define PHP_UNIQID_ENTROPY_LEN 10
> +
>  /* {{{ proto string uniqid([string prefix [, bool more_entropy]])
>     Generates a unique ID */
>  #ifdef HAVE_GETTIMEOFDAY
> @@ -77,7 +79,22 @@ PHP_FUNCTION(uniqid)
>          * digits for usecs.
>          */
>         if (more_entropy) {
> -               uniqid = strpprintf(0, "%s%08x%05x%.8F", prefix, sec, usec,
> php_combined_lcg() * 10);
> +               int i;
> +               unsigned char c, entropy[PHP_UNIQID_ENTROPY_LEN+1];
> +
> +               for(i = 0; i < PHP_UNIQID_ENTROPY_LEN;) {
> +                       php_random_bytes_throw(&c, sizeof(c));
> +                       /* Avoid modulo bias */
> +                       if (c > 249) {
> +                               continue;
> +                       }
> +                       entropy[i] = c % 10 + '0';
> +                       i++;
> +               }
> +               /* Set . for compatibility */
> +               entropy[1] = '.';
> +               entropy[PHP_UNIQID_ENTROPY_LEN] = '\0';
> +               uniqid = strpprintf(0, "%s%08x%05x%s", prefix, sec, usec,
> entropy);
>         } else {
>                 uniqid = strpprintf(0, "%s%08x%05x", prefix, sec, usec);
>         }
>
>
> I'll revert the revert commit to master.

No.  Lauri's version is better.

Your php_random_bytes_throw() version is significantly slow.  Lauri's
version is faster and cleaner.

[original uniqid() using php_combined_lcg]
$ time ./php_uniqid_orig -r 'for($i=0; $i<1000000;$i++) uniqid("",true);'
real    0m0.366s
user    0m0.350s
sys     0m0.010s

[your php_random_bytes_throw version (commit 48f1a17886d874dc90867c669481804de90509e8)]
$ time ./php_uniqid_yohgaki -r 'for($i=0; $i<1000000;$i++) uniqid("",true);'
real    0m4.509s
user    0m0.430s
sys     0m4.070s

[Lauri's php_random_int version]
$ time ./php_uniqid_lauri -r 'for($i=0; $i<1000000;$i++) uniqid("",true);'
real    0m0.664s
user    0m0.260s
sys     0m0.400s


-- 
Kazuo Oishi