From ab5491f5058b57256ee306744fcbe5b201abe178 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= <tim@bastelstu.be>
Date: Fri, 22 Jul 2022 11:45:17 +0200
Subject: [PATCH] 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.
---
 ext/random/random.c                              | 8 ++++----
 ext/random/tests/03_randomizer/get_int_user.phpt | 6 ++++--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/ext/random/random.c b/ext/random/random.c
index b2d400caca90c..a3a0a569a8ed1 100644
--- a/ext/random/random.c
+++ b/ext/random/random.c
@@ -92,7 +92,7 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
 	total_size = 0;
 	do {
 		r = algo->generate(status);
-		result = (result << (8 * status->last_generated_size)) | r;
+		result = result | (r << (total_size * 8));
 		total_size += status->last_generated_size;
 		if (status->last_unsafe) {
 			return 0;
@@ -127,7 +127,7 @@ static inline uint32_t rand_range32(const php_random_algo *algo, php_random_stat
 		total_size = 0;
 		do {
 			r = algo->generate(status);
-			result = (result << (8 * status->last_generated_size)) | r;
+			result = result | (r << (total_size * 8));
 			total_size += status->last_generated_size;
 			if (status->last_unsafe) {
 				return 0;
@@ -148,7 +148,7 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
 	total_size = 0;
 	do {
 		r = algo->generate(status);
-		result = (result << (8 * status->last_generated_size)) | r;
+		result = result | (r << (total_size * 8));
 		total_size += status->last_generated_size;
 		if (status->last_unsafe) {
 			return 0;
@@ -183,7 +183,7 @@ static inline uint64_t rand_range64(const php_random_algo *algo, php_random_stat
 		total_size = 0;
 		do {
 			r = algo->generate(status);
-			result = (result << (8 * status->last_generated_size)) | r;
+			result = result | (r << (total_size * 8));
 			total_size += status->last_generated_size;
 			if (status->last_unsafe) {
 				return 0;
diff --git a/ext/random/tests/03_randomizer/get_int_user.phpt b/ext/random/tests/03_randomizer/get_int_user.phpt
index fe0e4492171ce..d6c995314319c 100644
--- a/ext/random/tests/03_randomizer/get_int_user.phpt
+++ b/ext/random/tests/03_randomizer/get_int_user.phpt
@@ -6,9 +6,11 @@ Random: Randomizer: User Engine results are correctly expanded for getInt()
 $randomizer = new \Random\Randomizer (
     new class () implements \Random\Engine
     {
+        public $count = 0;
+
         public function generate(): string
         {
-            return "\x01";
+            return "\x01\x02\x03\x04\x05\x06\x07\x08"[$this->count++];
         }
     }
 );
@@ -17,4 +19,4 @@ var_dump(bin2hex(pack('V', $randomizer->getInt(0, 0xFFFFFF))));
 
 ?>
 --EXPECT--
-string(8) "01010100"
+string(8) "01020300"