Skip to content

Commit 60f149f

Browse files
TimWollaTimWolla
authoredJul 25, 2022
Improve error reporting in random extension (#9071)
* Use `php_random_bytes_throw()` in Secure engine's generate() This exposes the underlying exception, improving debugging: Fatal error: Uncaught Exception: Cannot open source device in php-src/test.php:5 Stack trace: #0 php-src/test.php(5): Random\Engine\Secure->generate() #1 {main} Next RuntimeException: Random number generation failed in php-src/test.php:5 Stack trace: #0 php-src/test.php(5): Random\Engine\Secure->generate() #1 {main} thrown in php-src/test.php on line 5 * Use `php_random_int_throw()` in Secure engine's range() This exposes the underlying exception, improving debugging: Exception: Cannot open source device in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3) #1 {main} Next RuntimeException: Random number generation failed in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3) #1 {main} * Throw exception when a user engine returns an empty string This improves debugging, because the actual reason for the failure is available as a previous Exception: DomainException: The returned string must not be empty in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getBytes(123) #1 {main} Next RuntimeException: Random number generation failed in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getBytes(123) #1 {main} * Throw exception when the range selector fails to get acceptable numbers in 50 attempts This improves debugging, because the actual reason for the failure is available as a previous Exception: RuntimeException: Failed to generate an acceptable random number in 50 attempts in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3) #1 {main} Next RuntimeException: Random number generation failed in php-src/test.php:17 Stack trace: #0 php-src/test.php(17): Random\Randomizer->getInt(1, 3) #1 {main} * Improve user_unsafe test Select parameters for ->getInt() that will actually lead to unsafe behavior. * Fix user_unsafe test If an engine fails once it will be permanently poisoned by setting `->last_unsafe`. This is undesirable for the test, because it skews the results. Fix this by creating a fresh engine for each "assertion". * Remove duplication in user_unsafe.phpt * Catch `Throwable` in user_unsafe.phpt As we print the full stringified exception we implicitly assert the type of the exception. No need to be overly specific in the catch block. * Throw an error if an engine returns an empty string * Throw an Error if range fails to find an acceptable number in 50 attempts
1 parent 34b352d commit 60f149f

File tree

4 files changed

+147
-68
lines changed

4 files changed

+147
-68
lines changed
 

‎ext/random/engine_secure.c

+3-3
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ static uint64_t generate(php_random_status *status)
2929
{
3030
zend_ulong r = 0;
3131

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

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

3939
static zend_long range(php_random_status *status, zend_long min, zend_long max)
4040
{
41-
zend_long result;
41+
zend_long result = 0;
4242

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

‎ext/random/engine_user.c

+1
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ static uint64_t generate(php_random_status *status)
5050
result += ((uint64_t) (unsigned char) Z_STRVAL(retval)[i]) << (8 * i);
5151
}
5252
} else {
53+
zend_throw_error(NULL, "A random engine must return a non-empty string");
5354
status->last_unsafe = true;
5455
return 0;
5556
}

‎ext/random/random.c

+6-2
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,8 @@ static zend_object_handlers random_engine_xoshiro256starstar_object_handlers;
8282
static zend_object_handlers random_engine_secure_object_handlers;
8383
static zend_object_handlers random_randomizer_object_handlers;
8484

