Fix undefined behaviour in unpack()

atoi()'s return value is actually undefined when an underflow or
overflow occurs. For example on 32-bit on my system the overflow test
which inputs "h2147483648" results in repetitions==2147483647 and on
64-bit this gives repetitions==-2147483648. The reason the test works on
32-bit is because there's a second undefined behaviour problem:
in case 'h' when repetitions==2147483647, we add 1 and divide by 2.
This is signed-wrap undefined behaviour and accidentally triggers the
overflow check like we wanted to.

Avoid all this trouble and use strtol with explicit error checking.

This also fixes a semantic bug where repetitions==INT_MAX would result
in the overflow check to trigger, even though there is no overflow.

Closes GH-10943.

Author: ndossche

NEWS

@@ -75,6 +75,7 @@ PHP                                                                        NEWS
     (apache2)). (nielsdos)
   . Fixed oss-fuzz #57392 (Buffer-overflow in php_fgetcsv() with \0 delimiter
     and enclosure). (ilutov)
+  . Fixed undefined behaviour in unpack(). (nielsdos)
 
 16 Mar 2023, PHP 8.1.17
 

ext/standard/pack.c

@@ -750,7 +750,16 @@ PHP_FUNCTION(unpack)
 			c = *format;
 
 			if (c >= '0' && c <= '9') {
-				repetitions = atoi(format);
+				errno = 0;
+				long tmp = strtol(format, NULL, 10);
+				/* There is not strtoi. We have to check the range ourselves.
+				 * With 32-bit long the INT_{MIN,MAX} are useless because long == int, but with 64-bit they do limit us to 32-bit. */
+				if (errno || tmp < INT_MIN || tmp > INT_MAX) {
+					php_error_docref(NULL, E_WARNING, "Type %c: integer overflow", type);
+					zend_array_destroy(Z_ARR_P(return_value));
+					RETURN_FALSE;
+				}
+				repetitions = tmp;
 
 				while (formatlen > 0 && *format >= '0' && *format <= '9') {
 					format++;
@@ -800,7 +809,7 @@ PHP_FUNCTION(unpack)
 
 			case 'h':
 			case 'H':
-				size = (repetitions > 0) ? (repetitions + (repetitions % 2)) / 2 : repetitions;
+				size = (repetitions > 0) ? ((unsigned int) repetitions + 1) / 2 : repetitions;
 				repetitions = 1;
 				break;
 
@@ -865,12 +874,6 @@ PHP_FUNCTION(unpack)
 				RETURN_THROWS();
 		}
 
-		if (size != 0 && size != -1 && size < 0) {
-			php_error_docref(NULL, E_WARNING, "Type %c: integer overflow", type);
-			zend_array_destroy(Z_ARR_P(return_value));
-			RETURN_FALSE;
-		}
-
 
 		/* Do actual unpacking */
 		for (i = 0; i != repetitions; i++ ) {

ext/standard/tests/strings/gh10940.phpt

@@ -0,0 +1,9 @@
+--TEST--
+Test unpacking at the 32-bit integer limit
+--FILE--
+<?php
+$a = pack("AAAAAAAAAAAA", 1,2,3,4,5,6,7,8,9,10,11,12);
+unpack('h2147483647', $a);
+?>
+--EXPECTF--
+Warning: unpack(): Type h: not enough input, need 1073741824, have 12 in %s on line %d