Possible regression with spreading nested arrays into a function call and references

[iTob191]

Description

I've noticed the following possible regression:

$args = [1];
$ref = [&$args];
function test(&$v) {
    $v = 7;
};
test(...$ref[0]);
var_dump($args[0]);

Starting with PHP 7.3, the output of the above code has changed:

$ docker run --rm php:7.2.34 -r "$args = [1]; $ref = [&$args]; function test(&$v) { $v = 7; }; test(...$ref[0]); var_dump($args[0]);"
int(7)

$ docker run --rm php:7.3    -r "$args = [1]; $ref = [&$args]; function test(&$v) { $v = 7; }; test(...$ref[0]); var_dump($args[0]);"
int(1)

$ docker run --rm php:8.3.7  -r "$args = [1]; $ref = [&$args]; function test(&$v) { $v = 7; }; test(...$ref[0]); var_dump($args[0]);"
int(1)

The only entry I've found within PHP 7.3's release notes that might be related is References returned by Array and Property Accesses are immediately unwrapped. But I'm not sure whether this does actually describe the behavior change in the code snippet above.

I'm also not really sure whether the code is valid or not. In case it is not, it might be helpful to display a message.

There exists a workaround to get the old result by introducing another variable:

$temp = &$ref[0];
test(...$temp);

PHP Version

PHP 7.3.0 - PHP 8.3.7

Operating System

No response

[iluuu1994]

It does seem like this should work. I think the issue is that ...$ref[0] will cause a FETCH_DIM_R, which will deref the value before storing it in the result. SEND_UNPACK can handle references, but it doesn't receive the reference here. We probably want a FETCH_DIM_W (or FETCH_DIM_FUNC_ARG) instead.

Edit: Actually, SEND_UNPACK probably needs to handle IS_INDIRECT, so that it can separate the inner array properly.

[iluuu1994]

See https://github.com/php/php-src/pull/14205. This turns out not to be an easy fix. I believe the commit that broke this is https://github.com/php/php-src/commit/d1a38743a57a7dcbeb3dbddb17e56817addcbb33, although I couldn't verify because the code no longer compiles for me.

Going into a bit more detail, your issue can be reduced to the following:

https://3v4l.org/LQaoS

function test(&$v) {
    $v = 7;
}

$args = [[1]];
test(...$args[0]);
var_dump($args[0]);

This dumps [1] for everything >= 7.0.7. However, everything <7.0.7 contained a different, probably worse bug:

https://3v4l.org/dvCtf

function test(&$v) {
    $v = 7;
}

$args = [[1]];
$argsCopy = $args;
test(...$args[0]);
var_dump($argsCopy[0]);

Here, $argsCopy is modified, even though it is unrelated to $args, which is the variable passed to test.

The issue is complex to explain, let me do it anyway for the sake of documentation. It basically boils down to how $args[0] is executed. Array offsets can either be:

• fetched "for read", which looks up the element, returns a copy of the element if it exists, or warns and returns null
• fetched "for write", which creates an offset and initializes it with null if it doesn't already exist, and returns a pointer to it
• fetched "for read+write", which works the same as "for write", but also warns when an offset doesn't exist
• There's also fetch for isset and unset, but they are not particularly relevant

None of those are really what we need. In this particular case, we would need a fetch mode that works the same as read+write, but that doesn't emit any indirect modification or related warnings (see Zend/tests/bug78356.php with this patch). Or better yet, warns only if any of the params are by-ref. Furthermore, always fetching by r+w has the downside of always separating arrays, even if unnecessary when all parameters are by-value.

Anyway, all of this to say, I don't think fixing this is worthwhile.

/cc @bwoebi

[iTob191]

Thanks for the thorough investigation 👍

Going into a bit more detail, your issue can be reduced to the following: https://3v4l.org/LQaoS [...] This dumps [1] for everything >= 7.0.7. However, everything <7.0.7 contained a different, probably worse bug:

That's surprising given that my issue (https://3v4l.org/erN2Q) only occurred >= 7.3.

[iluuu1994]

@iTob191 Interesting. Unfortunately, 7.2/7.3 also doesn't build for me, and 7.3 changed too many things to look through the commits. There's likely some other factor regarding references for your specific case, but the root issue remains.