From: Nikita Popov <nikita.ppv@gmail.com>
Date: Thu, 7 Sep 2017 18:03:23 +0000 (+0200)
Subject: Fixed bug #75170
X-Git-Tag: php-7.2.0RC2~32
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=fd07302024bc47082b13b32217147fd39d1e9e61;p=php

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 311f95fc82..1a153dde11 100644
--- 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 d35d1b4cd6..c8b4ef3d68 100644
--- 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 0594ab24e1..2335a92a1a 100644
--- 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
index 0000000000..a9aab06ef4
--- /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%