Fix undefined behavior of MT_RAND_PHP if range exceeds ZEND_LONG_MAX

[TimWolla]

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

[zeriyoshi]

The implementation appears adequate. However, given the current situation where MT_RAND_PHP is left only for compatibility, it is difficult to determine if the resulting loss of compatibility is appropriate.

Nevertheless, we must also carefully consider whether we should align our results with Intel or others. Maybe I should conform to Intel's behavior considering the actual workload of PHP, but I can't determine that anyway...

[TimWolla]

However, given the current situation where MT_RAND_PHP is left only for compatibility, it is difficult to determine if the resulting loss of compatibility is appropriate.

This is undefined behavior. By definition there is no previous correct behavior. It can depend on compiler, version, flags, operating system, and architecture. In fact with my simple test script I get different results for both clang and gcc and for both -O0 and -O2:

#include<stdint.h>
#include<stdio.h>

typedef int64_t zend_long;
typedef uint64_t zend_ulong;

# define PHP_MT_RAND_MAX ((zend_long) (0x7FFFFFFF))
# define RAND_RANGE_BADSCALING(__n, __min, __max, __tmax) \
	(__n) = (__min) + (zend_long) ((double) ( (double) (__max) - (__min) + 1.0) * ((__n) / ((__tmax) + 1.0)))

int
main(void)
{
	zend_long min, max;
	zend_ulong r;

	min = INT64_MIN;
	max = INT64_MAX;
	r = PHP_MT_RAND_MAX - 1;

	RAND_RANGE_BADSCALING(r, min, max, PHP_MT_RAND_MAX);

	printf("%ld\n", (zend_long) r);
}

$ gcc -O2 -Wall test.c; ./a.out
-1
$ clang -O2 -Wall test.c; ./a.out
140724660442312
$ clang -O0 -Wall test.c; ./a.out
0
$ gcc -O0 -Wall test.c; ./a.out
0

… and all of the results are crap when taking into account that r is almost identical to PHP_MT_RAND_MAX (i.e. the result should be near max).

With the zend_ulong cast in the macro I reliably get:

$ gcc -O0 -Wall test.c; ./a.out
9223372019674906624
$ gcc -O2 -Wall test.c; ./a.out
9223372019674906624
$ clang -O2 -Wall test.c; ./a.out
9223372019674906624
$ clang -O0 -Wall test.c; ./a.out
9223372019674906624

which is also in the rough neighborhood of INT_MAX (difference is 17179869183).

This scaling function is also unable to generate all possible results in the general case, because doubles only have 53 bits of precision. So once the range is larger than 53 bits, some numbers will be impossible to generate.

[cmb69]

Thank you for the PR!

I wonder whether we should use zend_dval_to_lval() to convert the double to zend_long. This function is build exactly for this purpose.

And instead of inlining the macro, maybe we should introduce an inline function.

[TimWolla]

I wonder whether we should use zend_dval_to_lval() to convert the double to zend_long. This function is build exactly for this purpose.

I'd rather not. The change I made, changing the cast from zend_long to zend_ulong is a minimally invasive change. The bad scaling exists solely for compatibility reasons and it was easy-ish for me to verify that the change from signed to unsigned only affected the cases that were previously affected by UB. The reasoning is outlined in the commit message and PR description. The new construct also is very similar to php_random_range where an unsigned offset is calculated, then the (signed) minimum is added to it (which C will implicitly promote to unsigned) and then the result is casted to signed.

In other words: Compatibility trumps cleanliness in this case.

And instead of inlining the macro, maybe we should introduce an inline function.

If anything I would remove the macro entirely, as it is broken (as the name implies). I've left the macro in place as it is, to avoid breakage for third-party consumers.

[zeriyoshi]

This is undefined behavior. By definition there is no previous correct behavior. It can depend on compiler, version, flags, operating system, and architecture.

That is certainly true. Although most production environments probably use binaries compiled with gcc due to performance issues, it may not be necessary to maintain an implementation whose behavior changes with optimization options.

I confirm that compatibility is maintained with proper usage. I approve this change.

<?php

mt_srand(1234, MT_RAND_PHP);
for ($i = 0; $i < 65535; $i++) {
    if (($i % 1024) === 0) {
        mt_srand(mt_rand(), MT_RAND_PHP);
    }
    echo mt_rand(1024, 2048) . PHP_EOL;
}

PS: Should this change be merge as a patch for older versions in support? I personally think it should be merge.

[TimWolla]

PS: Should this change be merge as a patch for older versions in support? I personally think it should be merge.

I don't have a strong opinion on this one, but I'd lean to "no". Paging @cmb69 for his opinion and to have a look at my answers to his previous remarks.

[cmb69]

Should this change be merge as a patch for older versions in support? I personally think it should be merge.

As I understand it, this is only for the legacy mode. If that is correct, I wouldn't backport the fix.

[TimWolla]

As I understand it, this is only for the legacy mode.

@cmb69 Yes, you understand that correctly. This fix applies only to MT_RAND_PHP which is the non-standard broken Mersenne Twister.

As the backporting question is resolved now (no backporting): Does the PR look good to you? See also my answers in: #9197 (comment)

[cmb69]

LGTM. Thank you!