call_user_func(_array): Don't abort on reference warning

[nikic]

This commit implements two changes:
a) zend_call_function() will treat reference zvals with refcount=1
as if they were non-reference zvals.
b) zend_call_function() will allow performing calls where a
reference argument is passed by-value. The usual warning is
still emitted, but the call is no longer aborted.

The motivation for these changes is that currently the interaction
of references and call_user_func_array() is very fragile and
harmless internal changes can break calls as a side-effect. The
reason is that due to internal optimizations (e.g. the recent
array_slice() change that breaks WordPress) reference zvals with
rc=1 may not be retained in an array (which is by itself perfectly
fine, because rc=1 references are not references). Currently this
will cause call_user_func_array() to fail.

This change tries to counteract this by a) making sure we don't
treat rc=1 zvals as references and thus make the behavior
independent from internal details and b) making sure we don't
introduce backwards-compatibility issues by no longer aborting
the function call on the warning.

[nikic]

Motivation: Fix fallout from e730c8f. Earlier issue (also relating to array_slice, but due to a different change) reported in https://bugs.php.net/bug.php?id=72598.

/cc @dstogov @laruence @bwoebi

[bwoebi]

In general I like the idea of making it consistent, it may though introduce some subtle BC breaks.
I'm not sure whether we shall already introduce it for 7.1.

Though, can't we just promote non-refs to refs inside the array instead?
I feel like this would also in general the least breaking option and most helpful one (e.g. mysqli_stmt_bind_params with cufa() may break with this change and promoting to refs will also remove the need for hacky hacks like foreach ($vals as &$val); [empty by-ref loop to create rc=1 refs] ... )

[nikic]

@bwoebi The problem with creating refs is that this would require accepting the array by-reference (which we don't).

I think that we need to do something for 7.1 to avoid currently existing breakage, i.e. should at least do the change to only throw a warning but not abort the call. I rolled the rc=1 change into this PR to make sure the warning is shown consistently, not depending on internal details.

[bwoebi]

We could use prefer_ref instead.

Yes, it's also a small change, but probably the most useful one.

As said, I fully agree it should be fixed, I just don't think your PR is the optimal solution.

Bob Weinand (iPhone)

Am 06.08.2016 um 16:24 schrieb Nikita Popov notifications@github.com:

@bwoebi The problem with creating refs is that this would require accepting the array by-reference (which we don't).

I think that we need to do something for 7.1 to avoid currently existing breakage, i.e. should at least do the change to only throw a warning but not abort the call. I rolled the rc=1 change into this PR to make sure the warning is shown consistently, not depending on internal details.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[dstogov]

@bwoebi, even if we use prefer_ref, call_user_func_array() might be called with array as value (or even immutable array), and in this case, we can't promote nested values to references.

So we may keep the original behavior for the first two cases and try to promote values to references for the third case, but this doesn't add consistency.

For now, I think this PR is a better compromise.

[bwoebi]

@dstogov what exactly is the issue? The function then gets called, with the parameter promoted to a reference in the copy of the array.

We do not get the result - but that's the responsibility of the caller. Not every by-ref parameter is meaningful to the caller.

E.g.

call_user_func_array("array_pop", [[1, 2, 3]]); // returns 3, doesn't modify the array
$a = [1, 2, 3];
call_user_func_array("array_pop", [$a]); // returns 3, doesn't modify $a
$a = [1, 2, 3];
call_user_func_array("array_pop", [&$a]); // returns 3 and $a is modified
$args = [[1,2,3]];
call_user_func_array("array_pop", $args); // returns 3 and $args[0] is modified

I.e. cufa() calls MAKE_REF in any case on the passed array (which will be a copy if a value has been passed), and pass RC=2 references to the function [if not yet ref]. If the value gets modified, the modified value will just be thrown away after the call.

[dstogov]

I tried to implement @bwoebi idea.
Please review #2085

[dstogov]

@nikic this PR breaks few PDO and PHAR tests:

via [ext/pdo_mysql/tests/common.phpt]
MySQL PDO Common: PDO::FETCH_CLASS [ext/pdo_mysql/tests/pdo_005.phpt]
via [ext/pdo_pgsql/tests/common.phpt]
Postgres PDO Common: PDO::FETCH_CLASS [ext/pdo_pgsql/tests/pdo_005.phpt]
via [ext/pdo_sqlite/tests/common.phpt]
SQLite PDO Common: PDO::FETCH_CLASS [ext/pdo_sqlite/tests/pdo_005.phpt]
Phar front controller with valid callback that is not good [cache_list] [ext/phar/tests/cache_list/frontcontroller32.phpt]
Phar front controller with valid callback that is not good [ext/phar/tests/frontcontroller32.phpt]
Phar::setSupportedSignatures() with hash [ext/phar/tests/phar_setsignaturealgo2.phpt]
Phar::setSupportedSignatures() with hash, tar-based [ext/phar/tests/tar/phar_setsignaturealgo2.phpt]
Phar::setSupportedSignatures() with hash, zip-based [ext/phar/tests/zip/phar_setsignaturealgo2.phpt]

[nikic]

@dstogov I've created a new PR at #2088. This one only allows the call to proceed (with warning), but does not change any behavior with regard to rc=1 references. The reason is that I was not sure what to do with the PDO failure on the FETCH_CLASS test -- it does some very odd things with references, but as there's a test for it it seems better to leave it alone.

[nikic]

Superseded by #2088.