Release call trampolines in zpp fcc

When using zpp 'f' or Z_PARAM_FUNC, if the fcc points to a call trampoline release it immediately and force zend_call_function to refetch it. This may require additional callability checks if __call is used, but avoids the need to carefully free fcc values in all internal functions -- in some cases this is not simple, as a type error might be triggered by a later argument in the same zpp call. This fixes oss-fuzz #25390. Closes GH-6073.
php · Sep 4, 2020 · 2e21818 · 2e21818
1 parent c0d6b05
commit 2e21818
Show file tree

Hide file tree

Showing 6 changed files with 30 additions and 18 deletions.
diff --git a/Zend/zend_API.c b/Zend/zend_API.c
@@ -825,6 +825,10 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
 
 				if (zend_fcall_info_init(arg, 0, fci, fcc, NULL, &is_callable_error) == SUCCESS) {
 					ZEND_ASSERT(!is_callable_error);
+					/* Release call trampolines: The function may not get called, in which case
+					 * the trampoline will leak. Force it to be refetched during
+					 * zend_call_function instead. */
+					zend_release_fcall_info_cache(fcc);
 					break;
 				}
 
@@ -2979,8 +2983,8 @@ ZEND_API void zend_release_fcall_info_cache(zend_fcall_info_cache *fcc) {
 			zend_string_release_ex(fcc->function_handler->common.function_name, 0);
 		}
 		zend_free_trampoline(fcc->function_handler);
+		fcc->function_handler = NULL;
 	}
-	fcc->function_handler = NULL;
 }
 
 static zend_always_inline bool zend_is_callable_check_func(int check_flags, zval *callable, zend_execute_data *frame, zend_fcall_info_cache *fcc, bool strict_class, char **error) /* {{{ */

diff --git a/Zend/zend_API.h b/Zend/zend_API.h
@@ -2004,6 +2004,10 @@ static zend_always_inline bool zend_parse_arg_func(zval *arg, zend_fcall_info *d
 	} else if (UNEXPECTED(zend_fcall_info_init(arg, 0, dest_fci, dest_fcc, NULL, error) != SUCCESS)) {
 		return 0;
 	}
+	/* Release call trampolines: The function may not get called, in which case
+	 * the trampoline will leak. Force it to be refetched during
+	 * zend_call_function instead. */
+	zend_release_fcall_info_cache(dest_fcc);
 	return 1;
 }
 

diff --git a/Zend/zend_builtin_functions.c b/Zend/zend_builtin_functions.c
@@ -1198,7 +1198,6 @@ ZEND_FUNCTION(set_error_handler)
 
 	ZVAL_COPY(&EG(user_error_handler), &(fci.function_name));
 	EG(user_error_handler_error_reporting) = (int)error_type;
-	zend_release_fcall_info_cache(&fcc);
 }
 /* }}} */
 
@@ -1254,7 +1253,6 @@ ZEND_FUNCTION(set_exception_handler)
 	}
 
 	ZVAL_COPY(&EG(user_exception_handler), &(fci.function_name));
-	zend_release_fcall_info_cache(&fcc);
 }
 /* }}} */
 

diff --git a/ext/spl/php_spl.c b/ext/spl/php_spl.c
@@ -519,6 +519,13 @@ PHP_FUNCTION(spl_autoload_register)
 
 	/* If first arg is not null */
 	if (ZEND_FCI_INITIALIZED(fci)) {
+		if (!fcc.function_handler) {
+			/* Call trampoline has been cleared by zpp. Refetch it, because we want to deal
+			 * with it outselves. It is important that it is not refetched on every call,
+			 * because calls may occur from different scopes. */
+			zend_is_callable_ex(&fci.function_name, NULL, 0, NULL, &fcc, NULL);
+		}
+
 		if (fcc.function_handler->type == ZEND_INTERNAL_FUNCTION &&
 			fcc.function_handler->internal_function.handler == zif_spl_autoload_call) {
 			zend_argument_value_error(1, "must not be the spl_autoload_call() function");
@@ -566,7 +573,7 @@ PHP_FUNCTION(spl_autoload_unregister)
 		RETURN_THROWS();
 	}
 
-	if (zend_string_equals_literal(
+	if (fcc.function_handler && zend_string_equals_literal(
 			fcc.function_handler->common.function_name, "spl_autoload_call")) {
 		/* Don't destroy the hash table, as we might be iterating over it right now. */
 		zend_hash_clean(SPL_G(autoload_functions));

diff --git a/ext/standard/array.c b/ext/standard/array.c
@@ -992,7 +992,6 @@ static int php_array_user_compare(Bucket *a, Bucket *b) /* {{{ */
 	BG(user_compare_fci_cache) = empty_fcall_info_cache; \
 
 #define PHP_ARRAY_CMP_FUNC_RESTORE() \
-	zend_release_fcall_info_cache(&BG(user_compare_fci_cache)); \
 	BG(user_compare_fci) = old_user_compare_fci; \
 	BG(user_compare_fci_cache) = old_user_compare_fci_cache; \
 
@@ -1515,7 +1514,6 @@ PHP_FUNCTION(array_walk)
 	);
 
 	php_array_walk(array, userdata, 0);
-	zend_release_fcall_info_cache(&BG(array_walk_fci_cache));
 	BG(array_walk_fci) = orig_array_walk_fci;
 	BG(array_walk_fci_cache) = orig_array_walk_fci_cache;
 	RETURN_TRUE;
@@ -1545,7 +1543,6 @@ PHP_FUNCTION(array_walk_recursive)
 	);
 
 	php_array_walk(array, userdata, 1);
-	zend_release_fcall_info_cache(&BG(array_walk_fci_cache));
 	BG(array_walk_fci) = orig_array_walk_fci;
 	BG(array_walk_fci_cache) = orig_array_walk_fci_cache;
 	RETURN_TRUE;
@@ -5951,7 +5948,6 @@ PHP_FUNCTION(array_reduce)
 	htbl = Z_ARRVAL_P(input);
 
 	if (zend_hash_num_elements(htbl) == 0) {
-		zend_release_fcall_info_cache(&fci_cache);
 		return;
 	}
 
@@ -5973,8 +5969,6 @@ PHP_FUNCTION(array_reduce)
 			RETURN_NULL();
 		}
 	} ZEND_HASH_FOREACH_END();
