Skip to content

Commit

Permalink
Fix GH-11406: segfault with unpacking and magic method closure
Browse files Browse the repository at this point in the history
The magic method trampoline closure may be variadic. However, the
arg_info for the variadic argument was not set, resulting in a crash
both in reflection and in the VM.

Fix it by creating an arg_info containing a single element in case of
the variadic case. The variadic argument is the last one (and in this
case only one) in the arg_info array.

We make sure the argument info is equivalent to the argument info of
`$closure` of the following code snippet:
```
function foo(...$arguments) {}
$closure = foo(...);
```

Closes GH-11417.
  • Loading branch information
nielsdos committed Jun 13, 2023
1 parent 18f2f0a commit 5c78980
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 0 deletions.
2 changes: 2 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ PHP NEWS
- Core:
. Fix GH-11388 (Allow "final" modifier when importing a method from a trait).
(nielsdos)
. Fixed bug GH-11406 (segfault with unpacking and magic method closure).
(nielsdos)

- DOM:
. Fix #79700 (wrong use of libxml oldNs leads to performance problem).
Expand Down
35 changes: 35 additions & 0 deletions Zend/tests/trampoline_closure_named_arguments.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,32 @@ class Test {

$test = new Test;

$array = ["unpacked"];

echo "-- Non-static cases --\n";
$test->test(1, 2, a: 123);
$test->test(...)(1, 2);
$test->test(...)(1, 2, a: 123, b: $test);
$test->test(...)(a: 123, b: $test);
$test->test(...)();
$test->test(...)(...$array);

echo "-- Static cases --\n";
Test::testStatic(1, 2, a: 123);
Test::testStatic(...)(1, 2);
Test::testStatic(...)(1, 2, a: 123, b: $test);
Test::testStatic(...)(a: 123, b: $test);
Test::testStatic(...)();
Test::testStatic(...)(...$array);

echo "-- Reflection tests --\n";
$reflectionFunction = new ReflectionFunction(Test::fail(...));
var_dump($reflectionFunction->getParameters());
$argument = $reflectionFunction->getParameters()[0];
var_dump($argument->isVariadic());
$type = $argument->getType();
var_dump($type);
var_dump($type->getName());

?>
--EXPECT--
Expand Down Expand Up @@ -70,6 +83,11 @@ array(2) {
string(4) "test"
array(0) {
}
string(4) "test"
array(1) {
[0]=>
string(8) "unpacked"
}
-- Static cases --
string(10) "testStatic"
array(3) {
Expand Down Expand Up @@ -110,3 +128,20 @@ array(2) {
string(10) "testStatic"
array(0) {
}
string(10) "testStatic"
array(1) {
[0]=>
string(8) "unpacked"
}
-- Reflection tests --
array(1) {
[0]=>
object(ReflectionParameter)#4 (1) {
["name"]=>
string(9) "arguments"
}
}
bool(true)
object(ReflectionNamedType)#5 (0) {
}
string(5) "mixed"
6 changes: 6 additions & 0 deletions Zend/zend_closures.c
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,9 @@ ZEND_API void zend_create_fake_closure(zval *res, zend_function *func, zend_clas
}
/* }}} */

/* __call and __callStatic name the arguments "$arguments" in the docs. */
static zend_internal_arg_info trampoline_arg_info[] = {ZEND_ARG_VARIADIC_TYPE_INFO(false, arguments, IS_MIXED, false)};

void zend_closure_from_frame(zval *return_value, zend_execute_data *call) { /* {{{ */
zval instance;
zend_internal_function trampoline;
Expand All @@ -856,6 +859,9 @@ void zend_closure_from_frame(zval *return_value, zend_execute_data *call) { /* {
trampoline.handler = zend_closure_call_magic;
trampoline.function_name = mptr->common.function_name;
trampoline.scope = mptr->common.scope;
if (trampoline.fn_flags & ZEND_ACC_VARIADIC) {
trampoline.arg_info = trampoline_arg_info;
}

zend_free_trampoline(mptr);
mptr = (zend_function *) &trampoline;
Expand Down

0 comments on commit 5c78980

Please sign in to comment.