Skip to content
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.

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

Fix shift in rand_rangeXX() #9088

Merged
merged 1 commit into from
Jul 22, 2022
Merged

Conversation

TimWolla
Copy link
Member

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 Random extension random.c:95:20: runtime error: shift exponent 32 is too large for 32-bit type 'unsigned int' #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.

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 Girgias merged commit ab5491f into php:master Jul 22, 2022
@TimWolla TimWolla deleted the random-range-shift branch July 22, 2022 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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