Skip to content

Commit 4561e92

Browse files
iliaalTimWolla
andauthored
Fix ReflectionMethod::invoke() for first class callables (php#21389)
* Fix ReflectionMethod::invoke() crash with internal closures The closure identity check added in phpGH-21366 accessed op_array.opcodes unconditionally, but internal closures (e.g. var_dump(...)) use internal_function, not op_array. This caused undefined behavior when comparing closures created via first-class callable syntax on internal functions. Check the function type first: compare op_array.opcodes for user closures, compare the function pointer directly for internal closures. * Fix internal closure comparison and expand test coverage The previous comparison (orig_func == given_func) could never match for internal closures since zend_get_closure_method_def() returns a pointer to each closure's embedded copy. Compare function_name and scope instead. Also handle the mixed user/internal type case explicitly. Add tests for: userland first-class callables, cloned internal closures, and cross-type (user vs internal) closure rejection. * php_reflection: Simplify the Closure::__invoke() check --------- Co-authored-by: Tim Düsterhus <tim@bastelstu.be>
1 parent 7e16d4e commit 4561e92

File tree

2 files changed

+79
-4
lines changed

2 files changed

+79
-4
lines changed

ext/reflection/php_reflection.c

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3441,14 +3441,27 @@ static void reflection_method_invoke(INTERNAL_FUNCTION_PARAMETERS, int variadic)
34413441
}
34423442

34433443
/* For Closure::__invoke(), closures from different source locations have
3444-
* different signatures, so we must reject those. However, closures created
3445-
* from the same source (e.g. in a loop) share the same op_array and should
3446-
* be allowed. Compare the underlying function pointer via op_array. */
3444+
* different signatures, so we must reject those. */
34473445
if (obj_ce == zend_ce_closure && !Z_ISUNDEF(intern->obj)
34483446
&& Z_OBJ_P(object) != Z_OBJ(intern->obj)) {
34493447
const zend_function *orig_func = zend_get_closure_method_def(Z_OBJ(intern->obj));
34503448
const zend_function *given_func = zend_get_closure_method_def(Z_OBJ_P(object));
3451-
if (orig_func->op_array.opcodes != given_func->op_array.opcodes) {
3449+
3450+
bool same_closure = false;
3451+
/* Check if they are either both fake closures or they both are not. */
3452+
if ((orig_func->common.fn_flags & ZEND_ACC_FAKE_CLOSURE) == (given_func->common.fn_flags & ZEND_ACC_FAKE_CLOSURE)) {
3453+
if (orig_func->common.fn_flags & ZEND_ACC_FAKE_CLOSURE) {
3454+
/* For fake closures, scope and name must match. */
3455+
same_closure = orig_func->common.scope == given_func->common.scope
3456+
&& orig_func->common.function_name == given_func->common.function_name;
3457+
} else {
3458+
/* Otherwise the opcode structure must be identical. */
3459+
ZEND_ASSERT(orig_func->type == ZEND_USER_FUNCTION);
3460+
same_closure = orig_func->op_array.opcodes == given_func->op_array.opcodes;
3461+
}
3462+
}
3463+
3464+
if (!same_closure) {
34523465
if (!variadic) {
34533466
efree(params);
34543467
}

ext/reflection/tests/gh21362.phpt

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,60 @@ $m2 = new ReflectionMethod($closures[0], '__invoke');
4343
foreach ($closures as $closure) {
4444
var_dump($m2->invoke($closure));
4545
}
46+
47+
// First-class callable of a userland function
48+
function my_func($x) { return "my_func: $x"; }
49+
function other_func($x) { return "other_func: $x"; }
50+
51+
$mf = my_func(...);
52+
$mf2 = my_func(...);
53+
$of = other_func(...);
54+
55+
$m3 = new ReflectionMethod($mf, '__invoke');
56+
var_dump($m3->invoke($mf, 'test'));
57+
var_dump($m3->invoke($mf2, 'test'));
58+
59+
try {
60+
$m3->invoke($of, 'test');
61+
echo "No exception thrown\n";
62+
} catch (ReflectionException $e) {
63+
echo $e->getMessage() . "\n";
64+
}
65+
66+
// Internal closures (first-class callable syntax) should also be validated
67+
$vd = var_dump(...);
68+
$pr = print_r(...);
69+
70+
$m4 = new ReflectionMethod($vd, '__invoke');
71+
$m4->invoke($vd, 'internal closure OK');
72+
73+
// Cloned internal closure is a different object but same function - should work
74+
$vd2 = clone $vd;
75+
$m4->invoke($vd2, 'cloned internal closure OK');
76+
77+
// Different internal closure should throw
78+
try {
79+
$m4->invoke($pr, 'should not print');
80+
echo "No exception thrown\n";
81+
} catch (ReflectionException $e) {
82+
echo $e->getMessage() . "\n";
83+
}
84+
85+
// Cross-type: userland Closure to internal Closure's invoke should throw
86+
try {
87+
$m4->invoke($c1, 'should not print');
88+
echo "No exception thrown\n";
89+
} catch (ReflectionException $e) {
90+
echo $e->getMessage() . "\n";
91+
}
92+
93+
// Cross-type: internal Closure to userland Closure's invoke should throw
94+
try {
95+
$m->invoke($vd, 'should not print');
96+
echo "No exception thrown\n";
97+
} catch (ReflectionException $e) {
98+
echo $e->getMessage() . "\n";
99+
}
46100
?>
47101
--EXPECT--
48102
c1: foo=FOO, bar=BAR
@@ -51,3 +105,11 @@ Given Closure is not the same as the reflected Closure
51105
int(0)
52106
int(1)
53107
int(2)
108+
string(13) "my_func: test"
109+
string(13) "my_func: test"
110+
Given Closure is not the same as the reflected Closure
111+
string(19) "internal closure OK"
112+
string(26) "cloned internal closure OK"
113+
Given Closure is not the same as the reflected Closure
114+
Given Closure is not the same as the reflected Closure
115+
Given Closure is not the same as the reflected Closure

0 commit comments

Comments
 (0)