Hi Lauri,
On Wed, Jan 4, 2017 at 4:56 AM, Lauri Kenttä <lauri.kentta@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 <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.
> 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
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
