2017-01-07 2:15 GMT+01:00 Yasuo Ohgaki <yohg...@ohgaki.net>:
> Hi Lauri,
>
> On Wed, Jan 4, 2017 at 4:56 AM, Lauri Kenttä <lauri.ken...@gmail.com>
> wrote:
>
> > On 2016-12-31 01:20, Yasuo Ohgaki wrote:
> >
> >> + 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);
> >
> > Also, your argument about PHPMailer has nothing to do with your main
> > complaint about lcg_value, since collisions of lcg_value are not the
> > problem there.
> >
>
> + php_random_int(1000000000, 9999999999, &rand, 1);
>
> This should be
>
> + php_random_int(0, 9999999999, &rand, 1);
>
> Anyway, I forgot about 32 bit systems. Thank you for catching this.
>
> My original patch is better, then,
>
> [yohgaki@dev PHP-master]$ git show 48f1a17886d874dc90867c669481804de90
> commit 48f1a17886d874dc90867c669481804de90509e8
> Author: Yasuo Ohgaki <yohg...@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.
>
>
> > Why don't you put your effort into a more useful solution such as
> > random_string or something?
> > random_string(PHP_STRING_HEX_LOWER, 32) would produce md5-style output.
> > random_string(PHP_STRING_BASE64, 32) would produce a lot more entropy.
> > random_string("my_charset", 20) would cover the general case.
> > random_array([1,2,3], 20) could extend this to arbitrary arrays.
> >
>
> They are useful. I would like to have such functions, too. However, they
> don't fix uniqid() problem. i.e. They are out of scope of uniqid()
> improvement. As I mentioned, we should get rid of useless goccha like
> uniqid() is merely a system timestamp. i.e. more_entropy option should be
> enabled by default. Additionally, we may provide stronger entropy such as
> 128 bits random value.
>
> Do you think uniqid() is good function that we should keep as it is now,
> forever? I guess you do not. Why not improve it
Because it's a BC break. A new function with the deprecation of the old one
is way easier for developers to handle. They don't need version checks.
Maybe it will work on older versions, but will behave differently.
Regards, Niklas
