Re: Use decent entropy for uniqid($prefix, TRUE)

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