Skip to content

Conversation

iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Sep 16, 2025

Don't access fbc->op_array.refcount on internal function. Don't attempt to cache ZEND_ACC_USER_ARG_INFO at all, which is only used in zend_get_closure_invoke_method(). This may reuse arg_info from a temporary closure, and hence caching would also be unsafe.

See https://github.com/php/php-src/actions/runs/17751635609/job/50447500518.
Introduced in #19654.

Don't access fbc->op_array.refcount on internal function. Don't attempt to cache
ZEND_ACC_USER_ARG_INFO at all, which is only used in
zend_get_closure_invoke_method(). This may reuse arg_info from a temporary
closure, and hence caching would also be unsafe.
@iluuu1994 iluuu1994 requested a review from nielsdos September 16, 2025 11:42
@iluuu1994 iluuu1994 requested a review from dstogov as a code owner September 16, 2025 11:42
@iluuu1994 iluuu1994 changed the base branch from master to PHP-8.3 September 16, 2025 11:42
@iluuu1994 iluuu1994 requested review from arnaud-lb and removed request for nielsdos September 16, 2025 13:35
@iluuu1994
Copy link
Member Author

iluuu1994 commented Sep 16, 2025

Hmm, actually I guess this also needs a fix for the if (fbc->common.fn_flags & ZEND_ACC_VARIADIC) { branch. I suppose we could just check for ZEND_ACC_USER_ARG_INFO there.

Edit: I don't think this should be necessary. zend_get_closure_invoke_method() will only be called with an empty dummy cache slot from what I can see.

@arnaud-lb
Copy link
Member

No you were right, I think you need to check for ZEND_ACC_USER_ARG_INFO in the variadic case as well. The cache slot is from the caller:

function invoke($closure) {
    $closure->__invoke(a: 1);
}
invoke(eval('return function(...$a) { var_dump($a); };'));
invoke(eval('return function(...$b) { var_dump($b); };'));

@iluuu1994
Copy link
Member Author

You're right. There are two calls to zend_handle_named_arg(), once from the VM for the call to Closure::__invoke(), and one from zend_call_function() (where the cache slot is empty). Somehow I missed the first call yesterday, and only considered the second one.

@iluuu1994
Copy link
Member Author

@arnaud-lb Check again please. 🙂

This may happen when a closure with a variadic parameter is called with
$closure->__invoke().
@iluuu1994 iluuu1994 force-pushed the zend_get_arg_offset_by_name-uouv branch from d3a1ca0 to d5d29a6 Compare September 17, 2025 11:36
@iluuu1994
Copy link
Member Author

Thanks for catching this. Hopefully it's ok now.

Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@iluuu1994 iluuu1994 closed this in 6eb3fae Sep 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants