Fix byte expansion in rand_rangeXX() #9056

TimWolla · 2022-07-19T22:31:03Z

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.

zeriyoshi · 2022-07-20T01:29:20Z

@TimWolla LGTM, Thank you!

iluuu1994 · 2022-07-21T11:32:00Z

ext/random/random.c

 		r = algo->generate(status);
-		result = (result << status->last_generated_size) | r;
+		result = (result << (8 * status->last_generated_size)) | r;


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

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.

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.

see GH-9083 see GH-9088 see GH-9056

TimWolla added 2 commits July 20, 2022 00:27

Fix shift in rand_range??()

db80d2f

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.

Correctly handle user engines returning less than 4 bytes in rand_ran…

5cc5da1

…geXX() We need to loop until we accumulate sufficient bytes, instead of just checking once. The version in the rejection loop was already correct.

github-actions bot added the Extension: random label Jul 19, 2022

Clean up some repetition in rand_rangeXX()

68be9c7

TimWolla merged commit 804c3fc into php:master Jul 20, 2022

TimWolla deleted the random-rand-range-shift branch July 20, 2022 15:33

iluuu1994 reviewed Jul 21, 2022

View reviewed changes

guilliamxavier mentioned this pull request Jul 21, 2022

Random extension random.c:95:20: runtime error: shift exponent 32 is too large for 32-bit type 'unsigned int' #9083

Closed

zeriyoshi mentioned this pull request Jul 21, 2022

[Random] fix undefined behaviour part2 #9085

Closed

TimWolla mentioned this pull request Jul 21, 2022

Fix shift in rand_rangeXX() #9088

Merged

TimWolla added a commit that referenced this pull request Jul 22, 2022

[ci skip] Update NEWS for byte expansion in rand_rangeXX()

eff9aed

see GH-9083 see GH-9088 see GH-9056

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix byte expansion in rand_rangeXX() #9056

Fix byte expansion in rand_rangeXX() #9056

TimWolla commented Jul 19, 2022

zeriyoshi commented Jul 20, 2022

iluuu1994 Jul 21, 2022 •

edited

Loading

guilliamxavier Jul 21, 2022

Fix byte expansion in rand_rangeXX() #9056

Fix byte expansion in rand_rangeXX() #9056

Conversation

TimWolla commented Jul 19, 2022

zeriyoshi commented Jul 20, 2022

iluuu1994 Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

guilliamxavier Jul 21, 2022

Choose a reason for hiding this comment

iluuu1994 Jul 21, 2022 •

edited

Loading