Skip to content

Zend: refactor zend_parse_arg_impl() to return zend_expected_type #23052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Girgias wants to merge 1 commit into
base: master
Choose a base branch
from
+73 −68
Open

Zend: refactor zend_parse_arg_impl() to return zend_expected_type #23052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 72 additions & 67 deletions Zend/zend_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,7 +829,7 @@ ZEND_API bool ZEND_FASTCALL zend_parse_arg_str_or_long_slow(zval *arg, zend_stri
}
/* }}} */

static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec, char **error, uint32_t arg_num) /* {{{ */
static zend_expected_type zend_parse_arg_impl(zval *arg, va_list *va, const char **spec, char **error, uint32_t arg_num) /* {{{ */
{
const char *spec_walk = *spec;
char c = *spec_walk++;
Expand Down Expand Up @@ -863,7 +863,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
}

if (!zend_parse_arg_long(arg, p, is_null, check_null, arg_num)) {
return check_null ? "?int" : "int";
return check_null ? Z_EXPECTED_LONG_OR_NULL : Z_EXPECTED_LONG;
}
}
break;
Expand All @@ -878,7 +878,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
}

if (!zend_parse_arg_double(arg, p, is_null, check_null, arg_num)) {
return check_null ? "?float" : "float";
return check_null ? Z_EXPECTED_DOUBLE_OR_NULL : Z_EXPECTED_DOUBLE;
}
}
break;
Expand All @@ -888,7 +888,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
zval **p = va_arg(*va, zval **);

if (!zend_parse_arg_number(arg, p, check_null, arg_num)) {
return check_null ? "int|float|null" : "int|float";
return check_null ? Z_EXPECTED_NUMBER_OR_NULL : Z_EXPECTED_NUMBER;
}
}
break;
Expand All @@ -898,7 +898,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
char **p = va_arg(*va, char **);
size_t *pl = va_arg(*va, size_t *);
if (!zend_parse_arg_string(arg, p, pl, check_null, arg_num)) {
return check_null ? "?string" : "string";
return check_null ? Z_EXPECTED_STRING_OR_NULL : Z_EXPECTED_STRING;
}
}
break;
Expand All @@ -908,12 +908,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
char **p = va_arg(*va, char **);
size_t *pl = va_arg(*va, size_t *);
if (!zend_parse_arg_path(arg, p, pl, check_null, arg_num)) {
if (Z_TYPE_P(arg) == IS_STRING) {
zend_spprintf(error, 0, "must not contain any null bytes");
return "";
} else {
return check_null ? "?string" : "string";
}
Comment on lines -911 to -916

@kamil-tekiela kamil-tekiela Aug 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correct that this was just so that the user is informed about NUL bytes in their path string? Because the NUL bytes are already handled in zend_str_has_nul_byte but the error message would be generic, right? Now, you handle it with zend_wrong_parameter_type_error.

@Girgias Girgias Aug 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is the purpose of this ZPP specifier, and has always been this way.

You get the same error message either way, fast ZPP only uses zend_wrong_parameter_type_error to generate error messages.

@kamil-tekiela kamil-tekiela Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but one thing I don't understand. Previously if (Z_TYPE_P(arg) == IS_STRING) { was after ZVAL_DEREF(arg) but now in the zend_wrong_parameter_type_error, you have this check but I cannot find any ZVAL_DEREF(arg) up the call stack. Am I missing something?

Do we have a unit test for something like this:

$s = "abc\0def";
$ref = &$s;
fopen($ref, 'r');

EDIT: So I asked Claude to explain this to me and it told me that it's not possible for zend_parse_arg/zend_parse_arg_impl to receive a reference, so there is no need for dereference checks. I can't say that I understand why it's in one place and not in the other, but at least it doesn't cause a regression.

@Girgias Girgias Aug 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not interested in a "review" from Claude.

@kamil-tekiela kamil-tekiela Aug 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, to avoid any misunderstanding, the review is mine. I only used Claude to help me understand the code. I am genuinely asking why we have ZVAL_DEREF(arg) but not in the other place.

@arnaud-lb arnaud-lb Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

diff --git a/ext/zend_test/test.c b/ext/zend_test/test.c
index a880c09fc1f..fe670890f7d 100644
--- a/ext/zend_test/test.c
+++ b/ext/zend_test/test.c
@@ -1969,3 +1969,14 @@ static PHP_FUNCTION(zend_test_gh19792)
        zend_error(E_WARNING, "a warning");
        zend_throw_error(NULL, "an exception");
 }
