Fixed bug #75170

This change may result in different mt_rand/rand sequences being
generated on 64-bit systems for a specific seed.

See also https://externals.io/message/100229.

diff --git a/NEWS b/NEWS
index 311f95fc8237cc0b2bc9c15b9ad133195d46185e..1a153dde1154d681e8a51133ab55df694d218573 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -30,6 +30,9 @@ PHP                                                                        NEWS
   . Fixed bug #75155 (AppendIterator::append() is broken when appending another
     AppendIterator). (Nikita)
 
+- Standard:
+  . Fixed bug #75170 (mt_rand() bias on 64-bit machines). (Nikita)
+
 - ZIP:
   . Fixed bug #75143 (new method setEncryptionName() seems not to exist
     in ZipArchive). (Anatol)

diff --git a/UPGRADING b/UPGRADING
index d35d1b4cd65592d74633a489a63b11b85ae54c06..c8b4ef3d68ccf7c664c945f64f267db566245923 100644 (file)
--- a/UPGRADING
+++ b/UPGRADING
@@ -94,6 +94,10 @@ PHP 7.2 UPGRADE NOTES
       sessions. Use output buffer just like web applications to resolve problems on
       CLI scripts.
 
+- Standard:
+  . Sequences generated by mt_rand() and rand() for a specific seed may differ
+    from PHP 7.1 on 64-bit machines. This change was necessary to resolve a
+    modulo bias bug in the implementation.
 
 ========================================
 2. New Features

diff --git a/ext/standard/mt_rand.c b/ext/standard/mt_rand.c
index 0594ab24e139ccc9cc8f44070b9513fbf0e9c926..2335a92a1a044394f692f1820efd65bdf31eb51d 100644 (file)
--- a/ext/standard/mt_rand.c
+++ b/ext/standard/mt_rand.c
@@ -212,50 +212,81 @@ PHP_FUNCTION(mt_srand)
 }
 /* }}} */
 
-/* {{{ php_mt_rand_range
- */
-PHPAPI zend_long php_mt_rand_range(zend_long min, zend_long max)
-{
-       zend_ulong umax = max - min;
-       zend_ulong limit;
-       zend_ulong result;
+static uint32_t rand_range32(uint32_t umax) {
+       uint32_t result, limit;
 
        result = php_mt_rand();
-#if ZEND_ULONG_MAX > UINT32_MAX
-       if (umax > UINT32_MAX) {
-               result = (result << 32) | php_mt_rand();
-       }
-#endif
 
        /* Special case where no modulus is required */
-       if (UNEXPECTED(umax == ZEND_ULONG_MAX)) {
-               return (zend_long)result;
+       if (UNEXPECTED(umax == UINT32_MAX)) {
+               return result;
        }
 
        /* Increment the max so the range is inclusive of max */
        umax++;
 
        /* Powers of two are not biased */
-       if (EXPECTED((umax & (umax - 1)) != 0)) {
-               /* Ceiling under which ZEND_LONG_MAX % max == 0 */
-               limit = ZEND_ULONG_MAX - (ZEND_ULONG_MAX % umax) - 1;
+       if ((umax & (umax - 1)) == 0) {
+               return result & (umax - 1);
+       }
+
+       /* Ceiling under which UINT32_MAX % max == 0 */
+       limit = UINT32_MAX - (UINT32_MAX % umax) - 1;
+
+       /* Discard numbers over the limit to avoid modulo bias */
+       while (UNEXPECTED(result > limit)) {
+               result = php_mt_rand();
+       }
+
+       return result % umax;
+}
 
-               /* Discard numbers over the limit to avoid modulo bias */
-               while (UNEXPECTED(result > limit)) {
 #if ZEND_ULONG_MAX > UINT32_MAX
-                       if (umax > UINT32_MAX) {
-                               result = (result << 32) | php_mt_rand();
-                       }
-                       else {
-                               result = php_mt_rand();
-                       }
-#else
-                       result = php_mt_rand();
+static uint64_t rand_range64(uint64_t umax) {
+       uint64_t result, limit;
+       
+       result = php_mt_rand();
+       result = (result << 32) | php_mt_rand();
+
+       /* Special case where no modulus is required */
+       if (UNEXPECTED(umax == UINT64_MAX)) {
+               return result;
+       }
+
+       /* Increment the max so the range is inclusive of max */
+       umax++;
+
+       /* Powers of two are not biased */
+       if ((umax & (umax - 1)) == 0) {
+               return result & (umax - 1);
+       }
+
+       /* Ceiling under which UINT64_MAX % max == 0 */
+       limit = UINT64_MAX - (UINT64_MAX % umax) - 1;
+
+       /* Discard numbers over the limit to avoid modulo bias */
+       while (UNEXPECTED(result > limit)) {
+               result = php_mt_rand();
+               result = (result << 32) | php_mt_rand();
+       }
+
+       return result % umax;
+}
 #endif
-               }
+
+/* {{{ php_mt_rand_range
+ */
+PHPAPI zend_long php_mt_rand_range(zend_long min, zend_long max)
+{
+       zend_ulong umax = max - min;
+
+#if ZEND_ULONG_MAX > UINT32_MAX
+       if (umax > UINT32_MAX) {
+               return (zend_long) (rand_range64(umax) + min);
        }
+#endif
 
-       return (zend_long)((result % umax) + min);
+       return (zend_long) (rand_range32(umax) + min);
 }
 /* }}} */
 

diff --git a/ext/standard/tests/math/bug75170.phpt b/ext/standard/tests/math/bug75170.phpt
new file mode 100644 (file)
index 0000000..a9aab06
--- /dev/null
+++ b/ext/standard/tests/math/bug75170.phpt
@@ -0,0 +1,32 @@
+--TEST--
+Bug #75170: mt_rand() bias on 64-bit machines
+--CREDITS--
+Solar Designer in https://externals.io/message/100229
+--FILE--
+<?php
+
+// PHP pre-7.1.0 modulo bias
+mt_srand(1234567890);
+$total = 10000;
+$max = 0x66666666;
+$halves[0] = $halves[1] = 0;
+for ($i = 0; $i < $total; $i++) {
+    $halves[(mt_rand(0, $max - 1) >> 1) & 1]++;
+}
+printf("%.1f%% vs. %.1f%%\n", 100. * $halves[0] / $total, 100. * $halves[1] / $total);
+
+// PHP 7.1.0 to 7.2.0beta2 modulo bias bug found during work
+// on http://www.openwall.com/php_mt_seed/
+mt_srand(1234567890);
+$total = 10000;
+$max = 0x66666666;
+$halves[0] = $halves[1] = 0;
+for ($i = 0; $i < $total; $i++) {
+    $halves[mt_rand(0, $max - 1) / ($max / 2)]++;
+}
+printf("%.1f%% vs. %.1f%%\n", 100. * $halves[0] / $total, 100. * $halves[1] / $total);
+
+?>
+--EXPECT--
+49.5% vs. 50.5%
+50.5% vs. 49.5%