Make zend_call_function() failure handling consistent

This API had rather peculiar behavior in case the provided function
is not callable. For some types of failures, it would silently
return FAILURE (e.g. a function does not exist), while for others
(e.g. a class does not exist) it would generate a warning. Depending
on what the calling code does, this can either result in silent
failure or duplicate errors.

This commit switches the contract such that zend_call_function()
always (*) succeeds, though that success might be in the form of
throwing an exception. Calling a non-callable will now consistently
throw an exception.

There are some rare callers that do want to ignore missing methods,
for legacy APIs that are specific with optional methods. For these
use cases a new zend_call_method_if_exists() API is provided.

Calling code generally does not need to explicitly check for and
report zend_call_function() failures -- it can rely on
zend_call_function() having already done so. However, existing
code that does check for failure should continue to work fine.

(*) The only exception to this is if EG(active) being false during
late engine shutdown. This is not relevant to most code, but code
running in destructors and similar may need to be aware of the
possibility.

Author: nikic

Zend/zend_API.h

@@ -646,6 +646,9 @@ ZEND_API void zend_fcall_info_argn(zend_fcall_info *fci, uint32_t argc, ...);
  */
 ZEND_API zend_result zend_fcall_info_call(zend_fcall_info *fci, zend_fcall_info_cache *fcc, zval *retval, zval *args);
 
+/* Can only return FAILURE if EG(active) is false during late engine shutdown.
+ * If the call or call setup throws, EG(exception) will be set and the retval
+ * will be UNDEF. Otherwise, the retval will be a non-UNDEF value. */
 ZEND_API zend_result zend_call_function(zend_fcall_info *fci, zend_fcall_info_cache *fci_cache);
 
 /* Call the provided zend_function with the given params.
@@ -679,6 +682,13 @@ static zend_always_inline void zend_call_known_instance_method_with_1_params(
 ZEND_API void zend_call_known_instance_method_with_2_params(
 		zend_function *fn, zend_object *object, zval *retval_ptr, zval *param1, zval *param2);
 
+/* Call method if it exists. Return FAILURE if method does not exist or call failed.
+ * If FAILURE is returned, retval will be UNDEF. As such, destroying retval unconditionally
+ * is legal. */
+ZEND_API zend_result zend_call_method_if_exists(
+		zend_object *object, zend_string *method_name, zval *retval,
+		uint32_t param_count, zval *params);
+
 ZEND_API zend_result zend_set_hash_symbol(zval *symbol, const char *name, size_t name_length, bool is_ref, int num_symbol_tables, ...);
 
 ZEND_API zend_result zend_delete_global_variable(zend_string *name);

Zend/zend_execute_API.c

@@ -719,7 +719,7 @@ zend_result zend_call_function(zend_fcall_info *fci, zend_fcall_info_cache *fci_
 	}
 
 	if (EG(exception)) {
-		return FAILURE; /* we would result in an instable executor otherwise */
+		return SUCCESS; /* we would result in an instable executor otherwise */
 	}
 
 	ZEND_ASSERT(fci->size == sizeof(zend_fcall_info));
@@ -731,15 +731,14 @@ zend_result zend_call_function(zend_fcall_info *fci, zend_fcall_info_cache *fci_
 			fci_cache = &fci_cache_local;
 		}
 
-		if (!zend_is_callable_ex(&fci->function_name, fci->object, IS_CALLABLE_CHECK_SILENT, NULL, fci_cache, &error)) {
-			if (error) {
-				zend_string *callable_name
-					= zend_get_callable_name_ex(&fci->function_name, fci->object);
-				zend_error(E_WARNING, "Invalid callback %s, %s", ZSTR_VAL(callable_name), error);
-				efree(error);
-				zend_string_release_ex(callable_name, 0);
-			}
-			return FAILURE;
+		if (!zend_is_callable_ex(&fci->function_name, fci->object, 0, NULL, fci_cache, &error)) {
+			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);
+			efree(error);
+			zend_string_release_ex(callable_name, 0);
+			return SUCCESS;
 		}
 
 		ZEND_ASSERT(!error);
@@ -762,7 +761,7 @@ zend_result zend_call_function(zend_fcall_info *fci, zend_fcall_info_cache *fci_
 
 		if (UNEXPECTED(EG(exception))) {
 			zend_vm_stack_free_call_frame(call);
-			return FAILURE;
+			return SUCCESS;
 		}
 	}
 
