Fix undefined behaviour in GENERATE_SEED()

[ndossche]

Signed multiply overflow is undefined behaviour.
If you run the CI tests with UBSAN enabled on a 32-bit platform, this is quite easy to hit. On 64-bit it's more difficult to hit though, but not impossible.

[TimWolla]

The change looks correct to me, but where are you still hitting GENERATE_SEED()? I believe everything should attempt to use the CSPRNG first and the CSPRNG is effectively infallible on modern systems.

[ndossche]

In gmp_init_random() there's a GENERATE_SEED():

php-src/ext/gmp/gmp.c

Lines 1717 to 1727 in 93e0f6b

	static void gmp_init_random(void)
	{
	if (!GMPG(rand_initialized)) {
	/* Initialize */
	gmp_randinit_mt(GMPG(rand_state));
	/* Seed */
	gmp_randseed_ui(GMPG(rand_state), GENERATE_SEED());
	GMPG(rand_initialized) = 1;
	}
	}

[TimWolla]

LGTM. Be careful with the merge to PHP 8.2, as all the random stuff moved into the random extension.

[TimWolla]

Can you also:

Suggested change

	#define GENERATE_SEED() (((zend_long) ((zend_ulong) time(0) * (zend_ulong) getpid())) ^ ((zend_long) (1000000.0 * php_combined_lcg())))
	#define GENERATE_SEED() (((zend_long) ((zend_ulong) time(NULL) * (zend_ulong) getpid())) ^ ((zend_long) (1000000.0 * php_combined_lcg())))

while you're at it?

[TimWolla]

Suggested change

	#define GENERATE_SEED() (((zend_long) ((zend_ulong) time(0) * (zend_ulong) GetCurrentProcessId())) ^ ((zend_long) (1000000.0 * php_combined_lcg())))
	#define GENERATE_SEED() (((zend_long) ((zend_ulong) time(NULL) * (zend_ulong) GetCurrentProcessId())) ^ ((zend_long) (1000000.0 * php_combined_lcg())))

[ndossche]

Thanks for the review. Noted, I'll look at what moved and I'll apply your suggestion in the merge. I'll wait first until CI is green.

[TimWolla]

In gmp_init_random() there's a GENERATE_SEED():

Okay, then this should likely be augmented with something like php_random_bytes_silent(&seed, sizeof(unsigned long)) == FAILURE to only use GENERATE_SEED() if the CSPRNG fails. A similar pattern is already in use in other locations. Are you interested in creating such a follow-up (likely master-only)?

The manual will also need a warning that the GMP RNG is not cryptographically secure.

[ndossche]

Okay, then this should likely be augmented with something like php_random_bytes_silent(&seed, sizeof(unsigned long)) == FAILURE to only use GENERATE_SEED() if the CSPRNG fails. A similar pattern is already in use in other locations. Are you interested in creating such a follow-up (likely master-only)?

Sure I'll take a look after this gets merged.