+
+static PHP_FUNCTION(zend_test_recv_path_by_ref_old_zpp)
+{
+       zend_string *path;
+
+       if (zend_parse_parameters(ZEND_NUM_ARGS(), "p", &path) == FAILURE) {
+               RETURN_THROWS();
+       }
+
+       RETURN_STR(path);
+}
diff --git a/ext/zend_test/test.stub.php b/ext/zend_test/test.stub.php
index 7c96ea176a8..6c8ffd4ca08 100644
--- a/ext/zend_test/test.stub.php
+++ b/ext/zend_test/test.stub.php
@@ -392,6 +392,8 @@ function zend_test_uri_parser(string $uri, string $parser): array { }
 
     /** @compile-time-eval */
     function zend_test_gh19792(): void {}
+
+    function zend_test_recv_path_by_ref_old_zpp(string &$path): void {}
 }
 
 namespace ZendTestNS {
$ php -r '$a = "foo\0bar"; zend_test_recv_path_by_ref_old_zpp($a);'

Fatal error: Uncaught TypeError: zend_test_recv_path_by_ref_old_zpp(): Argument #1 ($path) must be of type string, string given in Command line code:1

@Girgias Girgias Aug 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even allow this sort of usage from ZPP? Every single by-ref param I've ever seen uses the Z_PARAM_ZVAL/z specifiers as I don't know how you'd assign a new value to the reference?

@arnaud-lb arnaud-lb Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point indeed!

@Girgias Girgias Aug 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess what we probably should do is add an assertion that the zval is not IS_REF (nor IS_INDIRECT) except for the 'z' specifier.

@arnaud-lb arnaud-lb Aug 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this would make sense

return check_null ? Z_EXPECTED_PATH_OR_NULL : Z_EXPECTED_PATH;
}
}
break;
Expand All @@ -922,12 +917,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
{
zend_string **str = va_arg(*va, zend_string **);
if (!zend_parse_arg_path_str(arg, str, check_null, arg_num)) {
if (Z_TYPE_P(arg) == IS_STRING) {
zend_spprintf(error, 0, "must not contain any null bytes");
return "";
} else {
return check_null ? "?string" : "string";
}
return check_null ? Z_EXPECTED_PATH_OR_NULL : Z_EXPECTED_PATH;
}
}
break;
Expand All @@ -936,7 +926,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
{
zend_string **str = va_arg(*va, zend_string **);
if (!zend_parse_arg_str(arg, str, check_null, arg_num)) {
return check_null ? "?string" : "string";
return check_null ? Z_EXPECTED_STRING_OR_NULL : Z_EXPECTED_STRING;
}
}
break;
Expand All @@ -951,7 +941,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
}

if (!zend_parse_arg_bool(arg, p, is_null, check_null, arg_num)) {
return check_null ? "?bool" : "bool";
return check_null ? Z_EXPECTED_BOOL_OR_NULL : Z_EXPECTED_BOOL;
}
}
break;
Expand All @@ -961,7 +951,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
zval **p = va_arg(*va, zval **);

if (!zend_parse_arg_resource(arg, p, check_null)) {
return check_null ? "resource or null" : "resource";
return check_null ? Z_EXPECTED_RESOURCE_OR_NULL : Z_EXPECTED_RESOURCE;
}
}
break;
Expand All @@ -972,7 +962,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
zval **p = va_arg(*va, zval **);

if (!zend_parse_arg_array(arg, p, check_null, c == 'A')) {
return check_null ? "?array" : "array";
return check_null ? Z_EXPECTED_ARRAY_OR_NULL : Z_EXPECTED_ARRAY;
}
}
break;
Expand All @@ -983,7 +973,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
HashTable **p = va_arg(*va, HashTable **);

if (!zend_parse_arg_array_ht(arg, p, check_null, c == 'H', separate)) {
return check_null ? "?array" : "array";
return check_null ? Z_EXPECTED_ARRAY_OR_NULL : Z_EXPECTED_ARRAY;
}
}
break;
Expand All @@ -993,7 +983,7 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
zval **p = va_arg(*va, zval **);

if (!zend_parse_arg_object(arg, p, NULL, check_null)) {
return check_null ? "?object" : "object";
return check_null ? Z_EXPECTED_OBJECT_OR_NULL : Z_EXPECTED_OBJECT;
}
}
break;
Expand All @@ -1005,50 +995,42 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec

if (!zend_parse_arg_object(arg, p, ce, check_null)) {
if (ce) {
if (check_null) {
zend_spprintf(error, 0, "must be of type ?%s, %s given", ZSTR_VAL(ce->name), zend_zval_value_name(arg));
return "";
} else {
return ZSTR_VAL(ce->name);
}
} else {
return check_null ? "?object" : "object";
*error = ZSTR_VAL(ce->name);
}
return check_null ? Z_EXPECTED_OBJECT_OR_NULL : Z_EXPECTED_OBJECT;
}
}
break;

case 'C':
{
zend_class_entry *lookup, **pce = va_arg(*va, zend_class_entry **);
zend_class_entry *ce_base = *pce;
zend_class_entry **pce = va_arg(*va, zend_class_entry **);
const zend_class_entry *ce_base = *pce;

if (check_null && Z_TYPE_P(arg) == IS_NULL) {
*pce = NULL;
break;
}
if (!try_convert_to_string(arg)) {
*pce = NULL;
return ""; /* try_convert_to_string() throws an exception */
}

if ((lookup = zend_lookup_class(Z_STR_P(arg))) == NULL) {
zend_string *class_name = NULL;
if (!zend_parse_arg_str(arg, &class_name, check_null, arg_num)) {
*pce = NULL;
} else {
*pce = lookup;
return check_null ? Z_EXPECTED_STRING_OR_NULL : Z_EXPECTED_STRING;
}

*pce = zend_lookup_class(class_name);
if (ce_base) {
if ((!*pce || !instanceof_function(*pce, ce_base))) {
zend_spprintf(error, 0, "must be a class name derived from %s%s, %s given",
ZSTR_VAL(ce_base->name), check_null ? " or null" : "", Z_STRVAL_P(arg));
*pce = NULL;
return "";
return check_null ? Z_EXPECTED_OBJECT_OR_CLASS_NAME_OR_NULL : Z_EXPECTED_OBJECT_OR_CLASS_NAME;
}
}
if (!*pce) {
zend_spprintf(error, 0, "must be a valid class name%s, %s given",
check_null ? " or null" : "", Z_STRVAL_P(arg));
return "";
return check_null ? Z_EXPECTED_OBJECT_OR_CLASS_NAME_OR_NULL : Z_EXPECTED_OBJECT_OR_CLASS_NAME;
}
break;

Expand All @@ -1060,19 +1042,11 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
{
zend_fcall_info *fci = va_arg(*va, zend_fcall_info *);
zend_fcall_info_cache *fcc = va_arg(*va, zend_fcall_info_cache *);
char *is_callable_error = NULL;
if (EXPECTED(zend_parse_arg_func(arg, fci, fcc, check_null, &is_callable_error, c == 'f'))) {
ZEND_ASSERT(!is_callable_error);
if (EXPECTED(zend_parse_arg_func(arg, fci, fcc, check_null, error, c == 'f'))) {
ZEND_ASSERT(!*error);
Comment on lines +1045 to +1046

@arnaud-lb arnaud-lb Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit / not for this PR: Both zend_parse_arg_func() and zend_is_callable_at_frame() zero *error. I'm wondering if we could move this responsibility to the caller. In the case of zend_parse_arg_impl(), *error is always NULL before calling zend_parse_arg_func() and zend_is_callable_at_frame().

break;
}

if (is_callable_error) {
zend_spprintf(error, 0, "must be a valid callback%s, %s", check_null ? " or null" : "", is_callable_error);
efree(is_callable_error);
return "";
} else {
return check_null ? "a valid callback or null" : "a valid callback";
}
return check_null ? Z_EXPECTED_FUNC_OR_NULL : Z_EXPECTED_FUNC;
}

case 'z':
Expand All @@ -1088,37 +1062,68 @@ static const char *zend_parse_arg_impl(zval *arg, va_list *va, const char **spec
ZEND_ASSERT(0 && "ZPP modifier no longer supported");
ZEND_FALLTHROUGH;
default:
return "unknown";
ZEND_ASSERT(false && "Unknown ZPP modifier");
}

*spec = spec_walk;

return NULL;
return Z_EXPECTED_LAST;
}
/* }}} */

static zend_result zend_parse_arg(uint32_t arg_num, zval *arg, va_list *va, const char **spec, int flags) /* {{{ */
{
const char *expected_type = NULL;
char *error = NULL;

expected_type = zend_parse_arg_impl(arg, va, spec, &error, arg_num);
if (expected_type) {
zend_expected_type expected_type = zend_parse_arg_impl(arg, va, spec, &error, arg_num);
if (expected_type != Z_EXPECTED_LAST) {
if (EG(exception)) {
return FAILURE;
}
if (!(flags & ZEND_PARSE_PARAMS_QUIET) && (*expected_type || error)) {

if (!(flags & ZEND_PARSE_PARAMS_QUIET)) {
/* More complex error, can only happen for:
* Objects of a specific class
* Z_EXPECTED_OBJECT
* Z_EXPECTED_OBJECT_OR_NULL
* Class names
* Z_EXPECTED_OBJECT_OR_CLASS_NAME
* Z_EXPECTED_OBJECT_OR_CLASS_NAME_OR_NULL
* Functions
* Z_EXPECTED_FUNC
* Z_EXPECTED_FUNC_OR_NULL
*/
if (error) {
if (strcmp(error, "must not contain any null bytes") == 0) {
zend_argument_value_error(arg_num, "%s", error);
} else {
zend_argument_type_error(arg_num, "%s", error);
switch (expected_type) {
case Z_EXPECTED_OBJECT:
/* DO NOT FREE error: it's a pointer to ZSTR_VAL(ce->name) */

@arnaud-lb arnaud-lb Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit / not for this PR: We should unify ownership of error and maybe make it a zend_string*.

@Girgias Girgias Aug 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this was really confusing to me. But error is currently how the "advanced callable errors" are done, so I'll need to think how to change this to a zend_string.

zend_wrong_parameter_class_error(arg_num, error, arg);
break;
case Z_EXPECTED_OBJECT_OR_NULL:
/* DO NOT FREE error: it's a pointer to ZSTR_VAL(ce->name) */
zend_wrong_parameter_class_or_null_error(arg_num, error, arg);
break;
case Z_EXPECTED_FUNC:
/* error is freed by zend_wrong_callback_error() */
zend_wrong_callback_error(arg_num, error);
break;
case Z_EXPECTED_FUNC_OR_NULL:
/* error is freed by zend_wrong_callback_or_null_error() */
zend_wrong_callback_or_null_error(arg_num, error);
break;
case Z_EXPECTED_OBJECT_OR_CLASS_NAME:
case Z_EXPECTED_OBJECT_OR_CLASS_NAME_OR_NULL:
zend_argument_type_error(arg_num, "%s", error);
efree(error);
break;
default:
ZEND_UNREACHABLE();
}
efree(error);
} else {
zend_argument_type_error(arg_num, "must be of type %s, %s given", expected_type, zend_zval_value_name(arg));
}
} else if (error) {
zend_wrong_parameter_type_error(arg_num, expected_type, arg);
} else if (error
/* DO NOT FREE error when it's a pointer to ZSTR_VAL(ce->name) */
&& expected_type != Z_EXPECTED_OBJECT && expected_type != Z_EXPECTED_OBJECT_OR_NULL) {
efree(error);
}

Expand Down
2 changes: 1 addition & 1 deletion ext/mysqli/tests/mysqli_fetch_object_oo.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ require_once 'skipifconnectfailure.inc';
require_once 'clean_table.inc';
?>
--EXPECT--
Error: Object of class mysqli could not be converted to string
TypeError: mysqli_result::fetch_object(): Argument #1 ($class) must be of type string, mysqli given
ArgumentCountError: mysqli_result::fetch_object() expects at most 2 arguments, 3 given
TypeError: mysqli_result::fetch_object(): Argument #2 ($constructor_args) must be of type array, null given
ArgumentCountError: Too few arguments to function mysqli_fetch_object_construct::__construct(), 1 passed and exactly 2 expected
Expand Down
Loading