Fix GH-10008: Narrowing occurred during type inference of ZEND_ADD_ARRAY_ELEMENT

nielsdos · nielsdos · commit e72fc12058dc · 2023-09-30T00:08:32.000+02:00
This test triggers narrowing for two ops: first ZEND_ADD_ARRAY_ELEMENT, and then ZEND_ASSIGN. The type inference happens in the following order: 1) The ZEND_ADD_ARRAY_ELEMENT infers type 0x40e04080 (packed flag is set), arr_type=0 at this point because it hasn't been set by ZEND_INIT_ARRAY yet. 2) The ZEND_INIT_ARRAY infers type 0x40804080 3) The ZEND_ADD_ARRAY_ELEMENT infers type 0x40e04080, arr_type=0x40804080, which does not have the packed flag set while the existing result of ZEND_ADD_ARRAY_ELEMENT has the packed flag set. This seems to occur because of the phi node introduced by the while loop. If I remove the loop the problem goes away. As Arnaud noted, this seems to be caused by a too wide type inference for arr_type==0. We should keep the invariant that if x>=y then key_type(x) >= key_type(y). If we write the possible results down in a table we get: ``` arr_type resulting key type --------------- -------------------------------------------------------------------------- HASH_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH PACKED_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) HASH || PACKED -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) 0 -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) ``` As we can see, `HASH_ONLY > 0` but `MAY_BE_ARRAY_NUMERIC_HASH < MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED`, which violates the invariant. Instead if we modify the zero case to have MAY_BE_ARRAY_NUMERIC_HASH instead, we get the following table which satisfies the invariant. ``` arr_type resulting key type --------------- -------------------------------------------------------------------------- HASH_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH PACKED_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) HASH || PACKED -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) 0 -> MAY_BE_ARRAY_NUMERIC_HASH ``` Broke in 1ffbb73. Closes GH-10294.
diff --git a/NEWS b/NEWS
@@ -8,6 +8,8 @@ PHP                                                                        NEWS
   . Fixed bug GH-12215 (Module entry being overwritten causes type errors in
     ext/dom). (nielsdos)
   . Fixed bug GH-12273 (__builtin_cpu_init check). (Freaky)
+  . Fixed bug GH-10008 (Narrowing occurred during type inference of
+    ZEND_ADD_ARRAY_ELEMENT). (nielsdos, arnaud-lb)
 
 - CType:
   . Fixed bug GH-11997 (ctype_alnum 5 times slower in PHP 8.1 or greater).
diff --git a/Zend/Optimizer/zend_inference.c b/Zend/Optimizer/zend_inference.c
@@ -1926,6 +1926,21 @@ ZEND_API uint32_t zend_array_element_type(uint32_t t1, zend_uchar op_type, int w
 	return tmp;
 }
 
