[go: up one dir, main page]

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

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

Closed
Girgias opened this issue Jul 21, 2022 · 5 comments · Fixed by #9088

Comments

@Girgias
Copy link
Member
Girgias commented Jul 21, 2022

Description

Various SPL tests on master now fail with:

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

PHP Version

master

Operating System

Fedora 35

@cmb69
Copy link
Member
cmb69 commented Jul 21, 2022

ext/spl/php_spl.c includes ext/random/php_random.h:

#include "ext/random/php_random.h"

However, that is apparently not needed for that compilation unit.

@guilliamxavier
Copy link
Contributor

@cmb69 indeed, this line (previously

#include "ext/standard/php_mt_rand.h"
) could have been removed in fd8770e (maybe should be now)

But besides that, there's indeed an UB to "fix": #9056 (comment)

@Girgias
Copy link
Member Author
Girgias commented Jul 21, 2022

ext/spl/php_spl.c includes ext/random/php_random.h:

#include "ext/random/php_random.h"

However, that is apparently not needed for that compilation unit.

Removing it doesn't seem to fix the issue in SPL anyway

@guilliamxavier
Copy link
Contributor

Sorry for the noise:

#include "ext/random/php_random.h"
(previously
#include "ext/standard/php_rand.h"
) seems unneeded too (but there I don't even see why it was added in the first place)

Anyway, removing it won't fix the issue either 😅

@cmb69
Copy link
Member
cmb69 commented Jul 21, 2022

Removing it doesn't seem to fix the issue in SPL anyway

Nope, but #9056 (comment) should.

TimWolla added a commit to TimWolla/php-src that referenced this issue 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 issue 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants