Fix ReflectionMethod::invoke() for first class callables

[iliaal]

Summary

Follow-up to GH-21366. The closure identity check accessed op_array.opcodes unconditionally, but internal closures created via first-class callable syntax (e.g. var_dump(...)) use internal_function, not op_array. This is undefined behavior and will crash under ASAN/debug builds.

• Check func->type == ZEND_USER_FUNCTION before accessing op_array.opcodes
• For internal closures, compare the zend_function pointer directly
• Added test coverage for internal closure validation

Reported by @ndossche in #21366 (comment)

[ndossche]

On first sight this seems right. One other thing that I notice is that op_array.opcodes isn't necessarily a unique address unless the ZEND_ACC_IMMUTABLE flag is set. However, the current code is likely fine because you hold onto the object so that op_array.opcodes will not alias to another function.
I'll have another look tomorrow.

[iliaal]

I think it should be fine, but another pair of eyes never hurts :-) Thanks!

[iluuu1994]

However, the current code is likely fine because you hold onto the object so that op_array.opcodes will not alias to another function.

I don't think that's possible for the reason you've mentioned. We're keeping the op_array alive through zend_op_array->refcount. zend_closure_free_storage() then calls destroy_op_array().

[TimWolla]

@iliaal I've taken the liberty to push into your branch, since this was easier than explaining the change.

AFAIK the only way for a Closure to be an internal function is by using first class callables (“fake closure”) and this is the check we should make, since for userland fake Closures we should also compare scope and function name. The “proper closure” check can then remain the OPcode structure comparison.

[ndossche]

This still looks wrong:

<?php

class Broken {
    function foo(){}
}

$func = (new Broken)->foo(...);

$rm = new ReflectionMethod(new Broken, 'foo');

$rm->invoke($func);

[iliaal]

@ndossche After Tim's change it looks to ok as far as I can tell, the code throws "ReflectionException: Given object is not an instance of the class this method was declared in".

As far as I can tell that is correct behavior. intern->obj is only set when the ReflectionMethod is constructed for Closure::__invoke (the ce == zend_ce_closure guard in instantiate_reflection_method). For new ReflectionMethod(new Broken, 'foo'), intern->obj remains UNDEF, so the closure validation block is skipped entirely. The instanceof_function check then correctly rejects the Closure as not being an instance of Broken.

[TimWolla]

The instanceof_function check then correctly rejects the Closure as not being an instance of Broken.

Or to put it another way: That has always been rejected, as per https://3v4l.org/4UiEW. The original issue was the check not rejecting enough.

[iliaal]

The instanceof_function check then correctly rejects the Closure as not being an instance of Broken.

Or to put it another way: That has always been rejected, as per https://3v4l.org/4UiEW. The original issue was the check not rejecting enough.

Thanks for the help on the patch BTW :)