+static zend_always_inline uint32_t assign_long_dim_array_result_type(uint32_t arr_type)
+{
+	/* Rules:
+	 * HASH_ONLY      -> MAY_BE_ARRAY_NUMERIC_HASH
+	 * PACKED_ONLY    -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG)
+	 * HASH || PACKED -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG)
+	 * 0              -> MAY_BE_ARRAY_NUMERIC_HASH
+	 */
+	if (MAY_BE_PACKED(arr_type)) {
+		return MAY_BE_ARRAY_KEY_LONG;
+	} else {
+		return MAY_BE_ARRAY_NUMERIC_HASH;
+	}
+}
+
 static uint32_t assign_dim_array_result_type(
 		uint32_t arr_type, uint32_t dim_type, uint32_t value_type, zend_uchar dim_op_type) {
 	uint32_t tmp = 0;
@@ -1939,13 +1954,13 @@ static uint32_t assign_dim_array_result_type(
 			if (arr_type & (MAY_BE_UNDEF|MAY_BE_NULL|MAY_BE_FALSE)) {
 				tmp |= MAY_BE_ARRAY_PACKED;
 			}
-			tmp |= MAY_BE_HASH_ONLY(arr_type) ? MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;
+			tmp |= assign_long_dim_array_result_type(arr_type);
 		} else {
 			if (dim_type & (MAY_BE_LONG|MAY_BE_FALSE|MAY_BE_TRUE|MAY_BE_RESOURCE|MAY_BE_DOUBLE)) {
 				if (arr_type & (MAY_BE_UNDEF|MAY_BE_NULL|MAY_BE_FALSE)) {
 					tmp |= MAY_BE_ARRAY_PACKED;
 				}
-				tmp |= MAY_BE_HASH_ONLY(arr_type) ? MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;
+				tmp |= assign_long_dim_array_result_type(arr_type);
 			}
 			if (dim_type & MAY_BE_STRING) {
 				tmp |= MAY_BE_ARRAY_KEY_STRING;
@@ -1954,7 +1969,7 @@ static uint32_t assign_dim_array_result_type(
 					if (arr_type & (MAY_BE_UNDEF|MAY_BE_NULL|MAY_BE_FALSE)) {
 						tmp |= MAY_BE_ARRAY_PACKED;
 					}
-					tmp |= MAY_BE_HASH_ONLY(arr_type) ? MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;
+					tmp |= assign_long_dim_array_result_type(arr_type);
 				}
 			}
 			if (dim_type & (MAY_BE_UNDEF|MAY_BE_NULL)) {
@@ -3254,17 +3269,15 @@ static zend_always_inline int _zend_update_type_info(
 							key_type |= MAY_BE_ARRAY_PACKED;
 						}
 						if (t1 & MAY_BE_ARRAY) {
-							key_type |= MAY_BE_HASH_ONLY(t1) ?
-								MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;
+							key_type |= assign_long_dim_array_result_type(t1);
 						}
 					} else {
 						if (t2 & (MAY_BE_LONG|MAY_BE_FALSE|MAY_BE_TRUE|MAY_BE_RESOURCE|MAY_BE_DOUBLE)) {
 							if (t1 & (MAY_BE_UNDEF|MAY_BE_NULL|MAY_BE_FALSE)) {
 								key_type |= MAY_BE_ARRAY_PACKED;
 							}
 							if (t1 & MAY_BE_ARRAY) {
-								key_type |= MAY_BE_HASH_ONLY(t1) ?
-									MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;
+								key_type |= assign_long_dim_array_result_type(t1);
 						    }
 						}
 						if (t2 & MAY_BE_STRING) {
@@ -3275,8 +3288,7 @@ static zend_always_inline int _zend_update_type_info(
 									key_type |= MAY_BE_ARRAY_PACKED;
 								}
 								if (t1 & MAY_BE_ARRAY) {
-									key_type |= MAY_BE_HASH_ONLY(t1) ?
-										MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;
+									key_type |= assign_long_dim_array_result_type(t1);
 							    }
 							}
 						}
diff --git a/ext/opcache/tests/opt/gh10008.phpt b/ext/opcache/tests/opt/gh10008.phpt
@@ -0,0 +1,30 @@
+--TEST--
+GH-10008 (Narrowing occurred during type inference of ZEND_ADD_ARRAY_ELEMENT)
+--INI--
+opcache.enable=1
+opcache.enable_cli=1
+opcache.optimization_level=0x20
+--EXTENSIONS--
+opcache
+--FILE--
+<?php
+
+function test()
+{
+    $bool = true;
+    for ($i = 0; $i < 10; $i++) {
+        if ($bool !== true) {
+            $string_key = "a";
+            // The following line triggers narrowing during type inference of ZEND_ADD_ARRAY_ELEMENT.
+            $array = ["b" => $bool, $string_key => 123];
+        }
+
+        $bool = false;
+    }
+}
+
+echo "Done\n";
+
+?>
+--EXPECT--
+Done

Original file line number	Diff line number	Diff line change
`@@ -1926,6 +1926,21 @@ ZEND_API uint32_t zend_array_element_type(uint32_t t1, zend_uchar op_type, int w`
`1926`	`1926`	`return tmp;`
`1927`	`1927`	`}`
`1928`	`1928`
	`1929`	`+static zend_always_inline uint32_t assign_long_dim_array_result_type(uint32_t arr_type)`
	`1930`	`+{`
	`1931`	`+ /* Rules:`
	`1932`	`+ * HASH_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH`
	`1933`	`+ * PACKED_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH \| MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG)`
	`1934`	`+ * HASH \|\| PACKED -> MAY_BE_ARRAY_NUMERIC_HASH \| MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG)`
	`1935`	`+ * 0 -> MAY_BE_ARRAY_NUMERIC_HASH`
	`1936`	`+ */`
	`1937`	`+ if (MAY_BE_PACKED(arr_type)) {`
	`1938`	`+ return MAY_BE_ARRAY_KEY_LONG;`
	`1939`	`+ } else {`
	`1940`	`+ return MAY_BE_ARRAY_NUMERIC_HASH;`
	`1941`	`+ }`
	`1942`	`+}`
	`1943`	`+`
`1929`	`1944`	`static uint32_t assign_dim_array_result_type(`
`1930`	`1945`	`uint32_t arr_type, uint32_t dim_type, uint32_t value_type, zend_uchar dim_op_type) {`
`1931`	`1946`	`uint32_t tmp = 0;`
`@@ -1939,13 +1954,13 @@ static uint32_t assign_dim_array_result_type(`
`1939`	`1954`	`if (arr_type & (MAY_BE_UNDEF\|MAY_BE_NULL\|MAY_BE_FALSE)) {`
`1940`	`1955`	`tmp \|= MAY_BE_ARRAY_PACKED;`
`1941`	`1956`	`}`
`1942`		`- tmp \|= MAY_BE_HASH_ONLY(arr_type) ? MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;`
	`1957`	`+ tmp \|= assign_long_dim_array_result_type(arr_type);`
`1943`	`1958`	`} else {`
`1944`	`1959`	`if (dim_type & (MAY_BE_LONG\|MAY_BE_FALSE\|MAY_BE_TRUE\|MAY_BE_RESOURCE\|MAY_BE_DOUBLE)) {`
`1945`	`1960`	`if (arr_type & (MAY_BE_UNDEF\|MAY_BE_NULL\|MAY_BE_FALSE)) {`
`1946`	`1961`	`tmp \|= MAY_BE_ARRAY_PACKED;`
`1947`	`1962`	`}`
`1948`		`- tmp \|= MAY_BE_HASH_ONLY(arr_type) ? MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;`
	`1963`	`+ tmp \|= assign_long_dim_array_result_type(arr_type);`
`1949`	`1964`	`}`
`1950`	`1965`	`if (dim_type & MAY_BE_STRING) {`
`1951`	`1966`	`tmp \|= MAY_BE_ARRAY_KEY_STRING;`
`@@ -1954,7 +1969,7 @@ static uint32_t assign_dim_array_result_type(`
`1954`	`1969`	`if (arr_type & (MAY_BE_UNDEF\|MAY_BE_NULL\|MAY_BE_FALSE)) {`
`1955`	`1970`	`tmp \|= MAY_BE_ARRAY_PACKED;`
`1956`	`1971`	`}`
`1957`		`- tmp \|= MAY_BE_HASH_ONLY(arr_type) ? MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;`
	`1972`	`+ tmp \|= assign_long_dim_array_result_type(arr_type);`
`1958`	`1973`	`}`
`1959`	`1974`	`}`
`1960`	`1975`	`if (dim_type & (MAY_BE_UNDEF\|MAY_BE_NULL)) {`
`@@ -3254,17 +3269,15 @@ static zend_always_inline int _zend_update_type_info(`
`3254`	`3269`	`key_type \|= MAY_BE_ARRAY_PACKED;`
`3255`	`3270`	`}`
`3256`	`3271`	`if (t1 & MAY_BE_ARRAY) {`
`3257`		`- key_type \|= MAY_BE_HASH_ONLY(t1) ?`
`3258`		`- MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;`
	`3272`	`+ key_type \|= assign_long_dim_array_result_type(t1);`
`3259`	`3273`	`}`
`3260`	`3274`	`} else {`
`3261`	`3275`	`if (t2 & (MAY_BE_LONG\|MAY_BE_FALSE\|MAY_BE_TRUE\|MAY_BE_RESOURCE\|MAY_BE_DOUBLE)) {`
`3262`	`3276`	`if (t1 & (MAY_BE_UNDEF\|MAY_BE_NULL\|MAY_BE_FALSE)) {`
`3263`	`3277`	`key_type \|= MAY_BE_ARRAY_PACKED;`
`3264`	`3278`	`}`
`3265`	`3279`	`if (t1 & MAY_BE_ARRAY) {`
`3266`		`- key_type \|= MAY_BE_HASH_ONLY(t1) ?`
`3267`		`- MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;`
	`3280`	`+ key_type \|= assign_long_dim_array_result_type(t1);`
`3268`	`3281`	`}`
`3269`	`3282`	`}`
`3270`	`3283`	`if (t2 & MAY_BE_STRING) {`
`@@ -3275,8 +3288,7 @@ static zend_always_inline int _zend_update_type_info(`
`3275`	`3288`	`key_type \|= MAY_BE_ARRAY_PACKED;`
`3276`	`3289`	`}`
`3277`	`3290`	`if (t1 & MAY_BE_ARRAY) {`
`3278`		`- key_type \|= MAY_BE_HASH_ONLY(t1) ?`
`3279`		`- MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG;`
	`3291`	`+ key_type \|= assign_long_dim_array_result_type(t1);`
`3280`	`3292`	`}`
`3281`	`3293`	`}`
`3282`	`3294`	`}`