Improve error messages in php_random_bytes() #9169
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmb69
reviewed
Jul 27, 2022
devnexen
reviewed
Jul 27, 2022
|
Looks ok in general minus @cmb69 remarks which makes sense. |
TimWolla
added a commit
to TimWolla/php-src
that referenced
this pull request
Jul 31, 2022
The only way that `php_binary_string_shuffle` fails is when the engine itself
fails. With the currently available list of engines we have:
- Mt19937 : Infallible.
- PcgOneseq128XslRr64: Infallible.
- Xoshiro256StarStar : Infallible.
- Secure : Practically infallible on modern systems.
Exception messages were cleaned up in phpGH-9169.
- User : Error when returning an empty string.
Error when seriously biased (range() fails).
And whatever Throwable the userland developer decides to use.
So the existing engines are either infallible or throw an Exception/Error with
a high quality message themselves, making this exception not a value-add and
possibly confusing.
TimWolla
added a commit
to TimWolla/php-src
that referenced
this pull request
Aug 1, 2022
The only way that `php_binary_string_shuffle` fails is when the engine itself
fails. With the currently available list of engines we have:
- Mt19937 : Infallible.
- PcgOneseq128XslRr64: Infallible.
- Xoshiro256StarStar : Infallible.
- Secure : Practically infallible on modern systems.
Exception messages were cleaned up in phpGH-9169.
- User : Error when returning an empty string.
Error when seriously biased (range() fails).
And whatever Throwable the userland developer decides to use.
So the existing engines are either infallible or throw an Exception/Error with
a high quality message themselves, making this exception not a value-add and
possibly confusing.
TimWolla
added a commit
that referenced
this pull request
Aug 2, 2022
* Remove exception in Randomizer::shuffleBytes()
The only way that `php_binary_string_shuffle` fails is when the engine itself
fails. With the currently available list of engines we have:
- Mt19937 : Infallible.
- PcgOneseq128XslRr64: Infallible.
- Xoshiro256StarStar : Infallible.
- Secure : Practically infallible on modern systems.
Exception messages were cleaned up in GH-9169.
- User : Error when returning an empty string.
Error when seriously biased (range() fails).
And whatever Throwable the userland developer decides to use.
So the existing engines are either infallible or throw an Exception/Error with
a high quality message themselves, making this exception not a value-add and
possibly confusing.
* Remove exception in Randomizer::shuffleArray()
Same reasoning as in the previous commit applies.
* Remove exception in Randomizer::getInt()
Same reasoning as in the previous commit applies.
* Remove exception in Randomizer::nextInt()
Same reasoning as in the previous commit applies, except that it won't throw on
a seriously biased user engine, as `range()` is not used.
* Remove exception in Randomizer::getBytes()
Same reasoning as in the previous commit applies.
* Remove exception in Mt19937::generate()
This implementation is shared across all native engines. Thus the same
reasoning as the previous commits applies, except that the User engine does not
use this method. Thus is only applicable to the Secure engine, which is the
only fallible native engine.
* [ci skip] Add cleanup of Randomizer exceptions to NEWS
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
see #9160 (comment) for context
Not sure if this is lipstick on a pig, because
php_random_bytes()really really should not fail on modern systems, but I'm putting it up for discussion nonetheless.Technically a breaking change, in case someone relies in the specific wording in the error message, but this exception really shouldn't be caught as the system likely is broken beyond repair.
Review requests for devnexen for the general C and operating system experience, cmb for the Windows and PHP internals experience.