Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MT_RAND_PHP causes undefined behavior #9191

Closed
zeriyoshi opened this issue Jul 29, 2022 · 1 comment · Fixed by #9197
Closed

MT_RAND_PHP causes undefined behavior #9191

zeriyoshi opened this issue Jul 29, 2022 · 1 comment · Fixed by #9197

Comments

@zeriyoshi
Copy link
Contributor

Description

The scaling algorithm used internally by MT_RAND_PHP causes undefined CPU-dependent behavior.

This breaks compatibility of the MT random number sequences generated across platforms. (Its properties as a random number are already broken because it is improperly used.)

The following code:

mt_srand(1234, MT_RAND_PHP);
echo mt_rand(PHP_INT_MIN, PHP_INT_MAX) . PHP_EOL;

Resulted in this output:

  • i386: 0
  • amd64: 0
  • arm32v7: -1
  • arm64v8: -1
  • s390x: -1

Easily reproduced in QEMU's available Docker environment:

$ docker run --rm -it i386/php:7.4-cli -r 'mt_srand(1234, MT_RAND_PHP); echo mt_rand(PHP_INT_MIN, PHP_INT_MAX) . PHP_EOL;'
0
$ docker run --rm -it amd64/php:7.4-cli -r 'mt_srand(1234, MT_RAND_PHP); echo mt_rand(PHP_INT_MIN, PHP_INT_MAX) . PHP_EOL;'
0
$ docker run --rm -it arm32v7/php:7.4-cli -r 'mt_srand(1234, MT_RAND_PHP); echo mt_rand(PHP_INT_MIN, PHP_INT_MAX) . PHP_EOL;'
-1
$ docker run --rm -it arm64v8/php:7.4-cli -r 'mt_srand(1234, MT_RAND_PHP); echo mt_rand(PHP_INT_MIN, PHP_INT_MAX) . PHP_EOL;'
-1
$ docker run --rm -it s390x/php:7.4-cli -r 'mt_srand(1234, MT_RAND_PHP); echo mt_rand(PHP_INT_MIN, PHP_INT_MAX) . PHP_EOL;'
-1

But I expected this output instead:

This should probably be an error since it is beyond the range of getrandmax(), but should be unified to 0 or -1 for compatibility.

PHP Version

PHP 7.4.x (All supported version)

Operating System

Debian 11

@zeriyoshi
Copy link
Contributor Author

zeriyoshi commented Jul 29, 2022

We can unify the behavior with a wicked implementation like this:

static inline zend_long float_to_long_intel(zend_long min, double f)
{
	if (f >= ((double) ZEND_LONG_MAX - 1.0)) {
		return 0;
	}

	return min + (zend_long) f;
}

TimWolla added a commit to TimWolla/php-src that referenced this issue Jul 29, 2022
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 phpGH-9190
Fixes phpGH-9191
TimWolla added a commit to TimWolla/php-src that referenced this issue Aug 3, 2022
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 phpGH-9190
Fixes phpGH-9191
TimWolla added a commit that referenced this issue Aug 3, 2022
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants