Fix GH-16266: _ZendTestClass::test() segfaults on named parameter

[cmb69]

We must not assume that an internal function has arg_info; that is at least not given for internal classes implementing the magic __call() method. In such cases we cannot look up the parameter offset by name, and raise a fatal error.

[iluuu1994]

IMO, the bug is that zend_test_class_method_get() doesn't set the arg_info in the first place. See zend_get_call_trampoline_func(), which uses some statically allocated memory.

[nielsdos]

Indeed, that's what I meant with that this shouldn't be NULL in first place.

[cmb69]

Pushed a sloppy fix (kept num_args==1, although the function is never called with an argument, and made an argument up, ignoring its type), which is supposed to resolve the issue. After having understood the problem, I'm not sure whether it's worth to fix this at all; at least for stable branches that's a bit doubtful. The only benefit would be to avoid further fuzzing reports.

[iluuu1994]

@cmb69 If the argument is not used, can you set num_args to 0 instead? Is there some other reason this was set to one? zend_test_func doesn't read one. I do think this is worth fixing. It's clearly wrong and the risk of breaking something by modifying zend_test is 0.

[dstogov]

PHP-8.2, PHP-8.3 and PHP-8.4 don't prohibit loading of third-party extensions with functions without arg_info.
They emit E_CORE_WARNING. So PHP-8.2 and PHP-8.3 have to support functions without arg_info.

I'm not sure if this may be changed in PHP-8.4 at this moment.

[iluuu1994]

@dstogov I see, there are indeed a few other instances where we assign NULL to arg_info. I suppose the original fix was appropriate then. Sorry for the confusion, @cmb69. That said, I would still change arg_num to 0, or parse the parameter in the function to show that it is passed correctly.

[cmb69]

Thanks @dstogov for the explanation!

@iluuu1994, I think it makes sense to split this:

• 2dfd2b9 with a new test case not relying on _ZendTestClass; and that would be a "proper" bug fix
• Improve _ZendTestClass with proper arg_num and arg_info (minor fix; could maybe target PHP-8.4)

I'll work on both issues soon.

[iluuu1994]

@cmb69 That makes sense to me.

[cmb69]

Regarding the segfault

First, this cannot only happen for the dynamic case (as in the bug report):

php-src/Zend/zend_execute.c

Lines 5307 to 5308 in edc94a8

	for (uint32_t i = 0; i < num_args; i++) {
	zend_internal_arg_info *arg_info = &fbc->internal_function.arg_info[i];

but also during compile time:

php-src/Zend/zend_compile.c

Lines 3712 to 3713 in edc94a8

	for (uint32_t i = 0; i < fn->common.num_args; i++) {
	zend_internal_arg_info *arg_info = &fn->internal_function.arg_info[i];

Second, usually functions without arginfo are registered with arg_num=0:

php-src/Zend/zend_API.c

Lines 3012 to 3013 in edc94a8

	internal_function->arg_info = NULL;
	internal_function->num_args = 0;

So to trigger a segfault

one needs to try hard

 ext/zend_test/test.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/ext/zend_test/test.c b/ext/zend_test/test.c
index 8989413317d..5388a04895e 100644
--- a/ext/zend_test/test.c
+++ b/ext/zend_test/test.c
@@ -373,6 +373,16 @@ static const zend_function_entry ext_function_legacy[] = {
 	ZEND_FE_END
 };
 
+static ZEND_FUNCTION(zend_test_without_arginfo)
+{
+	ZEND_PARSE_PARAMETERS_NONE();
+}
+
+static const zend_function_entry ext_function_without_arginfo[] = {
+	ZEND_FE(zend_test_without_arginfo, NULL)
+	ZEND_FE_END
+};
+
 /* Call a method on a class or object using zend_call_method() */
 static ZEND_FUNCTION(zend_call_method)
 {
@@ -1014,6 +1024,12 @@ PHP_MINIT_FUNCTION(zend_test)
 
 	zend_register_functions(NULL, ext_function_legacy, NULL, EG(current_module)->type);
 
+	zend_register_functions(NULL, ext_function_without_arginfo, NULL, EG(current_module)->type);
+	zend_string *bad_func_name = ZSTR_INIT_LITERAL("zend_test_without_arginfo", 0);
+	zend_internal_function *bad_func = (zend_internal_function *) zend_hash_find_ptr(CG(function_table), bad_func_name);
+	zend_string_release(bad_func_name);
+	bad_func->num_args = 1;
+
 	// Loading via dl() not supported with the observer API
 	if (type != MODULE_TEMPORARY) {
 		REGISTER_INI_ENTRIES();

<?php
zend_test_without_arginfo(unknown: 42);

However, if one tries hard enough they can break the engine arbitrarily. I think an actual runtime guard is overkill. Instead we can help extension developers by inserting

some sanity checks

 Zend/zend_compile.c | 1 +
 Zend/zend_execute.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index 8662e188aaa..5cf42edd4f5 100644
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -3495,6 +3495,7 @@ static uint32_t zend_get_arg_num(zend_function *fn, zend_string *arg_name) {
 			}
 		}
 	} else {
+		ZEND_ASSERT(fn->common.num_args == 0 || fn->internal_function.arg_info);
 		for (uint32_t i = 0; i < fn->common.num_args; i++) {
 			zend_internal_arg_info *arg_info = &fn->internal_function.arg_info[i];
 			size_t len = strlen(arg_info->name);
diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c
index 7b7d618c37d..752ba119dee 100644
--- a/Zend/zend_execute.c
+++ b/Zend/zend_execute.c
@@ -4945,6 +4945,7 @@ static zend_always_inline uint32_t zend_get_arg_offset_by_name(
 			}
 		}
 	} else {
+		ZEND_ASSERT(num_args == 0 || fbc->internal_function.arg_info);
 		for (uint32_t i = 0; i < num_args; i++) {
 			zend_internal_arg_info *arg_info = &fbc->internal_function.arg_info[i];
 			size_t len = strlen(arg_info->name);

[iluuu1994]

So, it seems we have accidentally required arg_info for functions with params when introducing named parameters. Given nobody has complained about this, an assert indeed seems enough. But preferably, it should be added to the code registering internal functions. Otherwise, extension developers can miss the error if they don't test with named parameters.

[cmb69]

So, it seems we have accidentally required arg_info for functions with params when introducing named parameters.

No, not really. The point is, if there is no arginfo, num_args is usually zero. And if this is given, the loop won't run, so the non-existent arginfo will never be accessed. That means everything is fine for the normal case.

proper function without arginfo, but 1 required arg per ZPP

 ext/zend_test/test.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/ext/zend_test/test.c b/ext/zend_test/test.c
index 8989413317d..61ceb203408 100644
--- a/ext/zend_test/test.c
+++ b/ext/zend_test/test.c
@@ -373,6 +373,22 @@ static const zend_function_entry ext_function_legacy[] = {
 	ZEND_FE_END
 };
 
+static ZEND_FUNCTION(zend_test_without_arginfo)
+{
+	zend_string *source_string = NULL;
+
+	ZEND_PARSE_PARAMETERS_START(1, 1)
+		Z_PARAM_STR(source_string)
+	ZEND_PARSE_PARAMETERS_END();
+
+	RETURN_STR(source_string);
+}
+
+static const zend_function_entry ext_function_without_arginfo[] = {
+	ZEND_FE(zend_test_without_arginfo, NULL)
+	ZEND_FE_END
+};
+
 /* Call a method on a class or object using zend_call_method() */
 static ZEND_FUNCTION(zend_call_method)
 {
@@ -1014,6 +1030,8 @@ PHP_MINIT_FUNCTION(zend_test)
 
 	zend_register_functions(NULL, ext_function_legacy, NULL, EG(current_module)->type);
 
+	zend_register_functions(NULL, ext_function_without_arginfo, NULL, EG(current_module)->type);
+
 	// Loading via dl() not supported with the observer API
 	if (type != MODULE_TEMPORARY) {
 		REGISTER_INI_ENTRIES();

<?php
var_dump(zend_test_without_arginfo("hello"));
var_dump(zend_test_without_arginfo(unknown: 42));

outputs for release builds:

Warning: Missing arginfo for zend_test_without_arginfo() in Unknown on line 0
string(5) "hello"

Fatal error: Uncaught Error: Unknown named parameter $unknown in %s:3

For debug builds, this fails, though, due to error "Arginfo / zpp mismatch during call of zend_test_without_arginfo()".

Only cases like _ZendTestClass::test() or the contrieved example I've posted above ("one needs to try hard") are broken, because there is no arginfo, but num_args>0.

[iluuu1994]

Ok, I see now that num_args is automatically set to 0 when arg_info is missing. So this should indeed be a non-issue, and is isolated to this case where we build the trampoline function manually (and incorrectly).

php-src/Zend/zend_API.c

Lines 3009 to 3014 in 4b97e7b

	zend_error(E_CORE_WARNING, "Missing arginfo for %s%s%s()",
	scope ? ZSTR_VAL(scope->name) : "", scope ? "::" : "", ptr->fname);
	internal_function->arg_info = NULL;
	internal_function->num_args = 0;
	internal_function->required_num_args = 0;

[cmb69]

See #16538 as follow-up with the sanity checks I've mentioned above.