Skip to content

Remove ->last_unsafe from php_random_status #9132

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

Merged
merged 1 commit into from
Jul 26, 2022
Merged

Remove ->last_unsafe from php_random_status #9132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion ext/random/engine_mt19937.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ PHP_METHOD(Random_Engine_Mt19937, generate)

generated = engine->algo->generate(engine->status);
size = engine->status->last_generated_size;
if (engine->status->last_unsafe) {
if (EG(exception)) {
zend_throw_exception(spl_ce_RuntimeException, "Random number generation failed", 0);
RETURN_THROWS();
Copy link
Contributor

@Danack Danack Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

tbh, I don't fully understand this bit, but it looks like an exception has happened elsewhere, and the code is replacing that exception with a new one. Is it possible to set the previous exception, so that the info isn't lost?

And same comment for the other ones below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it possible to set the previous exception, so that the info isn't lost?

zend_throw_exception is smart enough to do that automatically:

php-src/Zend/zend_exceptions.c

Line 182 in 9090e26

zend_object *previous = EG(exception);

This can be seen in action here:

php-src/ext/random/tests/03_randomizer/user_unsafe.phpt

Lines 135 to 143 in 9090e26

Error: 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}

I plan to fully remove the RuntimeException here in a future PR, though, so that the underlying error of the broken engine is directly exposed to the user. This PR focuses on the cleanup work to make that possible, one piece at a time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. TIL zend_throw_exception is smarter than me.

}
Expand Down
8 changes: 2 additions & 6 deletions ext/random/engine_secure.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,7 @@ static uint64_t generate(php_random_status *status)
{
zend_ulong r = 0;

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

return r;
}
Expand All @@ -40,9 +38,7 @@ static zend_long range(php_random_status *status, zend_long min, zend_long max)
{
zend_long result = 0;

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

return result;
}
Expand Down
2 changes: 0 additions & 2 deletions ext/random/engine_user.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ static uint64_t generate(php_random_status *status)
zend_call_known_instance_method_with_0_params(s->generate_method, s->object, &retval);

if (EG(exception)) {
status->last_unsafe = true;
return 0;
}

Expand All @@ -51,7 +50,6 @@ static uint64_t generate(php_random_status *status)
}
} else {
zend_throw_error(NULL, "A random engine must return a non-empty string");
status->last_unsafe = true;
return 0;
}

Expand Down
1 change: 0 additions & 1 deletion ext/random/php_random.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,6 @@ PHPAPI int php_random_int(zend_long min, zend_long max, zend_long *result, bool

typedef struct _php_random_status_ {
size_t last_generated_size;
bool last_unsafe;
void *state;
} php_random_status;

Expand Down
12 changes: 4 additions & 8 deletions ext/random/random.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
r = algo->generate(status);
result = result | (r << (total_size * 8));
total_size += status->last_generated_size;
if (status->last_unsafe) {
if (EG(exception)) {
return 0;
}
} while (total_size < sizeof(uint32_t));
Expand All @@ -122,7 +122,6 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
/* If the requirements cannot be met in a cycles, return fail */
if (++count > RANDOM_RANGE_ATTEMPTS) {
zend_throw_error(NULL, "Failed to generate an acceptable random number in %d attempts", RANDOM_RANGE_ATTEMPTS);
status->last_unsafe = true;
return 0;
}

Expand All @@ -132,7 +131,7 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
r = algo->generate(status);
result = result | (r << (total_size * 8));
total_size += status->last_generated_size;
if (status->last_unsafe) {
if (EG(exception)) {
return 0;
}
} while (total_size < sizeof(uint32_t));
Expand All @@ -153,7 +152,7 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
r = algo->generate(status);
result = result | (r << (total_size * 8));
total_size += status->last_generated_size;
if (status->last_unsafe) {
if (EG(exception)) {
return 0;
}
} while (total_size < sizeof(uint64_t));
Expand All @@ -179,7 +178,6 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
/* If the requirements cannot be met in a cycles, return fail */
if (++count > RANDOM_RANGE_ATTEMPTS) {
zend_throw_error(NULL, "Failed to generate an acceptable random number in %d attempts", RANDOM_RANGE_ATTEMPTS);
status->last_unsafe = true;
return 0;
}

Expand All @@ -189,7 +187,7 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
r = algo->generate(status);
result = result | (r << (total_size * 8));
total_size += status->last_generated_size;
if (status->last_unsafe) {
if (EG(exception)) {
return 0;
}
} while (total_size < sizeof(uint64_t));
Expand Down Expand Up @@ -245,7 +243,6 @@ PHPAPI php_random_status *php_random_status_alloc(const php_random_algo *algo, c
php_random_status *status = pecalloc(1, sizeof(php_random_status), persistent);

status->last_generated_size = algo->generate_size;
status->last_unsafe = false;
status->state = algo->state_size > 0 ? pecalloc(1, algo->state_size, persistent) : NULL;

return status;
Expand All @@ -254,7 +251,6 @@ PHPAPI php_random_status *php_random_status_alloc(const php_random_algo *algo, c
PHPAPI php_random_status *php_random_status_copy(const php_random_algo *algo, php_random_status *old_status, php_random_status *new_status)
{
new_status->last_generated_size = old_status->last_generated_size;
new_status->last_unsafe = old_status->last_unsafe;
new_status->state = memcpy(new_status->state, old_status->state, algo->state_size);

return new_status;
Expand Down
6 changes: 3 additions & 3 deletions ext/random/randomizer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ PHP_METHOD(Random_Randomizer, getInt)
zend_throw_exception(spl_ce_RuntimeException, "Generated value exceeds size of int", 0);
RETURN_THROWS();
}
if (randomizer->status->last_unsafe) {
if (EG(exception)) {
zend_throw_exception(spl_ce_RuntimeException, "Random number generation failed", 0);
RETURN_THROWS();
}
Expand All @@ -123,7 +123,7 @@ PHP_METHOD(Random_Randomizer, getInt)
}

result = randomizer->algo->range(randomizer->status, min, max);
if (randomizer->status->last_unsafe) {
if (EG(exception)) {
zend_throw_exception(spl_ce_RuntimeException, "Random number generation failed", 0);
RETURN_THROWS();
}
Expand Down Expand Up @@ -155,7 +155,7 @@ PHP_METHOD(Random_Randomizer, getBytes)

while (total_size < required_size) {
result = randomizer->algo->generate(randomizer->status);
if (randomizer->status->last_unsafe) {
if (EG(exception)) {
zend_string_free(retval);
zend_throw_exception(spl_ce_RuntimeException, "Random number generation failed", 0);
RETURN_THROWS();
Expand Down
4 changes: 2 additions & 2 deletions ext/standard/array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2936,7 +2936,7 @@ PHPAPI bool php_array_data_shuffle(const php_random_algo *algo, php_random_statu
}
while (--n_left) {
rnd_idx = algo->range(status, 0, n_left);
if (status->last_unsafe) {
if (EG(exception)) {
return false;
}
if (rnd_idx != n_left) {
Expand Down Expand Up @@ -2964,7 +2964,7 @@ PHPAPI bool php_array_data_shuffle(const php_random_algo *algo, php_random_statu
}
while (--n_left) {
rnd_idx = algo->range(status, 0, n_left);
if (status->last_unsafe) {
if (EG(exception)) {
return false;
}
if (rnd_idx != n_left) {
Expand Down
2 changes: 1 addition & 1 deletion ext/standard/string.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5755,7 +5755,7 @@ PHPAPI bool php_binary_string_shuffle(const php_random_algo *algo, php_random_st

while (--n_left) {
rnd_idx = algo->range(status, 0, n_left);
if (status->last_unsafe) {
if (EG(exception)) {
return false;
}
if (rnd_idx != n_left) {
Expand Down