#55194 closed defect (bug) (fixed)
wp_rand( 0, 0) result in random integer
Reported by: | mlajo | Owned by: | audrasjb |
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | 5.9 |
Component: | General | Keywords: | has-patch has-unit-tests needs-dev-note |
Focuses: | docs | Cc: |
Description (last modified by )
wp_rand( 0, 0) result in random integer instead of 0
Attachments (4)
Change History (36)
#2
@
3 years ago
Ryan is not really active anymore, so I doubt you'll get a response :-)
The function does not really support $max = 0
. $max = 0
means use the maximum upper bound, which is 4294967295.
So this works as expected.
And the function does not support negative values by design.
Perhaps the inline documentation could be improved a bit.
#3
in reply to:
↑ 1
;
follow-up:
↓ 6
@
3 years ago
- Description modified (diff)
- Focuses docs added
- Milestone changed from Awaiting Review to 6.0
Replying to azouamauriac:
I'm able to reproduce the issue, the function was introduce here [8728]. For what I understand it used to replace mt_rand RE https://www.php.net/manual/en/function.mt-rand.php.
but "why" i don't know.
You can find more details in the WordPress 2.6.2 release post.
Looks like the current behavior is intentional, see for example this line added in [34981] / #28633:
$_max = ( 0 != $max ) ? $max : $max_random_number;
This is also consistent with mt_rand()
, which returns a random integer and not 0 when parameters are omitted. So changing this behavior does not seem feasible due to security implications, but we can update the documentation to mention that the $max
value is only taken into account if it's greater than zero.
#4
@
3 years ago
Something like this I think would be correct:
* @param int $min Optional. Lower limit for the generated number. * Accepts positive integers or zero. Default 0. * @param int $max Optional. Upper limit for the generated number. * Accepts positive integers. Defaults to 4294967295.
#5
@
3 years ago
@SergeyBiryukov
It would probably be good also to explicitly say that 0 is treated as Max...
#6
in reply to:
↑ 3
;
follow-up:
↓ 7
@
3 years ago
Replying to SergeyBiryukov:
This is also consistent with
mt_rand()
, which returns a random integer and not 0 when parameters are omitted. So changing this behavior does not seem feasible due to security implications, but we can update the documentation to mention that the$max
value is only taken into account if it's greater than zero.
mt_rand( 0, 0 ) returns 0
#7
in reply to:
↑ 6
@
3 years ago
Replying to mlajo:
Replying to SergeyBiryukov:
This is also consistent with
mt_rand()
, which returns a random integer and not 0 when parameters are omitted.
mt_rand( 0, 0 ) returns 0
Indeed, thanks! I meant mt_rand()
without any parameters, in that case $max
defaults to mt_getrandmax()
.
I guess wp_rand()
too should have used null
as the default value for both parameters, so that we could differentiate between wp_rand()
and wp_rand( 0, 0 )
, which is not currently possible as zero is the default value. But it's too late to change that now without security implications.
#8
follow-up:
↓ 9
@
3 years ago
This is not consistent with mt_rand and contra intuitive. It caused lots of trouble in our code until we pinpoint it to this function. Also coding standard https://github.com/WordPress/WordPress-Coding-Standards/blob/develop/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php recommend to use it.
mt_rand() is discouraged. Use the far less predictable wp_rand() instead.
#9
in reply to:
↑ 8
;
follow-ups:
↓ 10
↓ 11
↓ 12
@
3 years ago
- Keywords has-patch added
Replying to mlajo:
This is not consistent with mt_rand and contra intuitive.
What I meant by consistency is that both mt_rand()
and wp_rand()
, when called without any parameters, return a random number. But you're right that the results for mt_rand( 0, 0 )
and wp_rand( 0, 0 )
are inconsistent.
On second thought, maybe we could still fix it, see 55194.diff. This would need a unit test and security review.
It caused lots of trouble in our code until we pinpoint it to this function.
Just out of curiousity, what would be the use case for a wp_rand( 0, 0 )
call?
#10
in reply to:
↑ 9
@
3 years ago
Replying to SergeyBiryukov:
Just out of curiousity, what would be the use case for a
wp_rand( 0, 0 )
call?
may be $min and $max are dynamiques variables, then it can happen that both of them get 0 as value, if that wp_rand will not given expected result... that's my mind... let's @mlajo enlighten us.
#11
in reply to:
↑ 9
@
3 years ago
Replying to SergeyBiryukov:
Just out of curiousity, what would be the use case for a
wp_rand( 0, 0 )
call?
something as @azouamauriac said, we have code with wp_rand( 0, $var ) where $var can sometimes be 0. Now we added one "if check" before to check if $var is 0 but it was confusing to debug because mt_rand() worked as expected.
If it stays like this it would be nice to put some kind of notice for that case in documentation.
#12
in reply to:
↑ 9
@
3 years ago
Replying to SergeyBiryukov:
On second thought, maybe we could still fix it, see 55194.diff. This would need a unit test and security review.
I'm working on writing tests based on your fix.
For my knowledge, i'm relatively new to contributing WordPress. How do you normally work, can I make a pull request according to your .diff or do you want to make one yourself ?
This ticket was mentioned in PR #2658 on WordPress/wordpress-develop by costdev.
3 years ago
#13
- Keywords has-unit-tests added
Trac ticket: https://core.trac.wordpress.org/ticket/55194
#15
@
3 years ago
- Milestone changed from 6.0 to 6.1
I'm moving this to the 6.1 milestone as it's technically a backward compatibility break, which doesn't seem great in the hours leading up to the RC. The change in behavior will also need a dev-note and there isn't time to put one together.
I did a search of the plugin repo and found an instance of a plugin using wp_rand(0, 0)
to generate a random key. I found one so prior to commit someone will need to get in touch with the author of the plugin.
#16
@
3 years ago
@peterwilsoncc I didn't find any plugin using this with wpdirectory search. Could you please provide a few insights about how you found them? Thanks!
#17
@
3 years ago
@audrasjb Sorry, I intended to link it but must have been rushing. This is the search I was using https://wpdirectory.net/search/01G37HMMWQRZ6QHH9CVMMSGP8R
#18
@
3 years ago
Ah thank you I didn't succeed to find the plugin, but my regular expression was wrong.
I'll try to reach out to the plugin author!
#19
@
3 years ago
Hi,
plugin author here, thanks for the heads up. I'll change it so that you can go forward with this change.
I am a bit stumped as to how this code even came to be - I was definitely counting on it being a random number and not 0... :/
#20
@
3 years ago
Hi,
we have updated the plugin to no longer depend on this behaviour.
Thanks for the heads up
#21
@
3 years ago
- Keywords needs-refresh added; commit removed
Great thank you @marekdedic!
I think we're good to ship this once the suggested change in @costdev’s PR is implemented :)
#22
@
3 years ago
- Keywords commit needs-dev-note added; needs-refresh removed
@audrasjb PR updated with version numbers as requested by Sergey. I believe that's all changes done.
Adding needs-dev-note
per comment 15.
#23
@
3 years ago
- Owner set to audrasjb
- Status changed from new to reviewing
Self assigning for final review and commit.
#24
follow-up:
↓ 31
@
3 years ago
- Resolution set to fixed
- Status changed from reviewing to closed
In 53473:
#25
@
3 years ago
- Keywords commit removed
Oops I missed the props
line in my commit, to credit the contributors listed in this ticket. I'll add them right now manually, using the tool provided by Make/Core P2 website. Sorry.
3 years ago
#26
Committed in https://core.trac.wordpress.org/changeset/53473
#28
@
3 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
It looks like the tests are not quite correct, this failure now happens occasionally:
1) Tests_Pluggable_wpRand::test_wp_rand_should_return_a_positive_integer with data set "-1 and 99" (-1, 99) The value was not greater than 0 Failed asserting that 0 is greater than 0. /var/www/tests/phpunit/tests/pluggable/wpRand.php:22 phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97
Looking at the test, it makes the assumption that the return value should always be greater than zero, due to using absint()
on the final result. However, since random_int() works with negative numbers too, given a range between -1 and 99, it can return zero as a valid result, which is then returned by wp_rand()
unchanged.
55194.3.diff corrects the affected assertion.
Hello welcome to WordPress trac! thanks for your report, you're right, I'm able to reproduce the issue, the function was introduce here [8728]. For what I understand it used to replace mt_rand RE https://www.php.net/manual/en/function.mt-rand.php.
but "why" i don't know.
pinging @ryan as he was author of commit, may be he can enlighten us.
And @SergeyBiryukov for milestone. I think the ticket should be milestone for 6.0 if it's accepted.
Also the function always returns abs value(e.g wp_rand( -1, -1) = 1 instead of -1)