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

Jump to bottom

Improve error reporting in random extension #9071

Merged
merged 10 commits into from
Jul 25, 2022
Merged

Improve error reporting in random extension #9071

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8daa5fe
Use `php_random_bytes_throw()` in Secure engine's generate()
TimWolla Jul 20, 2022
b7bdf1a
Use `php_random_int_throw()` in Secure engine's range()
TimWolla Jul 20, 2022
f93daeb
Throw exception when a user engine returns an empty string
TimWolla Jul 20, 2022
fdfa2f3
Throw exception when the range selector fails to get acceptable numbe…
TimWolla Jul 20, 2022
12b07ce
Improve user_unsafe test
TimWolla Jul 20, 2022
3e18441
Fix user_unsafe test
TimWolla Jul 20, 2022
8adb67a
Remove duplication in user_unsafe.phpt
TimWolla Jul 20, 2022
3090379
Catch `Throwable` in user_unsafe.phpt
TimWolla Jul 21, 2022
39b996a
Throw an error if an engine returns an empty string
TimWolla Jul 24, 2022
734c79c
Throw an Error if range fails to find an acceptable number in 50 atte…
TimWolla Jul 24, 2022
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
6 changes: 3 additions & 3 deletions ext/random/engine_secure.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ static uint64_t generate(php_random_status *status)
{
zend_ulong r = 0;

if (php_random_bytes_silent(&r, sizeof(zend_ulong)) == FAILURE) {
if (php_random_bytes_throw(&r, sizeof(zend_ulong)) == FAILURE) {
status->last_unsafe = true;
}

Expand All @@ -38,9 +38,9 @@ static uint64_t generate(php_random_status *status)

static zend_long range(php_random_status *status, zend_long min, zend_long max)
{
zend_long result;
zend_long result = 0;

if (php_random_int_silent(min, max, &result) == FAILURE) {
if (php_random_int_throw(min, max, &result) == FAILURE) {
status->last_unsafe = true;
}

Expand Down
4 changes: 4 additions & 0 deletions ext/random/engine_user.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
#include "php.h"
#include "php_random.h"

#include "ext/spl/spl_exceptions.h"
TimWolla marked this conversation as resolved.
Show resolved Hide resolved
#include "Zend/zend_exceptions.h"

static uint64_t generate(php_random_status *status)
{
php_random_status_state_user *s = status->state;
Expand Down Expand Up @@ -50,6 +53,7 @@ static uint64_t generate(php_random_status *status)
result += ((uint64_t) (unsigned char) Z_STRVAL(retval)[i]) << (8 * i);
}
} else {
zend_throw_exception(spl_ce_DomainException, "A random engine must return a non-empty string", 0);
Copy link
Member

Choose a reason for hiding this comment

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

I would rather use a ValueError here

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that, but ValueError is documented as:

A ValueError is thrown when the type of an argument is correct but the value of it is incorrect.

Thus it is documented to apply to parameters / arguments, which is not exactly the case here.

Copy link
Member

Choose a reason for hiding this comment

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

We could update the ValueError documentation; but probably throwing an Error directly is the way to go. zend_throw_error() accepts NULL for the first parameter, and that is already used quite a lot across php-src. I don't think that introducing a new error type would be particularly useful, since Errors are not supposed to be caught.

status->last_unsafe = true;
return 0;
}
Expand Down
2 changes: 2 additions & 0 deletions ext/random/random.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
while (UNEXPECTED(result > limit)) {
/* If the requirements cannot be met in a cycles, return fail */
if (++count > 50) {
zend_throw_exception(spl_ce_RuntimeException, "Failed to generate an acceptable random number in 50 attempts", 0);
TimWolla marked this conversation as resolved.
Show resolved Hide resolved
status->last_unsafe = true;
return 0;
}
Expand Down Expand Up @@ -175,6 +176,7 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
while (UNEXPECTED(result > limit)) {
/* If the requirements cannot be met in a cycles, return fail */
if (++count > 50) {
zend_throw_exception(spl_ce_RuntimeException, "Failed to generate an acceptable random number in 50 attempts", 0);
TimWolla marked this conversation as resolved.
Show resolved Hide resolved
status->last_unsafe = true;
return 0;
}
Expand Down
200 changes: 137 additions & 63 deletions ext/random/tests/03_randomizer/user_unsafe.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,81 +3,155 @@ Random: Randomizer: User: Engine unsafe
--FILE--
<?php

// Empty generator
$randomizer = (new \Random\Randomizer(
new class () implements \Random\Engine {
public function generate(): string
{
return '';
}
}
));
use Random\Randomizer;

try {
var_dump($randomizer->getInt(\PHP_INT_MIN, \PHP_INT_MAX));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
final class EmptyStringEngine implements \Random\Engine {
public function generate(): string
{
return '';
}
}

try {
var_dump(bin2hex($randomizer->getBytes(1)));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
final class HeavilyBiasedEngine implements \Random\Engine {
public function generate(): string
{
return "\xff\xff\xff\xff\xff\xff\xff\xff";
}
}

try {
var_dump($randomizer->shuffleArray(\range(1, 10)));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
}
echo "=====================", PHP_EOL;

try {
var_dump($randomizer->shuffleBytes('foobar'));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
}
foreach ([
EmptyStringEngine::class,
HeavilyBiasedEngine::class,
] as $engine) {
echo $engine, PHP_EOL, "=====================", PHP_EOL, PHP_EOL;

// Infinite loop
$randomizer = (new \Random\Randomizer(
new class () implements \Random\Engine {
public function generate(): string
{
return "\xff\xff\xff\xff\xff\xff\xff\xff";
}
try {
var_dump((new Randomizer(new $engine()))->getInt(0, 123));
} catch (\RuntimeException $e) {
echo $e, PHP_EOL;
}

echo PHP_EOL, "-------", PHP_EOL, PHP_EOL;

try {
var_dump(bin2hex((new Randomizer(new $engine()))->getBytes(1)));
} catch (\RuntimeException $e) {
echo $e, PHP_EOL;
}

echo PHP_EOL, "-------", PHP_EOL, PHP_EOL;

try {
var_dump((new Randomizer(new $engine()))->shuffleArray(\range(1, 10)));
} catch (\RuntimeException $e) {
echo $e, PHP_EOL;
}

echo PHP_EOL, "-------", PHP_EOL, PHP_EOL;

try {
var_dump((new Randomizer(new $engine()))->shuffleBytes('foobar'));
} catch (\RuntimeException $e) {
echo $e, PHP_EOL;
}
));

try {
var_dump($randomizer->getInt(\PHP_INT_MIN, \PHP_INT_MAX));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
echo PHP_EOL, "=====================", PHP_EOL;
}

try {
var_dump(bin2hex($randomizer->getBytes(1)));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
}
?>
--EXPECTF--
=====================
EmptyStringEngine
=====================

try {
var_dump($randomizer->shuffleArray(\range(1, 10)));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
}
DomainException: A random engine must return a non-empty string in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getInt(0, 123)
#1 {main}

try {
var_dump($randomizer->shuffleBytes('foobar'));
} catch (\RuntimeException $e) {
echo $e->getMessage() . PHP_EOL;
}
Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getInt(0, 123)
#1 {main}

-------

DomainException: A random engine must return a non-empty string in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getBytes(1)
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getBytes(1)
#1 {main}

-------

DomainException: A random engine must return a non-empty string in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
#1 {main}

-------

DomainException: A random engine must return a non-empty string in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
#1 {main}

=====================
HeavilyBiasedEngine
=====================

RuntimeException: Failed to generate an acceptable random number in 50 attempts in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getInt(0, 123)
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->getInt(0, 123)
#1 {main}

-------

?>
--EXPECTF--
Random number generation failed
Random number generation failed
Random number generation failed
Random number generation failed
int(%d)
string(2) "ff"
Random number generation failed
Random number generation failed

-------

RuntimeException: Failed to generate an acceptable random number in 50 attempts in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
#1 {main}

-------

RuntimeException: Failed to generate an acceptable random number in 50 attempts in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
#1 {main}

Next RuntimeException: Random number generation failed in %s:%d
Stack trace:
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
#1 {main}

=====================