Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ PHP NEWS
. Added ReflectionConstant::inNamespace(). (Khaled Alam)
. Added ReflectionProperty::isReadable() and ReflectionProperty::isWritable().
(ilutov)
. Fixed bug GH-21362 (ReflectionMethod::invoke/invokeArgs() did not verify
Closure instance identity for Closure::__invoke()). (Ilia Alshanetsky)

- Session:
. Fixed bug 71162 (updateTimestamp never called when session data is empty).
Expand Down
21 changes: 20 additions & 1 deletion ext/reflection/php_reflection.c
Original file line number Diff line number Diff line change
Expand Up @@ -3306,7 +3306,9 @@ static void instantiate_reflection_method(INTERNAL_FUNCTION_PARAMETERS, bool is_
&& memcmp(lcname, ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1) == 0
&& (mptr = zend_get_closure_invoke_method(orig_obj)) != NULL)
{
/* do nothing, mptr already set */
/* Store the original closure object so we can validate it in invoke/invokeArgs.
* Each closure has a unique __invoke signature, so we must reject different closures. */
ZVAL_OBJ_COPY(&intern->obj, orig_obj);
} else if ((mptr = zend_hash_str_find_ptr(&ce->function_table, lcname, method_name_len)) == NULL) {
efree(lcname);
zend_throw_exception_ex(reflection_exception_ptr, 0,
Expand Down Expand Up @@ -3441,6 +3443,23 @@ static void reflection_method_invoke(INTERNAL_FUNCTION_PARAMETERS, int variadic)
_DO_THROW("Given object is not an instance of the class this method was declared in");
RETURN_THROWS();
}

/* For Closure::__invoke(), closures from different source locations have
* different signatures, so we must reject those. However, closures created
* from the same source (e.g. in a loop) share the same op_array and should
* be allowed. Compare the underlying function pointer via op_array. */
if (obj_ce == zend_ce_closure && !Z_ISUNDEF(intern->obj)
&& Z_OBJ_P(object) != Z_OBJ(intern->obj)) {
const zend_function *orig_func = zend_get_closure_method_def(Z_OBJ(intern->obj));
const zend_function *given_func = zend_get_closure_method_def(Z_OBJ_P(object));
if (orig_func->op_array.opcodes != given_func->op_array.opcodes) {
Copy link
Member

@ndossche ndossche Mar 8, 2026

Choose a reason for hiding this comment

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

This code is wrong for internal closures because they don't have an op_array.
Intuitively, the code also seems a bit convoluted, it feels wrong to have to store the object.

Anyway, here is something that shouldn't be allowed most likely, and even if it were allowed then the access to op_array is still wrong (only allowed for userland):

<?php

$closure = var_dump(...);
$closure2 = print_r(...);

$rm = new ReflectionMethod($closure, '__invoke');
$rm->invoke($closure2, 1);

Copy link
Member

Choose a reason for hiding this comment

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

@iliaal thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a valid point, I made a follow-up PR #21389 to address this issue (along with corresponding test).

if (!variadic) {
efree(params);
}
_DO_THROW("Given Closure is not the same as the reflected Closure");
RETURN_THROWS();
}
}
}
/* Copy the zend_function when calling via handler (e.g. Closure::__invoke()) */
callback = _copy_function(mptr);
Expand Down
53 changes: 53 additions & 0 deletions ext/reflection/tests/gh21362.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
--TEST--
GH-21362 (ReflectionMethod::invokeArgs() for Closure::__invoke() accepts objects from different Closures)
--FILE--
<?php

$c1 = function ($foo, $bar) {
echo "c1: foo={$foo}, bar={$bar}\n";
};

$c2 = function ($bar, $foo) {
echo "c2: foo={$foo}, bar={$bar}\n";
};

$m = new ReflectionMethod($c1, '__invoke');

// invokeArgs with the correct Closure should work
$m->invokeArgs($c1, ['foo' => 'FOO', 'bar' => 'BAR']);

// invokeArgs with a different Closure should throw
try {
$m->invokeArgs($c2, ['foo' => 'FOO', 'bar' => 'BAR']);
echo "No exception thrown\n";
} catch (ReflectionException $e) {
echo $e->getMessage() . "\n";
}

// invoke with a different Closure should also throw
try {
$m->invoke($c2, 'FOO', 'BAR');
echo "No exception thrown\n";
} catch (ReflectionException $e) {
echo $e->getMessage() . "\n";
}

// Closures from the same source (e.g. loop) share the same op_array
// and should be allowed to invoke each other's ReflectionMethod
$closures = [];
for ($i = 0; $i < 3; $i++) {
$closures[] = function () use ($i) { return $i; };
}

$m2 = new ReflectionMethod($closures[0], '__invoke');
foreach ($closures as $closure) {
var_dump($m2->invoke($closure));
}
?>
--EXPECT--
c1: foo=FOO, bar=BAR
Given Closure is not the same as the reflected Closure
Given Closure is not the same as the reflected Closure
int(0)
int(1)
int(2)
Loading