Skip to content

Fix byte expansion in rand_rangeXX() #9056

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 20, 2022
Merged

Fix byte expansion in rand_rangeXX() #9056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db80d2f
Fix shift in rand_range??()
TimWolla Jul 19, 2022
5cc5da1
Correctly handle user engines returning less than 4 bytes in rand_ran…
TimWolla Jul 19, 2022
68be9c7
Clean up some repetition in rand_rangeXX()
TimWolla Jul 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 20 additions & 32 deletions ext/random/random.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,19 +88,16 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
size_t total_size = 0;
uint32_t count = 0;

result = algo->generate(status);
total_size = status->last_generated_size;
if (status->last_unsafe) {
return 0;
}
if (total_size < sizeof(uint32_t)) {
result = 0;
total_size = 0;
do {
r = algo->generate(status);
result = (result << status->last_generated_size) | r;
result = (result << (8 * status->last_generated_size)) | r;
Copy link
Member

@iluuu1994 iluuu1994 Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xref #9083

total_size += status->last_generated_size;
if (status->last_unsafe) {
return 0;
}
}
} while (total_size < sizeof(uint32_t));

/* Special case where no modulus is required */
if (UNEXPECTED(umax == UINT32_MAX)) {
Expand All @@ -126,19 +123,16 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
return 0;
}

result = algo->generate(status);
total_size = status->last_generated_size;
if (status->last_unsafe) {
return 0;
}
while (total_size < sizeof(uint32_t)) {
result = 0;
total_size = 0;
do {
r = algo->generate(status);
result = (result << status->last_generated_size) | r;
result = (result << (8 * status->last_generated_size)) | r;
total_size += status->last_generated_size;
if (status->last_unsafe) {
return 0;
}
}
} while (total_size < sizeof(uint32_t));
}

return result % umax;
Expand All @@ -150,19 +144,16 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
size_t total_size = 0;
uint32_t count = 0;

result = algo->generate(status);
total_size = status->last_generated_size;
if (status->last_unsafe) {
return 0;
}
if (total_size < sizeof(uint64_t)) {
result = 0;
total_size = 0;
do {
r = algo->generate(status);
result = (result << status->last_generated_size) | r;
result = (result << (8 * status->last_generated_size)) | r;
total_size += status->last_generated_size;
if (status->last_unsafe) {
return 0;
}
}
} while (total_size < sizeof(uint64_t));

/* Special case where no modulus is required */
if (UNEXPECTED(umax == UINT64_MAX)) {
Expand All @@ -188,19 +179,16 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
return 0;
}

result = algo->generate(status);
total_size = status->last_generated_size;
if (status->last_unsafe) {
return 0;
}
while (total_size < sizeof(uint64_t)) {
result = 0;
total_size = 0;
do {
r = algo->generate(status);
result = (result << status->last_generated_size) | r;
result = (result << (8 * status->last_generated_size)) | r;
total_size += status->last_generated_size;
if (status->last_unsafe) {
return 0;
}
}
} while (total_size < sizeof(uint64_t));
}

return result % umax;
Expand Down
20 changes: 20 additions & 0 deletions ext/random/tests/03_randomizer/get_int_user.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
--TEST--
Random: Randomizer: User Engine results are correctly expanded for getInt()
--FILE--
<?php

$randomizer = new \Random\Randomizer (
new class () implements \Random\Engine
{
public function generate(): string
{
return "\x01";
}
}
);

var_dump(bin2hex(pack('V', $randomizer->getInt(0, 0xFFFFFF))));

?>
--EXPECT--
string(8) "01010100"