Fix byte expansion in rand_rangeXX()#9056
Merged
TimWolla merged 3 commits intophp:masterfrom Jul 20, 2022
Merged
Conversation
The last generated size is in bytes, whereas the shift is in bits. Multiple the generated size by 8 to correctly handle each byte once.
…geXX() We need to loop until we accumulate sufficient bytes, instead of just checking once. The version in the rejection loop was already correct.
Contributor
|
@TimWolla LGTM, Thank you! |
iluuu1994
reviewed
Jul 21, 2022
| do { | ||
| r = algo->generate(status); | ||
| result = (result << status->last_generated_size) | r; | ||
| result = (result << (8 * status->last_generated_size)) | r; |
Member
There was a problem hiding this comment.
This triggers undefined behavior when shifting:
If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined
This should either do % 32 on the rhs of << so that the shifted width doesn't exceed the integer width, or yield 0 if rhs >= 32 (that's what both gcc and clang seem to do with the UB right now).
For context:
https://github.com/php/php-src/runs/7440978954?check_suite_focus=true
TimWolla
added a commit
to TimWolla/php-src
that referenced
this pull request
Jul 21, 2022
The previous shifting logic is problematic for two reasons: 1. It invokes undefined behavior when the `->last_generated_size` is at least as large as the target integer in `result`, because the shift is larger than the target integer. This was reported in phpGH-9083. 2. It expands the returned bytes in a big-endian fashion: Earlier bytes are shifting into the most-significant position. As all the other logic in the random extension treats byte-strings as little-endian numbers this is inconsistent. By fixing the second issue, we can implicitly fix the first one: Instead of shifting the existing bits by the number of "newly added" bits, we shift the newly added bits by the number of existing bits. As we stop requesting new bits once the total_size reached the size of the target integer we can be sure to never invoke undefined behavior during shifting. The get_int_user.phpt test was adjusted to verify the little-endian behavior. It generates a single byte per call and we expect the first byte generated to appear at the start of the resulting number. see phpGH-9056 for a previous fix in the same area. Fixes phpGH-9083 which reports the undefined behavior. Resolves phpGH-9085 which was an alternative attempt to fix phpGH-9083.
Girgias
pushed a commit
that referenced
this pull request
Jul 22, 2022
The previous shifting logic is problematic for two reasons: 1. It invokes undefined behavior when the `->last_generated_size` is at least as large as the target integer in `result`, because the shift is larger than the target integer. This was reported in GH-9083. 2. It expands the returned bytes in a big-endian fashion: Earlier bytes are shifting into the most-significant position. As all the other logic in the random extension treats byte-strings as little-endian numbers this is inconsistent. By fixing the second issue, we can implicitly fix the first one: Instead of shifting the existing bits by the number of "newly added" bits, we shift the newly added bits by the number of existing bits. As we stop requesting new bits once the total_size reached the size of the target integer we can be sure to never invoke undefined behavior during shifting. The get_int_user.phpt test was adjusted to verify the little-endian behavior. It generates a single byte per call and we expect the first byte generated to appear at the start of the resulting number. see GH-9056 for a previous fix in the same area. Fixes GH-9083 which reports the undefined behavior. Resolves GH-9085 which was an alternative attempt to fix GH-9083.
TimWolla
added a commit
that referenced
this pull request
Jul 22, 2022
This file contains hidden or 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
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.
/cc @zeriyoshi