Skip to content
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.

Sign up for GitHub

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

[9.x] Use secure randomness in Arr:random and Arr:shuffle #46105

Merged
merged 8 commits into from
Feb 16, 2023
Merged

[9.x] Use secure randomness in Arr:random and Arr:shuffle #46105

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6e7534e
Use secure randomness in Arr:random and Arr:shuffle
valorin Feb 14, 2023
9f39ac6
Simplify random and shuffle algos
valorin Feb 14, 2023
a0d9d0a
Fix function name
valorin Feb 14, 2023
b223c3b
Prevent shuffle from preserving keys
valorin Feb 15, 2023
1474192
Fix algo to better implementation (and fix broken test)
valorin Feb 15, 2023
55b8d17
Fix increment style
valorin Feb 15, 2023
d074c06
Fix broken shuffle and make test useful
valorin Feb 16, 2023
63ddf4e
Keeping StyleCI happy
valorin Feb 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 15 additions & 20 deletions src/Illuminate/Collections/Arr.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,28 +635,14 @@ public static function random($array, $number = null, $preserveKeys = false)
}

if (is_null($number)) {
return $array[array_rand($array)];
return head(array_slice($array, random_int(0, $count - 1), 1));
}

if ((int) $number === 0) {
return [];
}

$keys = array_rand($array, $number);

$results = [];

if ($preserveKeys) {
foreach ((array) $keys as $key) {
$results[$key] = $array[$key];
}
} else {
foreach ((array) $keys as $key) {
$results[] = $array[$key];
}
}

return $results;
return array_slice(static::shuffle($array), 0, $number, $preserveKeys);
Copy link
Contributor

@vlakoff vlakoff Feb 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your current implementation of Arr::shuffle() changed from not preserving the keys, to preserving the keys. Changing it back to not preserving the keys, the above line in Arr::random() which makes use of Arr::shuffle() and array_slice() (with preserve_keys parameter) would no longer work, the code would need to be adjusted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my bad. I added in preserving keys because I needed it for the Arr:random($num) usage, and overlooked the fact that shuffle didn't preserve them. I'll split them out so random() can preserve it's keys and shuffle() doesn't.

}

/**
Expand Down Expand Up @@ -708,15 +694,24 @@ public static function set(&$array, $key, $value)
*/
public static function shuffle($array, $seed = null)
{
if (is_null($seed)) {
shuffle($array);
} else {
if (! is_null($seed)) {
mt_srand($seed);
shuffle($array);
mt_srand();

return $array;
}

return $array;
$keys = array_keys($array);
$shuffled = [];

for ($i = count($keys) - 1; $i >= 0; $i--) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given a zero-based array, the Fisher-Yates algorithm iterates from count - 1 down to 1.

Therefore, it should be: for ($i = count($keys) - 1; $i >= 1; $i--) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I just noticed that in 9f39ac6 you tweaked the original algorithm, so I can't say for sure…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the core counts down to 1 not 0, but since it shuffles in-place while I'm injecting into a new array to preserve keys, the final element was always dropped off the end. This change ensures the final element is added into the new array.

Since it shouldn't be preserving keys, I will go back to the original algorithm.

$j = random_int(0, $i);
$shuffled[$keys[$j]] = $array[$keys[$j]];
$keys[$j] = $keys[$i];
}

return $shuffled;
}

/**
Expand Down
54 changes: 54 additions & 0 deletions tests/Support/SupportArrTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,25 @@ public function testRandom()
$this->assertCount(2, array_intersect_assoc(['one' => 'foo', 'two' => 'bar', 'three' => 'baz'], $random));
}

public function testRandomIsActuallyRandom()
{
$values = [];

for ($i = 0; $i < 100; $i++) {
$values[] = Arr::random(['foo', 'bar', 'baz']);
}

$this->assertContains('foo', $values);
$this->assertContains('bar', $values);
$this->assertContains('baz', $values);
}

public function testRandomNotIncrementingKeys()
{
$random = Arr::random(['foo' => 'foo', 'bar' => 'bar', 'baz' => 'baz']);
$this->assertContains($random, ['foo', 'bar', 'baz']);
}

public function testRandomOnEmptyArray()
{
$random = Arr::random([], 0);
Expand Down Expand Up @@ -817,6 +836,41 @@ public function testShuffleWithSeed()
);
}

public function testShuffle()
{
$this->assertEquals(
Arr::shuffle(range(0, 10)),
Arr::shuffle(range(0, 10)),
'Shuffled array should have the same elements.'
);

$this->assertNotSame(
Arr::shuffle(range(0, 10)),
Arr::shuffle(range(0, 10)),
'Shuffled array should not have the same order.'
);
}

public function testShuffleWithKeys()
{
$array = ['one' => 'foo', 'two' => 'bar', 'three' => 'baz', 'four' => 'boom'];

$sameElements = true;
$dontMatch = false;

// Attempt 5x times to prevent random failures
for ($i = 0; $i < 5; $i++) {
$one = Arr::shuffle($array);
$two = Arr::shuffle($array);

$sameElements = $sameElements && $one == $two;
$dontMatch = $dontMatch || $one !== $two;
}

$this->assertTrue($sameElements, 'Shuffled array should always have the same elements.');
$this->assertTrue($dontMatch, 'Shuffled array should not have the same order.');
}

public function testSort()
{
$unsorted = [
Expand Down