Skip to content

Commit ab5491f

Browse files
authored
Fix shift in rand_rangeXX() (#9088)
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.
1 parent c8f4801 commit ab5491f

File tree

2 files changed

+8
-6
lines changed

2 files changed

+8
-6
lines changed

ext/random/random.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
9292
total_size = 0;
9393
do {
9494
r = algo->generate(status);
95-
result = (result << (8 * status->last_generated_size)) | r;
95+
result = result | (r << (total_size * 8));
9696
total_size += status->last_generated_size;
9797
if (status->last_unsafe) {
9898
return 0;
@@ -127,7 +127,7 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
127127
total_size = 0;
128128
do {
129129
r = algo->generate(status);
130-
result = (result << (8 * status->last_generated_size)) | r;
130+
result = result | (r << (total_size * 8));
131131
total_size += status->last_generated_size;
132132
if (status->last_unsafe) {
133133
return 0;
@@ -148,7 +148,7 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
148148
total_size = 0;
149149
do {
150150
r = algo->generate(status);
151-
result = (result << (8 * status->last_generated_size)) | r;
151+
result = result | (r << (total_size * 8));
152152
total_size += status->last_generated_size;
153153
if (status->last_unsafe) {
154154
return 0;
@@ -183,7 +183,7 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
183183
total_size = 0;
184184
do {
185185
r = algo->generate(status);
186-
result = (result << (8 * status->last_generated_size)) | r;
186+
result = result | (r << (total_size * 8));
187187
total_size += status->last_generated_size;
188188
if (status->last_unsafe) {
189189
return 0;

ext/random/tests/03_randomizer/get_int_user.phpt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,11 @@ Random: Randomizer: User Engine results are correctly expanded for getInt()
66
$randomizer = new \Random\Randomizer (
77
new class () implements \Random\Engine
88
{
9+
public $count = 0;
10+
911
public function generate(): string
1012
{
11-
return "\x01";
13+
return "\x01\x02\x03\x04\x05\x06\x07\x08"[$this->count++];
1214
}
1315
}
1416
);
@@ -17,4 +19,4 @@ var_dump(bin2hex(pack('V', $randomizer->getInt(0, 0xFFFFFF))));
1719

1820
?>
1921
--EXPECT--
20-
string(8) "01010100"
22+
string(8) "01020300"

0 commit comments

Comments
 (0)