Skip to content

Commit 60ace13

Browse files
authored
Fix undefined behavior of MT_RAND_PHP if range exceeds ZEND_LONG_MAX (#9197)
RAND_RANGE_BADSCALING() invokes undefined behavior when (max - min) > ZEND_LONG_MAX, because the intermediate `double` might not fit into `zend_long`. Fix this by inlining a fixed version of the macro into Mt19937's range() function. Fixing the macro itself cannot be done in the general case, because the types of the inputs are not known. Instead of replacing one possibly broken version with another possibly broken version, the macro is simply left as is and should be removed in a future version. The fix itself is simple: Instead of storing the "offset" in a `zend_long`, we use a `zend_ulong` which is capable of storing the resulting double by construction. With this fix the implementation of this broken scaling is effectively identical to the implementation of php_random_range from a data type perspective, making it easy to verify the correctness. It was further empirically verified that the broken macro and the fix return the same results for all possible values of `r` for several distinct pairs of (min, max). Fixes GH-9190 Fixes GH-9191
1 parent 3331832 commit 60ace13

File tree

2 files changed

+11
-3
lines changed

2 files changed

+11
-3
lines changed

NEWS

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ PHP NEWS
55
- Random:
66
. Fixed bug GH-9235 (non-existant $sequence parameter in stub for
77
PcgOneseq128XslRr64::__construct()). (timwolla)
8+
. Fixed bug GH-9190, GH-9191 (undefined behavior for MT_RAND_PHP when
9+
handling large ranges). (timwolla)
810
. Removed redundant RuntimeExceptions from Randomizer methods. The
911
exceptions thrown by the engines will be exposed directly. (timwolla)
1012
. Added extension specific Exceptions/Errors (RandomException, RandomError,

ext/random/engine_mt19937.c

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -168,11 +168,17 @@ static zend_long range(php_random_status *status, zend_long min, zend_long max)
168168
return php_random_range(&php_random_algo_mt19937, status, min, max);
169169
}
170170

171-
uint64_t r = php_random_algo_mt19937.generate(status) >> 1;
172171
/* Legacy mode deliberately not inside php_mt_rand_range()
173172
* to prevent other functions being affected */
174-
RAND_RANGE_BADSCALING(r, min, max, PHP_MT_RAND_MAX);
175-
return (zend_long) r;
173+
174+
uint64_t r = php_random_algo_mt19937.generate(status) >> 1;
175+
176+
/* This is an inlined version of the RAND_RANGE_BADSCALING macro that does not invoke UB when encountering
177+
* (max - min) > ZEND_LONG_MAX.
178+
*/
179+
zend_ulong offset = (double) ( (double) max - min + 1.0) * (r / (PHP_MT_RAND_MAX + 1.0));
180+
181+
return (zend_long) (offset + min);
176182
}
177183

178184
static bool serialize(php_random_status *status, HashTable *data)

0 commit comments

Comments
 (0)