-
-	zend_release_fcall_info_cache(&fci_cache);
 }
 /* }}} */
 
@@ -6002,7 +5996,6 @@ PHP_FUNCTION(array_filter)
 
 	if (zend_hash_num_elements(Z_ARRVAL_P(array)) == 0) {
 		RETVAL_EMPTY_ARRAY();
-		zend_release_fcall_info_cache(&fci_cache);
 		return;
 	}
 	array_init(return_value);
@@ -6064,8 +6057,6 @@ PHP_FUNCTION(array_filter)
 		}
 		zval_add_ref(operand);
 	} ZEND_HASH_FOREACH_END();
-
-	zend_release_fcall_info_cache(&fci_cache);
 }
 /* }}} */
 
@@ -6092,7 +6083,6 @@ PHP_FUNCTION(array_map)
 		int ret;
 
 		if (Z_TYPE(arrays[0]) != IS_ARRAY) {
-			zend_release_fcall_info_cache(&fci_cache);
 			zend_argument_type_error(2, "must be of type array, %s given", zend_zval_type_name(&arrays[0]));
 			RETURN_THROWS();
 		}
@@ -6101,7 +6091,6 @@ PHP_FUNCTION(array_map)
 		/* Short-circuit: if no callback and only one array, just return it. */
 		if (!ZEND_FCI_INITIALIZED(fci) || !maxlen) {
 			ZVAL_COPY(return_value, &arrays[0]);
-			zend_release_fcall_info_cache(&fci_cache);
 			return;
 		}
 
@@ -6126,8 +6115,6 @@ PHP_FUNCTION(array_map)
 				zend_hash_index_add_new(Z_ARRVAL_P(return_value), num_key, &result);
 			}
 		} ZEND_HASH_FOREACH_END();
-
-		zend_release_fcall_info_cache(&fci_cache);
 	} else {
 		uint32_t *array_pos = (HashPosition *)ecalloc(n_arrays, sizeof(HashPosition));
 
@@ -6219,7 +6206,6 @@ PHP_FUNCTION(array_map)
 			}
 
 			efree(params);
-			zend_release_fcall_info_cache(&fci_cache);
 		}
 		efree(array_pos);
 	}

diff --git a/ext/standard/tests/array/bug74345.phpt b/ext/standard/tests/array/bug74345.phpt
@@ -18,6 +18,11 @@ try {
 } catch (Error $e) {
     echo $e->getMessage(), "\n";
 }
+try {
+    array_map($cb, null, null);
+} catch (Error $e) {
+    echo $e->getMessage(), "\n";
+}
 array_filter([], $cb);
 array_reduce([], $cb);
 
@@ -26,8 +31,16 @@ array_walk($array, $cb);
 array_walk_recursive($array, $cb);
 usort($array, $cb);
 
+try {
+    preg_replace_callback('/./', $cb, new stdClass);
+} catch (Error $e) {
+    echo $e->getMessage(), "\n";
+}
+
 ?>
 ===DONE===
 --EXPECT--
 array_map(): Argument #2 ($array1) must be of type array, null given
+array_map(): Argument #2 ($array1) must be of type array, null given
+preg_replace_callback(): Argument #3 ($subject) must be of type string|array, stdClass given
 ===DONE===