Skip to content

srand(0) set random seed to 0 instead random #10292

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

Closed
adic3x opened this issue Jan 11, 2023 · 15 comments · Fixed by #10380
Closed

srand(0) set random seed to 0 instead random #10292

adic3x opened this issue Jan 11, 2023 · 15 comments · Fixed by #10380

Comments

@adic3x
Copy link

adic3x commented Jan 11, 2023

Description

The following code:

<?php
function foo(): int
{
	srand(0);
	$bar = rand(0, 1023);
	var_dump($bar);
	return $bar;
}

var_dump(foo() === foo());

Resulted in this output:

int(684)
int(684)
bool(true)

But I expected this output instead:

int(23)
int(756)
bool(false)

But this code:

<?php
function foo(): int
{
	srand();
	$bar = rand(0, 1023);
	var_dump($bar);
	return $bar;
}

var_dump(foo() === foo());

Works fine:

int(722)
int(132)
bool(false)

https://www.php.net/manual/en/function.srand.php

Seeds the random number generator with seed or with a random value if seed is 0.

It's seems like srand(0) process 0 as new seed value instead 0 must reset seed to new random value.

PHP Version

PHP 8.2.0

Operating System

Win 11 build 25272 (insider)

@hormus
Copy link

hormus commented Jan 11, 2023

<?php

function foo()
{
	srand(0);
	$bar = rand(0, 1023);
	return $bar;
}

var_dump(foo() === foo());
?>

From php >= 4.3.0 expected result bool true
mt_srand(0) From php >= 7.1.0 expected result bool true
https://3v4l.org/JNHF8

@damianwadley
Copy link
Member

The srand page is a bit weird because it seems to try to document a function that doesn't exist anymore: it's mt_srand now, and that does not do the "with a random value if seed is 0" behavior. And given that 3v4l, it seems the original rand didn't actually do this?

Either way this seems like a documentation issue.

@damianwadley damianwadley transferred this issue from php/php-src Jan 11, 2023
@cmb69
Copy link
Member

cmb69 commented Jan 11, 2023

The srand page is a bit weird because it seems to try to document a function that doesn't exist anymore: it's mt_srand now

The docs mention that srand() is an alias of mt_srand() as of PHP 7.1.0, and since the docs are currently supposed to document PHP 7.0 - 8.2, that looks okay.

Anyhow, the problem is that the stubs are not matching the implementation; the stubs claim the default of $seed would be zero:

function mt_srand(int $seed = 0, int $mode = MT_RAND_MT19937): void {}

However, actually the implementation just checks whether an argument has been passed, or not:

if (ZEND_NUM_ARGS() == 0) {

So there is no default value.

So back to php-src. Documentation should be fixed afterwards.

@cmb69 cmb69 transferred this issue from php/doc-en Jan 11, 2023
@cmb69
Copy link
Member

cmb69 commented Jan 11, 2023

The big question is how to solve this: fix the stubs (resulting in a painful signature), or fix the implementation (resulting in a BC break). @kocsismate, thoughts?

@cmb69
Copy link
Member

cmb69 commented Jan 11, 2023

Ah, and since this is now part of ext/random, maybe @zeriyoshi and @TimWolla have thoughts on this as well.

@TimWolla
Copy link
Member

I do have thoughts on this: The only good reason to use mt_rand() and mt_srand() in the first place since PHP 7.0 is getting a reproducible sequence, thus changing the long-standing behavior of 0 is a no-go in my book, especially since this would either also affect the OOP Mt19937 engine or result in differences between Mt19937 and mt_rand, which directly goes against the intended behavior.

I suggest to change the actual default value to null, like Mt19937's default is null as well and make null equivalent to a random uint32_t.

@adic3x
Copy link
Author

adic3x commented Jan 11, 2023

https://www.php.net/manual/en/function.mt-srand.php

Seeds the random number generator with seed or with a random value if no seed is given.

Ok, but it factually contradicts to signature of this function:

mt_srand(int $seed = 0, int $mode = MT_RAND_MT19937): void

Where default value of $seed is 0. And I expect mt_srand(); and mt_srand(0); must works identically. For example, if I declare some function foo(int $ i = 0) then foo(); and foo(0); works identically.

As for me

srand(?int $seed = null, int $mode = MT_RAND_MT19937): void

looks more intuitive. For example substr takes int or null:

substr(string $string, int $offset, ?int $length = null): string

@adic3x
Copy link
Author

adic3x commented Jan 11, 2023

Also:

function foo(): int
{
	srand(mode: MT_RAND_MT19937);
	$bar = rand(0, 1023);
	var_dump($bar);
	return $bar;
}

var_dump(foo() === foo());
int(684)
int(684)
bool(true)

Also reproduces this bug, although it looks like I didn't pass $seed as argument. I understand why this is happening, I just think that ?int $seed = null will solve this problem.

@hormus
Copy link

hormus commented Jan 11, 2023

Seeds the random number generator with seed or with a random value if no seed is given.
PHP >= 7.1.0

mt_srand(): void
// default MT_RAND_MT19937
mt_srand(int $seed = 32767, int $mode = MT_RAND_MY19937): void
// random number generator with seed explicit "32767" end default mode MT_RAND_MT19937 or mode MT_RAND_PHP

Note* MT_RAND_PHP This mode is available for backward compatibility.
The signature without arguments is also valid, i see a documentation problem from historical php 7.1.0
MT_RAND_MT19937 value 0 for now.
If seed type integer and use function *rand return value is integer from seed for optionally range between min and max of function *rand

function foo(): int
{
	srand(seed: MT_RAND_MT19937);
	$bar = rand(0, 1023);
	var_dump($bar);
	return $bar;
}

var_dump(foo() === foo()); // Range Between 0 and 1023 with seed 0 is return integer value 684

@cmb69
Copy link
Member

cmb69 commented Jan 12, 2023

I suggest to change the actual default value to null, like Mt19937's default is null as well and make null equivalent to a random uint32_t.

I think this is indeed be the best way forward, although I'm slightly concerned regarding coercive typing; prior to PHP 8.1.0, one could pass null as $seed which would be coerced to zero. However, the alternative of changing the default to UNKNOWN would prevent any automatic seeding when the $mode parameter would be used.

@adic3x
Copy link
Author

adic3x commented Jan 12, 2023

mt_srand(?int $seed = null, int $mode = MT_RAND_MT19937): void

srand(int $seed = 0, int $mode = MT_RAND_MT19937): void
{
    mt_srand(($seed === 0) ? null : $seed, $mode);
}

Everything should work as written in the documentation.

Now we have some secret/hidden/undocumented function overloading feature over default arguments feature.

@cmb69
Copy link
Member

cmb69 commented Jan 12, 2023

srand() is an alias of mt_srand() as of PHP 7.1.0. As such, both functions have the same behavior anyway.

Everything should work as written in the documentation.

Unless the documentation is in error.

@adic3x
Copy link
Author

adic3x commented Jan 12, 2023

Unless the documentation is in error.

Oh, I thought the documentation was correct and this bug appeared version update.

@Girgias
Copy link
Member

Girgias commented Jan 12, 2023

Everything should work as written in the documentation.

No the documentation should reflect the actual implementation in this case. So the issue is more related to the docs lying currently.

@kocsismate
Copy link
Member

kocsismate commented Jan 19, 2023

I think this is indeed be the best way forward, although I'm slightly concerned regarding coercive typing; prior to PHP 8.1.0, one could pass null as $seed which would be coerced to zero. However, the alternative of changing the default to UNKNOWN would prevent any automatic seeding when the $mode parameter would be used.

I agree with making the default value null, but I think we should only fix this in master due to the BC break, shouldn't we? The BC break is very unfortunate, but we changed quite many similar signatures in the past, so this shouldn't be too big of an issue, especially because the documentation clearly states that the parameter is an int right now, and the description is correct about when the automatic seed is applied (when no parameter is given).

kocsismate added a commit that referenced this issue Jan 24, 2023
- PHP-8.1:
  Fix GH-10292 1st param of mt_srand() has UNKNOWN default on PHP <8.3
kocsismate added a commit that referenced this issue Jan 24, 2023
- PHP-8.1:
  Fix GH-10292 1st param of mt_srand() has UNKNOWN default on PHP <8.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
@TimWolla @cmb69 @kocsismate @Girgias @adic3x @damianwadley @hormus and others