mirror of
https://github.com/php/php-src.git
synced 2025-08-15 21:48:51 +02:00
random: Fix off-by-one in fast path selection of Randomizer::getBytesFromString() (#10449)
With a single byte we can choose offsets between 0x00 and 0xff, thus 0x100 different offsets. We only need to use the slow path for sources of more than 0x100 bytes. The previous version was correct with regard to the output expectations, it was just slower than necessary. Better fix this now while we still can before being bound by our BC guarantees with regard to emitted sequences. This also adds a test to verify the behavior: For powers of two we never reject any values during rejection sampling, we just need to mask off the unneeded bits. Thus we can specifically verify that the number of calls to the engine match the expected amount. We also verify that all the possible values are emitted to make sure the masking does not remove any required bits. For inputs longer than 0x100 bytes we need trust the `range()` implementation to be unbiased, but still verify the number of engine calls and perform a basic output check.
This commit is contained in:
parent
8f318c383d
commit
64d9080534
3 changed files with 122 additions and 1 deletions
|
@ -401,7 +401,7 @@ PHP_METHOD(Random_Randomizer, getBytesFromString)
|
||||||
|
|
||||||
retval = zend_string_alloc(length, 0);
|
retval = zend_string_alloc(length, 0);
|
||||||
|
|
||||||
if (source_length > 0xFF) {
|
if (source_length > 0x100) {
|
||||||
while (total_size < length) {
|
while (total_size < length) {
|
||||||
uint64_t offset = randomizer->algo->range(randomizer->status, 0, source_length - 1);
|
uint64_t offset = randomizer->algo->range(randomizer->status, 0, source_length - 1);
|
||||||
|
|
||||||
|
|
|
@ -0,0 +1,112 @@
|
||||||
|
--TEST--
|
||||||
|
Random: Randomizer: getBytesFromString(): Fast Path Masking
|
||||||
|
--FILE--
|
||||||
|
<?php
|
||||||
|
|
||||||
|
use Random\Engine\Test\TestWrapperEngine;
|
||||||
|
use Random\Engine\Xoshiro256StarStar;
|
||||||
|
use Random\Randomizer;
|
||||||
|
|
||||||
|
require __DIR__ . "/../../engines.inc";
|
||||||
|
|
||||||
|
$allBytes = implode('', array_map(
|
||||||
|
fn ($byte) => chr($byte),
|
||||||
|
range(0x00, 0xff)
|
||||||
|
));
|
||||||
|
|
||||||
|
// Xoshiro256** is the fastest engine available.
|
||||||
|
$xoshiro = new Xoshiro256StarStar();
|
||||||
|
|
||||||
|
var_dump(strlen($allBytes));
|
||||||
|
echo PHP_EOL;
|
||||||
|
|
||||||
|
// Fast path: Inputs less than or equal to 256.
|
||||||
|
for ($i = 1; $i <= strlen($allBytes); $i *= 2) {
|
||||||
|
echo "{$i}:", PHP_EOL;
|
||||||
|
|
||||||
|
$wrapper = new TestWrapperEngine($xoshiro);
|
||||||
|
$r = new Randomizer($wrapper);
|
||||||
|
$result = $r->getBytesFromString(substr($allBytes, 0, $i), 20000);
|
||||||
|
|
||||||
|
// Xoshiro256** is a 64 Bit engine and thus generates 8 bytes at once.
|
||||||
|
// For powers of two we expect no rejections and thus exactly
|
||||||
|
// 20000/8 = 2500 calls to the engine.
|
||||||
|
var_dump($wrapper->getCount());
|
||||||
|
|
||||||
|
$count = [];
|
||||||
|
for ($j = 0; $j < strlen($result); $j++) {
|
||||||
|
$b = $result[$j];
|
||||||
|
$count[ord($b)] ??= 0;
|
||||||
|
$count[ord($b)]++;
|
||||||
|
}
|
||||||
|
|
||||||
|
// We also expect that each possible value appears at least once, if
|
||||||
|
// not is is very likely that some bits were erroneously masked away.
|
||||||
|
var_dump(count($count));
|
||||||
|
|
||||||
|
echo PHP_EOL;
|
||||||
|
}
|
||||||
|
|
||||||
|
echo "Slow Path:", PHP_EOL;
|
||||||
|
|
||||||
|
$wrapper = new TestWrapperEngine($xoshiro);
|
||||||
|
$r = new Randomizer($wrapper);
|
||||||
|
$result = $r->getBytesFromString($allBytes . $allBytes, 20000);
|
||||||
|
|
||||||
|
// In the slow path we expect one call per byte, i.e. 20000
|
||||||
|
var_dump($wrapper->getCount());
|
||||||
|
|
||||||
|
$count = [];
|
||||||
|
for ($j = 0; $j < strlen($result); $j++) {
|
||||||
|
$b = $result[$j];
|
||||||
|
$count[ord($b)] ??= 0;
|
||||||
|
$count[ord($b)]++;
|
||||||
|
}
|
||||||
|
|
||||||
|
// We also expect that each possible value appears at least once, if
|
||||||
|
// not is is very likely that some bits were erroneously masked away.
|
||||||
|
var_dump(count($count));
|
||||||
|
|
||||||
|
?>
|
||||||
|
--EXPECT--
|
||||||
|
int(256)
|
||||||
|
|
||||||
|
1:
|
||||||
|
int(2500)
|
||||||
|
int(1)
|
||||||
|
|
||||||
|
2:
|
||||||
|
int(2500)
|
||||||
|
int(2)
|
||||||
|
|
||||||
|
4:
|
||||||
|
int(2500)
|
||||||
|
int(4)
|
||||||
|
|
||||||
|
8:
|
||||||
|
int(2500)
|
||||||
|
int(8)
|
||||||
|
|
||||||
|
16:
|
||||||
|
int(2500)
|
||||||
|
int(16)
|
||||||
|
|
||||||
|
32:
|
||||||
|
int(2500)
|
||||||
|
int(32)
|
||||||
|
|
||||||
|
64:
|
||||||
|
int(2500)
|
||||||
|
int(64)
|
||||||
|
|
||||||
|
128:
|
||||||
|
int(2500)
|
||||||
|
int(128)
|
||||||
|
|
||||||
|
256:
|
||||||
|
int(2500)
|
||||||
|
int(256)
|
||||||
|
|
||||||
|
Slow Path:
|
||||||
|
int(20000)
|
||||||
|
int(256)
|
|
@ -27,14 +27,23 @@ final class TestShaEngine implements Engine
|
||||||
|
|
||||||
final class TestWrapperEngine implements Engine
|
final class TestWrapperEngine implements Engine
|
||||||
{
|
{
|
||||||
|
private int $count = 0;
|
||||||
|
|
||||||
public function __construct(private readonly Engine $engine)
|
public function __construct(private readonly Engine $engine)
|
||||||
{
|
{
|
||||||
}
|
}
|
||||||
|
|
||||||
public function generate(): string
|
public function generate(): string
|
||||||
{
|
{
|
||||||
|
$this->count++;
|
||||||
|
|
||||||
return $this->engine->generate();
|
return $this->engine->generate();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public function getCount(): int
|
||||||
|
{
|
||||||
|
return $this->count;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
final class TestXoshiro128PlusPlusEngine implements Engine
|
final class TestXoshiro128PlusPlusEngine implements Engine
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue