Make WordPress Core

Opened 2 years ago

Closed 21 months ago

Last modified 21 months ago

#55194 closed defect (bug) (fixed)

wp_rand( 0, 0) result in random integer

Reported by: mlajo's profile mlajo Owned by: audrasjb's profile 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 SergeyBiryukov)

wp_rand( 0, 0) result in random integer instead of 0

Attachments (4)

55194.diff (2.2 KB) - added by SergeyBiryukov 2 years ago.
55194.2.diff (3.4 KB) - added by acoulombe 23 months ago.
Capture d’écran 2022-06-07 à 00.55.25.png (388.7 KB) - added by audrasjb 21 months ago.
credits added manually :)
55194.3.diff (1.4 KB) - added by SergeyBiryukov 21 months ago.

Download all attachments as: .zip

Change History (36)

#1 follow-up: @azouamauriac
2 years ago

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)

Last edited 2 years ago by azouamauriac (previous) (diff)

#2 @swissspidy
2 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: @SergeyBiryukov
2 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 @SergeyBiryukov
2 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 @tobifjellner
2 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: @mlajo
2 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 @SergeyBiryukov
2 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: @mlajo
2 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.

@SergeyBiryukov
2 years ago

#9 in reply to: ↑ 8 ; follow-ups: @SergeyBiryukov
2 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 @azouamauriac
2 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 @mlajo
2 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 @acoulombe
23 months 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 ?

@acoulombe
23 months ago

This ticket was mentioned in PR #2658 on WordPress/wordpress-develop by costdev.


22 months ago
#13

  • Keywords has-unit-tests added

#14 @costdev
22 months ago

  • Keywords commit added

#15 @peterwilsoncc
22 months 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 @audrasjb
22 months 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 @peterwilsoncc
22 months 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 @audrasjb
22 months 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 @marekdedic
22 months 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 @marekdedic
22 months ago

Hi,
we have updated the plugin to no longer depend on this behaviour.

Thanks for the heads up

#21 @audrasjb
22 months 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 @costdev
22 months 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 @audrasjb
21 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Self assigning for final review and commit.

#24 follow-up: @audrasjb
21 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 53473:

General: Ensure wp_rand() returns 0 when $min and $max values are equal to 0.

This changeset ensures wp_rand() returns zero instead of a random number when both $min and $max values are equal to zero.

Fixes #55194.

#25 @audrasjb
21 months 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.

@audrasjb
21 months ago

credits added manually :)

#27 @SergeyBiryukov
21 months ago

In 53477:

Tests: Use more descriptive names for wp_rand() test methods.

This aims to clarify the intention of the tests.

Follow-up to [53473].

Props mlajo, costdev, acoulombe, azouamauriac, swissspidy, tobifjellner, peterwilsoncc, audrasjb, marekdedic, SergeyBiryukov.
See #55194.

#28 @SergeyBiryukov
21 months 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.

#29 @SergeyBiryukov
21 months ago

In 53479:

Tests: Correct an assertion in wp_rand() tests.

The function returns non-negative integers, which includes zero.

Follow-up to [53473], [53477].

See #55194.

#30 @SergeyBiryukov
21 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

#31 in reply to: ↑ 24 @SergeyBiryukov
21 months ago

Replying to audrasjb:

In 53473:

General: Ensure wp_rand() returns 0 when $min and $max values are equal to 0.

This changeset ensures wp_rand() returns zero instead of a random number when both $min and $max values are equal to zero.

Fixes #55194.

It might be helpful to mention in the dev note that this brings parity with the random_int() and mt_rand() native PHP functions.

This ticket was mentioned in Slack in #core by sergey. View the logs.


21 months ago

Note: See TracTickets for help on using tickets.