Zend: always populate error pointer on failure of zend_is_callable() (#24435)

* Zend: always populate error pointer on failure

Most of the code base expects an error to be set if a char* is provided, as this used to be always the case. It no longer was the case since the partially supported callables were deprecates as the deprecation could be promoted to an exception in which case we skipped allocating a char*.

The simple fix, allocate a char* even in case of an exception so callers don't need to check if it still NULL. This fixes some error messages at the same time.

* tree: simplify error handling for failed zend_is_callable() calls

* docs: clarify docs for zend_is_callable()

---------

Co-authored-by: Arnaud Le Blanc <365207+arnaud-lb@users.noreply.github.com>

Zend/tests/error_handler/gh_21699.phpt

@@ -24,7 +24,7 @@ Stack trace:
 #1 %s(%d): bar->test()
 #2 {main}
 
-Next TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, (null) in %s:%d
+Next TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, cannot use "self"/"parent"/"static" in callables in %s:%d
 Stack trace:
 #0 %s(%d): bar->test()
 #1 {main}

Zend/tests/error_handler/gh_21699_parent.phpt

@@ -25,7 +25,7 @@ Stack trace:
 #1 %s(%d): Child->test()
 #2 {main}
 
-Next TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, (null) in %s:%d
+Next TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, cannot use "self"/"parent"/"static" in callables in %s:%d
 Stack trace:
 #0 %s(%d): Child->test()
 #1 {main}

Zend/tests/error_handler/gh_21699_static.phpt

@@ -24,7 +24,7 @@ Stack trace:
 #1 %s(%d): bar->test()
 #2 {main}
 
-Next TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, (null) in %s:%d
+Next TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, cannot use "self"/"parent"/"static" in callables in %s:%d
 Stack trace:
 #0 %s(%d): bar->test()
 #1 {main}

Zend/tests/gh16799.phpt

@@ -19,7 +19,7 @@ Stack trace:
 #1 %s(%d): Test::test()
 #2 {main}
 
-Next TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, (null) in %s:%d
+Next TypeError: call_user_func(): Argument #1 ($callback) must be a valid callback, cannot use "self"/"parent"/"static" in callables in %s:%d
 Stack trace:
 #0 %s(%d): Test::test()
 #1 {main}

Zend/zend_API.c

@@ -3817,6 +3817,11 @@ static bool zend_is_callable_check_class(zend_string *name, zend_class_entry *sc
 	ZSTR_ALLOCA_FREE(lcname, use_heap);
 	/* User error handlers may throw from deprecations above; do not report callable as valid. */
 	if (UNEXPECTED(EG(exception))) {
+		/* We allocate an error string that will not be used as callers of zend_is_callable() expect an
+		 * error to be assigned on failure */
+		if (error && *error == NULL) {
+			*error = estrdup("cannot use \"self\"/\"parent\"/\"static\" in callables");
+		}
 		return false;
 	}
 	return ret;

Zend/zend_execute_API.c

@@ -848,13 +848,11 @@ zend_result zend_call_function(zend_fcall_info *fci, zend_fcall_info_cache *fci_
 		}
 
 		if (!zend_is_callable_ex(&fci->function_name, fci->object, 0, NULL, fci_cache, &error)) {
+			ZEND_ASSERT(error && "Should have error if not callable");
 			if (EG(exception)) {
-				if (error) {
-					efree(error);
-				}
+				efree(error);
 				return SUCCESS;
 			}
-			ZEND_ASSERT(error && "Should have error if not callable");
 			zend_string *callable_name
 				= zend_get_callable_name_ex(&fci->function_name, fci->object);
 			zend_throw_error(NULL, "Invalid callback %s, %s", ZSTR_VAL(callable_name), error);

docs/source/core/data-structures/fci-fcc.rst

@@ -80,8 +80,9 @@ There are some functions that do not require the use of either the FCI or the FC
       -  Check if the ``callable`` zval is a PHP callable. Returns true if it is, false otherwise.
          ``fcc`` is an out parameter constructing the FCC struct, see below for details. ``error``
          is an optional out parameter holding the error message if ``callable`` is not a PHP
-         callable. Note: the signature used to be ``bool zend_is_callable(zval *callable, uint32_t
-         check_flags, zend_string **callable_name)`` prior to PHP 8.7.
+         callable. If ``error`` is non-NULL and false is returned it is *always* populated. Note:
+         the signature used to be ``bool zend_is_callable(zval *callable, uint32_t check_flags,
+         zend_string **callable_name)`` prior to PHP 8.7.
 
    -  -  .. code:: c
 

ext/curl/interface.c

@@ -1656,7 +1656,7 @@ static bool php_curl_set_callable_handler(zend_fcall_info_cache *const handler_f
 	}
 
 	char *error = NULL;
-	if (UNEXPECTED(!zend_is_callable_ex(callable, /* object */ NULL, /* check_flags */ 0, /* callable_name */ NULL, handler_fcc, /* error */ &error))) {
+	if (UNEXPECTED(!zend_is_callable(callable, handler_fcc, /* error */ &error))) {
 		if (!EG(exception)) {
 			zend_argument_type_error(2 + !is_array_config, "must be a valid callback for option %s, %s", option_name, error);
 		}

ext/curl/multi.c

@@ -485,7 +485,7 @@ static bool _php_curl_multi_setopt(php_curlm *mh, zend_long option, zval *zvalue
 			}
 
 			char *error_str = NULL;
-			if (UNEXPECTED(!zend_is_callable_ex(zvalue, /* object */ NULL, /* check_flags */ 0, /* callable_name */ NULL, &mh->handlers.server_push, /* error */ &error_str))) {
+			if (UNEXPECTED(!zend_is_callable(zvalue, &mh->handlers.server_push, &error_str))) {
 				if (!EG(exception)) {
 					zend_argument_type_error(2, "must be a valid callback for option CURLMOPT_PUSHFUNCTION, %s", error_str);
 				}

ext/openssl/xp_ssl.c

@@ -2012,13 +2012,9 @@ static zend_result php_openssl_validate_and_allocate_psk_callback(
 	char *is_callable_error = NULL;
 	zend_fcall_info_cache fcc = {0};
 	if (!zend_is_callable(callable, &fcc, &is_callable_error)) {
-		if (is_callable_error) {
-			zend_type_error("%s must be a valid callback, %s",
-					callback_name, is_callable_error);
-			efree(is_callable_error);
-		} else {
-			zend_type_error("%s must be a valid callback", callback_name);
-		}
+		zend_type_error("%s must be a valid callback, %s",
+				callback_name, is_callable_error);
+		efree(is_callable_error);
 		return FAILURE;
 	}
 
@@ -2135,12 +2131,8 @@ static zend_result php_openssl_setup_server_early_data(php_stream *stream,
 		char *is_callable_error = NULL;
 		zend_fcall_info_cache fcc = {0};
 		if (!zend_is_callable(val, &fcc, &is_callable_error)) {
-			if (is_callable_error) {
-				zend_type_error("early_data_cb must be a valid callback, %s", is_callable_error);
-				efree(is_callable_error);
-			} else {
-				zend_type_error("early_data_cb must be a valid callback");
-			}
+			zend_type_error("early_data_cb must be a valid callback, %s", is_callable_error);
+			efree(is_callable_error);
 			return FAILURE;
 		}
 
@@ -2323,12 +2315,8 @@ static zend_result php_openssl_validate_and_allocate_session_callback(
 	/* Validate callable */
 	zend_fcall_info_cache fcc;
 	if (!zend_is_callable(callable, &fcc, &is_callable_error)) {
-		if (is_callable_error) {
-			zend_type_error("%s must be a valid callback, %s", callback_name, is_callable_error);
-			efree(is_callable_error);
-		} else {
-			zend_type_error("%s must be a valid callback", callback_name);
-		}
+		zend_type_error("%s must be a valid callback, %s", callback_name, is_callable_error);
+		efree(is_callable_error);
 		return FAILURE;
 	}
 

ext/pdo/pdo_stmt.c

@@ -1137,12 +1137,8 @@ static bool pdo_get_fcc_from_zval(zend_fcall_info_cache *fcc, zval *callable) {
 
 	char *is_callable_error = NULL;
 	if (!zend_is_callable(callable, fcc, &is_callable_error)) {
-		if (is_callable_error) {
-			zend_type_error("%s", is_callable_error);
-			efree(is_callable_error);
-		} else {
-			zend_type_error("User-supplied function must be a valid callback");
-		}
+		zend_type_error("%s", is_callable_error);
+		efree(is_callable_error);
 		return false;
 	}
 	return true;

main/streams/stream_errors.c

@@ -370,10 +370,8 @@ static void php_stream_call_error_handler(const zval *handler, zval *errors_arra
 	char *is_callable_error = NULL;
 
 	if (!zend_is_callable(handler, &fcc, &is_callable_error)) {
-		if (is_callable_error) {
-			zend_type_error("stream error handler must be a valid callback, %s", is_callable_error);
-			efree(is_callable_error);
-		}
+		zend_type_error("stream error handler must be a valid callback, %s", is_callable_error);
+		efree(is_callable_error);
 		return;
 	}