85+
#define RANDOM_RANGE_ATTEMPTS (50)
86+
8587
static inline uint32_t rand_range32(const php_random_algo *algo, php_random_status *status, uint32_t umax)
8688
{
8789
uint32_t result, limit, r;
@@ -118,7 +120,8 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
118120
/* Discard numbers over the limit to avoid modulo bias */
119121
while (UNEXPECTED(result > limit)) {
120122
/* If the requirements cannot be met in a cycles, return fail */
121-
if (++count > 50) {
123+
if (++count > RANDOM_RANGE_ATTEMPTS) {
124+
zend_throw_error(NULL, "Failed to generate an acceptable random number in %d attempts", RANDOM_RANGE_ATTEMPTS);
122125
status->last_unsafe = true;
123126
return 0;
124127
}
@@ -174,7 +177,8 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
174177
/* Discard numbers over the limit to avoid modulo bias */
175178
while (UNEXPECTED(result > limit)) {
176179
/* If the requirements cannot be met in a cycles, return fail */
177-
if (++count > 50) {
180+
if (++count > RANDOM_RANGE_ATTEMPTS) {
181+
zend_throw_error(NULL, "Failed to generate an acceptable random number in %d attempts", RANDOM_RANGE_ATTEMPTS);
178182
status->last_unsafe = true;
179183
return 0;
180184
}

‎ext/random/tests/03_randomizer/user_unsafe.phpt

+137-63
Original file line numberDiff line numberDiff line change
@@ -3,81 +3,155 @@ Random: Randomizer: User: Engine unsafe
33
--FILE--
44
<?php
55

6-
// Empty generator
7-
$randomizer = (new \Random\Randomizer(
8-
new class () implements \Random\Engine {
9-
public function generate(): string
10-
{
11-
return '';
12-
}
13-
}
14-
));
6+
use Random\Randomizer;
157

16-
try {
17-
var_dump($randomizer->getInt(\PHP_INT_MIN, \PHP_INT_MAX));
18-
} catch (\RuntimeException $e) {
19-
echo $e->getMessage() . PHP_EOL;
8+
final class EmptyStringEngine implements \Random\Engine {
9+
public function generate(): string
10+
{
11+
return '';
12+
}
2013
}
2114

22-
try {
23-
var_dump(bin2hex($randomizer->getBytes(1)));
24-
} catch (\RuntimeException $e) {
25-
echo $e->getMessage() . PHP_EOL;
15+
final class HeavilyBiasedEngine implements \Random\Engine {
16+
public function generate(): string
17+
{
18+
return "\xff\xff\xff\xff\xff\xff\xff\xff";
19+
}
2620
}
2721

28-
try {
29-
var_dump($randomizer->shuffleArray(\range(1, 10)));
30-
} catch (\RuntimeException $e) {
31-
echo $e->getMessage() . PHP_EOL;
32-
}
22+
echo "=====================", PHP_EOL;
3323

34-
try {
35-
var_dump($randomizer->shuffleBytes('foobar'));
36-
} catch (\RuntimeException $e) {
37-
echo $e->getMessage() . PHP_EOL;
38-
}
24+
foreach ([
25+
EmptyStringEngine::class,
26+
HeavilyBiasedEngine::class,
27+
] as $engine) {
28+
echo $engine, PHP_EOL, "=====================", PHP_EOL, PHP_EOL;
3929

40-
// Infinite loop
41-
$randomizer = (new \Random\Randomizer(
42-
new class () implements \Random\Engine {
43-
public function generate(): string
44-
{
45-
return "\xff\xff\xff\xff\xff\xff\xff\xff";
46-
}
30+
try {
31+
var_dump((new Randomizer(new $engine()))->getInt(0, 123));
32+
} catch (Throwable $e) {
33+
echo $e, PHP_EOL;
34+
}
35+
36+
echo PHP_EOL, "-------", PHP_EOL, PHP_EOL;
37+
38+
try {
39+
var_dump(bin2hex((new Randomizer(new $engine()))->getBytes(1)));
40+
} catch (Throwable $e) {
41+
echo $e, PHP_EOL;
42+
}
43+
44+
echo PHP_EOL, "-------", PHP_EOL, PHP_EOL;
45+
46+
try {
47+
var_dump((new Randomizer(new $engine()))->shuffleArray(\range(1, 10)));
48+
} catch (Throwable $e) {
49+
echo $e, PHP_EOL;
50+
}
51+
52+
echo PHP_EOL, "-------", PHP_EOL, PHP_EOL;
53+
54+
try {
55+
var_dump((new Randomizer(new $engine()))->shuffleBytes('foobar'));
56+
} catch (Throwable $e) {
57+
echo $e, PHP_EOL;
4758
}
48-
));
4959

50-
try {
51-
var_dump($randomizer->getInt(\PHP_INT_MIN, \PHP_INT_MAX));
52-
} catch (\RuntimeException $e) {
53-
echo $e->getMessage() . PHP_EOL;
60+
echo PHP_EOL, "=====================", PHP_EOL;
5461
}
5562

56-
try {
57-
var_dump(bin2hex($randomizer->getBytes(1)));
58-
} catch (\RuntimeException $e) {
59-
echo $e->getMessage() . PHP_EOL;
60-
}
63+
?>
64+
--EXPECTF--
65+
=====================
66+
EmptyStringEngine
67+
=====================
6168

62-
try {
63-
var_dump($randomizer->shuffleArray(\range(1, 10)));
64-
} catch (\RuntimeException $e) {
65-
echo $e->getMessage() . PHP_EOL;
66-
}
69+
Error: A random engine must return a non-empty string in %s:%d
70+
Stack trace:
71+
#0 %s(%d): Random\Randomizer->getInt(0, 123)
72+
#1 {main}
6773

68-
try {
69-
var_dump($randomizer->shuffleBytes('foobar'));
70-
} catch (\RuntimeException $e) {
71-
echo $e->getMessage() . PHP_EOL;
72-
}
74+
Next RuntimeException: Random number generation failed in %s:%d
75+
Stack trace:
76+
#0 %s(%d): Random\Randomizer->getInt(0, 123)
77+
#1 {main}
78+
79+
-------
80+
81+
Error: A random engine must return a non-empty string in %s:%d
82+
Stack trace:
83+
#0 %s(%d): Random\Randomizer->getBytes(1)
84+
#1 {main}
85+
86+
Next RuntimeException: Random number generation failed in %s:%d
87+
Stack trace:
88+
#0 %s(%d): Random\Randomizer->getBytes(1)
89+
#1 {main}
90+
91+
-------
92+
93+
Error: A random engine must return a non-empty string in %s:%d
94+
Stack trace:
95+
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
96+
#1 {main}
97+
98+
Next RuntimeException: Random number generation failed in %s:%d
99+
Stack trace:
100+
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
101+
#1 {main}
102+
103+
-------
104+
105+
Error: A random engine must return a non-empty string in %s:%d
106+
Stack trace:
107+
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
108+
#1 {main}
109+
110+
Next RuntimeException: Random number generation failed in %s:%d
111+
Stack trace:
112+
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
113+
#1 {main}
114+
115+
=====================
116+
HeavilyBiasedEngine
117+
=====================
118+
119+
Error: Failed to generate an acceptable random number in 50 attempts in %s:%d
120+
Stack trace:
121+
#0 %s(%d): Random\Randomizer->getInt(0, 123)
122+
#1 {main}
123+
124+
Next RuntimeException: Random number generation failed in %s:%d
125+
Stack trace:
126+
#0 %s(%d): Random\Randomizer->getInt(0, 123)
127+
#1 {main}
128+
129+
-------
73130

74-
?>
75-
--EXPECTF--
76-
Random number generation failed
77-
Random number generation failed
78-
Random number generation failed
79-
Random number generation failed
80-
int(%d)
81131
string(2) "ff"
82-
Random number generation failed
83-
Random number generation failed
132+
133+
-------
134+
135+
Error: Failed to generate an acceptable random number in 50 attempts in %s:%d
136+
Stack trace:
137+
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
138+
#1 {main}
139+
140+
Next RuntimeException: Random number generation failed in %s:%d
141+
Stack trace:
142+
#0 %s(%d): Random\Randomizer->shuffleArray(Array)
143+
#1 {main}
144+
145+
-------
146+
147+
Error: Failed to generate an acceptable random number in 50 attempts in %s:%d
148+
Stack trace:
149+
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
150+
#1 {main}
151+
152+
Next RuntimeException: Random number generation failed in %s:%d
153+
Stack trace:
154+
#0 %s(%d): Random\Randomizer->shuffleBytes('foobar')
155+
#1 {main}
156+
157+
=====================

0 commit comments

Comments
 (0)