Skip to content

Fix GH-21362: ReflectionMethod::invoke/invokeArgs() rejects different Closure instances#21366

Open
iliaal wants to merge 1 commit intophp:masterfrom
iliaal:fix/gh-21362-reflection-closure-invoke
Open

Fix GH-21362: ReflectionMethod::invoke/invokeArgs() rejects different Closure instances#21366
iliaal wants to merge 1 commit intophp:masterfrom
iliaal:fix/gh-21362-reflection-closure-invoke

Conversation

@iliaal
Copy link
Contributor

@iliaal iliaal commented Mar 6, 2026

Summary

Fixes #21362

ReflectionMethod::invokeArgs() (and invoke()) for Closure::__invoke() incorrectly accepted any Closure object, not just the one the ReflectionMethod was created from.

Root cause

All Closure objects share a single zend_ce_closure class entry. The instanceof_function() check in reflection_method_invoke() compares class entries, so it always sees zend_ce_closure == zend_ce_closure and passes -- it cannot distinguish between different Closure instances.

Fix

Two changes to ext/reflection/php_reflection.c:

  1. instantiate_reflection_method(): Store the original Closure object in intern->obj via ZVAL_OBJ_COPY() when reflecting Closure::__invoke(). Previously this was a no-op (/* do nothing, mptr already set */).

  2. reflection_method_invoke(): After the existing instanceof_function check passes for Closures, add an identity check (Z_OBJ_P(object) != Z_OBJ(intern->obj)) to reject different Closure instances.

Test

ext/reflection/tests/gh21362.phpt covers:

  • invokeArgs() with the correct Closure (should work)
  • invokeArgs() with a different Closure (should throw ReflectionException)
  • invoke() with a different Closure (should throw ReflectionException)

@TimWolla
Copy link
Member

TimWolla commented Mar 6, 2026

I did not test, but based on reading the diff I think this is not quite right, yet. Instead of comparing the objects, we should compare the inner op_array, because for:

$closures = [];

for ($i = 0; $i < 3; $i++) {
    $closures[] = function () use ($i) { return $i; };
}

$m = new ReflectionMethod(array_first($closures), '__invoke');
foreach ($closures as $closure) {
    var_dump($m->invoke($closure));
}

I expect this to successfully dump 0, 1, 2. All the closure instances are "of the same class". If that already works: Great, please just add that test case.

…ent Closure instances

ReflectionMethod::invokeArgs() (and invoke()) for Closure::__invoke()
incorrectly accepted any Closure object, not just the one the
ReflectionMethod was created from. This happened because all Closures
share a single zend_ce_closure class entry, so the instanceof_function()
check always passed.

Fix: store the original Closure object in intern->obj during
ReflectionMethod construction, then compare object identity in
reflection_method_invoke() to reject different Closure instances.

Closes phpGH-21362
@iliaal iliaal force-pushed the fix/gh-21362-reflection-closure-invoke branch from 1581e11 to 277d301 Compare March 6, 2026 22:36
@iliaal
Copy link
Contributor Author

iliaal commented Mar 6, 2026

Good catch -- the object identity check was too strict. Updated the comparison to use op_array.opcodes instead, so closures from the same source (e.g. created in a loop with different use bindings) are accepted, while closures from different source locations are still rejected.

Added the loop test case you suggested. All 515 reflection tests pass.

P.S. Thanks for the feedback, my php internals are a bit rusty 😆

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.

ReflectionMethod::invokeArgs() for Closure::__invoke() accepts objects from different Closures

2 participants