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

[iliaal]

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]

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.

[iliaal]

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 😆

[DanielEScherzer]

Thanks, this looks good now. Please add to NEWS/UPGRADING and I'll merge

[iliaal]

Added NEWS entry, don't think UPGRADED needs updating

[DanielEScherzer]

Squashed and merged, thanks!

[ndossche]

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);

[DanielEScherzer]

@iliaal thoughts?

[iliaal]

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