@@ -789,7 +788,7 @@ zend_result zend_call_function(zend_fcall_info *fci, zend_fcall_info_cache *fci_
 cleanup_args:
 						zend_vm_stack_free_args(call);
 						zend_vm_stack_free_call_frame(call);
-						return FAILURE;
+						return SUCCESS;
 					}
 				}
 			}
@@ -1010,6 +1009,28 @@ ZEND_API void zend_call_known_instance_method_with_2_params(
 	zend_call_known_instance_method(fn, object, retval_ptr, 2, params);
 }
 
+ZEND_API zend_result zend_call_method_if_exists(
+		zend_object *object, zend_string *method_name, zval *retval,
+		uint32_t param_count, zval *params)
+{
+	zend_fcall_info fci;
+	fci.size = sizeof(zend_fcall_info);
+	fci.object = object;
+	ZVAL_STR(&fci.function_name, method_name);
+	fci.retval = retval;
+	fci.param_count = param_count;
+	fci.params = params;
+	fci.named_params = NULL;
+
+	zend_fcall_info_cache fcc;
+	if (!zend_is_callable_ex(&fci.function_name, fci.object, 0, NULL, &fcc, NULL)) {
+		ZVAL_UNDEF(retval);
+		return FAILURE;
+	}
+
+	return zend_call_function(&fci, &fcc);
+}
+
 /* 0-9 a-z A-Z _ \ 0x80-0xff */
 static const uint32_t valid_chars[8] = {
 	0x00000000,

ext/standard/tests/assert/assert_variation.phpt

@@ -47,11 +47,15 @@ var_dump($r2=assert(0 != 0));
 echo"\n";
 
 
-echo "Reset the name of the callback routine to a class method and check that it works\n";
+echo "Reset the name of the callback routine to a class method\n";
 var_dump($rc=assert_options(ASSERT_CALLBACK, "c1"));
 echo "assert_options(ASSERT_CALLBACK) => [".assert_options(ASSERT_CALLBACK)."]\n";
 echo "ini.get(\"assert.callback\") => [".ini_get("assert.callback")."]\n";
-var_dump($r2=assert(0 != 0));
+try {
+    var_dump($r2=assert(0 != 0));
+} catch (Error $e) {
+    echo $e->getMessage(), "\n";
+}
 echo"\n";
 
 echo "Reset callback options to use a class method \n";
@@ -94,11 +98,11 @@ ini.get("assert.callback") => [f2]
 f3 called
 bool(false)
 
-Reset the name of the callback routine to a class method and check that it works
+Reset the name of the callback routine to a class method
 string(2) "f3"
 assert_options(ASSERT_CALLBACK) => [c1]
 ini.get("assert.callback") => [f2]
-bool(false)
+Invalid callback c1, function "c1" not found or invalid function name
 
 Reset callback options to use a class method 
 string(2) "c1"
@@ -110,7 +114,7 @@ array(2) {
 }
 ini.get("assert.callback") => [f2]
 
-Class assertion failed 52, "assert(0 != 0)"
+Class assertion failed 56, "assert(0 != 0)"
 bool(false)
 
 Reset callback options to use an object method 
@@ -122,14 +126,14 @@ array(2) {
 }
 array(2) {
   [0]=>
-  &object(c1)#1 (0) {
+  &object(c1)#2 (0) {
   }
   [1]=>
   string(6) "assert"
 }
 ini.get("assert.callback") => [f2]
 
-Class assertion failed 60, "assert(0 != 0)"
+Class assertion failed 64, "assert(0 != 0)"
 bool(false)
 
 Set callback to something silly

ext/standard/tests/serialize/serialization_objects_008.phpt

@@ -3,14 +3,11 @@ Bad unserialize_callback_func
 --FILE--
 <?php
 ini_set('unserialize_callback_func','Nonexistent');
-$o = unserialize('O:3:"FOO":0:{}');
-var_dump($o);
-echo "Done";
-?>
---EXPECTF--
-Warning: unserialize(): defined (Nonexistent) but not found in %s on line %d
-object(__PHP_Incomplete_Class)#%d (1) {
-  ["__PHP_Incomplete_Class_Name"]=>
-  string(3) "FOO"
+try {
+    unserialize('O:3:"FOO":0:{}');
+} catch (Error $e) {
+    echo $e->getMessage(), "\n";
 }
-Done
+?>
+--EXPECT--
+Invalid callback Nonexistent, function "Nonexistent" not found or invalid function name

ext/standard/user_filters.c

@@ -111,24 +111,18 @@ PHP_RSHUTDOWN_FUNCTION(user_filters)
 static void userfilter_dtor(php_stream_filter *thisfilter)
 {
 	zval *obj = &thisfilter->abstract;
-	zval func_name;
 	zval retval;
 
 	if (Z_ISUNDEF_P(obj)) {
 		/* If there's no object associated then there's nothing to dispose of */
 		return;
 	}
 
-	ZVAL_STRINGL(&func_name, "onclose", sizeof("onclose")-1);
-
-	call_user_function(NULL,
-			obj,
-			&func_name,
-			&retval,
-			0, NULL);
+	zend_string *func_name = zend_string_init("onclose", sizeof("onclose")-1, 0);
+	zend_call_method_if_exists(Z_OBJ_P(obj), func_name, &retval, 0, NULL);
+	zend_string_release(func_name);
 
 	zval_ptr_dtor(&retval);
-	zval_ptr_dtor(&func_name);
 
 	/* kill the object */
 	zval_ptr_dtor(obj);
@@ -243,7 +237,6 @@ static php_stream_filter *user_filter_factory_create(const char *filtername,
 	struct php_user_filter_data *fdat = NULL;
 	php_stream_filter *filter;
 	zval obj;
-	zval func_name;
 	zval retval;
 	size_t len;
 
@@ -319,15 +312,9 @@ static php_stream_filter *user_filter_factory_create(const char *filtername,
 	}
 
 	/* invoke the constructor */
-	ZVAL_STRINGL(&func_name, "oncreate", sizeof("oncreate")-1);
-
-	call_user_function(NULL,
-			&obj,
-			&func_name,
-			&retval,
-			0, NULL);
-
-	zval_ptr_dtor(&func_name);
+	zend_string *func_name = zend_string_init("oncreate", sizeof("oncreate")-1, 0);
+	zend_call_method_if_exists(Z_OBJ(obj), func_name, &retval, 0, NULL);
+	zend_string_release(func_name);
 
 	if (Z_TYPE(retval) != IS_UNDEF) {
 		if (Z_TYPE(retval) == IS_FALSE) {

ext/xml/tests/bug72085.phpt

@@ -6,45 +6,11 @@ xml
 <?php
 $var1 = xml_parser_create_ns();
 xml_set_element_handler($var1, new Exception(""), 4096);
-xml_parse($var1,  str_repeat("<a>", 10));
+try {
+    xml_parse($var1,  str_repeat("<a>", 10));
+} catch (Error $e) {
+    echo $e->getMessage(), "\n";
+}
 ?>
---EXPECTF--
-Warning: Invalid callback Exception::__invoke, no array or string given in %s on line %d
-
-Warning: xml_parse(): Unable to call handler in %s on line %d
-
-Warning: Invalid callback Exception::__invoke, no array or string given in %s on line %d
-
-Warning: xml_parse(): Unable to call handler in %s on line %d
-
-Warning: Invalid callback Exception::__invoke, no array or string given in %s on line %d
-
-Warning: xml_parse(): Unable to call handler in %s on line %d
-
-Warning: Invalid callback Exception::__invoke, no array or string given in %s on line %d
-
-Warning: xml_parse(): Unable to call handler in %s on line %d
-
-Warning: Invalid callback Exception::__invoke, no array or string given in %s on line %d
-
-Warning: xml_parse(): Unable to call handler in %s on line %d
-
-Warning: Invalid callback Exception::__invoke, no array or string given in %s on line %d
-
-Warning: xml_parse(): Unable to call handler in %s on line %d
-
-Warning: Invalid callback Exception::__invoke, no array or string given in %s on line %d
-
-Warning: xml_parse(): Unable to call handler in %s on line %d
-
-Warning: Invalid callback Exception::__invoke, no array or string given in %s on line %d
-
-Warning: xml_parse(): Unable to call handler in %s on line %d
-
-Warning: Invalid callback Exception::__invoke, no array or string given in %s on line %d
-
-Warning: xml_parse(): Unable to call handler in %s on line %d
-
-Warning: Invalid callback Exception::__invoke, no array or string given in %s on line %d
-
-Warning: xml_parse(): Unable to call handler in %s on line %d
+--EXPECT--
+Invalid callback Exception::__invoke, no array or string given