php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #72598 Reference is lost after array_slice()
Submitted: 2016-07-14 15:45 UTC Modified: 2016-08-16 13:37 UTC
From: dominikschilling+php at gmail dot com Assigned:
Status: Closed Package: Scripting Engine problem
PHP Version: 7.1Git-2016-07-14 (Git) OS:
Private report: No CVE-ID:
 [2016-07-14 15:45 UTC] dominikschilling+php at gmail dot com
Description:
------------
I noticed a weird issue when running the unit tests of WordPress with PHP 7.1.0.
Our ticket: https://core.trac.wordpress.org/ticket/37295
The failing test: https://travis-ci.org/aaronjorbin/develop.wordpress/jobs/142322444#L387-L396

It looks like array_slice() is removing the reference of a variable under certain conditions. The test script is a short form of what WordPress is doing here.




Test script:
---------------
<?php
global $wp_filter;
$wp_filter[ 'pre_get_posts' ][ 9 ][ 1 ] = [ 'function' => 'function_which_expects_a_reference', 'accepted_args' => 1 ];
$wp_filter[ 'pre_get_posts' ][ 9 ][ 2 ] = [ 'function' => 'function_which_expects_a_reference', 'accepted_args' => 1 ];

class FooBar {
	function __construct() {
		do_action_ref_array( 'pre_get_posts', [ &$this ] );
	}
}

function do_action_ref_array( $tag, $args ) {
	global $wp_filter;
	reset( $wp_filter[ $tag ] );
	do {
		foreach ( current( $wp_filter[ $tag ] ) as $the_ ) {
			$a = array_slice( $args, 0, (int) $the_['accepted_args'] );
			xdebug_debug_zval( 'a' );
			call_user_func_array( $the_['function'], $a );
		}
	} while ( next( $wp_filter[ $tag ] ) !== false );
}

function function_which_expects_a_reference( &$variable ) {}
new FooBar();

Expected result:
----------------
a: (refcount=1, is_ref=0)=array (0 => (refcount=3, is_ref=1)=class FooBar {  })
a: (refcount=1, is_ref=0)=array (0 => (refcount=3, is_ref=1)=class FooBar {  })

Actual result:
--------------
a: (refcount=1, is_ref=0)=array (0 => (refcount=1, is_ref=1)=class FooBar {  })
a: (refcount=1, is_ref=0)=array (0 => (refcount=4, is_ref=0)=class FooBar {  })
PHP Warning:  Parameter 1 to function_which_expects_a_reference() expected to be a reference, value given in /php71.php on line 21
PHP Stack trace:
PHP   1. {main}() /php71.php:0
PHP   2. FooBar->__construct() /php71.php:28
PHP   3. do_action_ref_array() /php71.php:8

Warning: Parameter 1 to function_which_expects_a_reference() expected to be a reference, value given in /php71.php on line 21

Call Stack:
    0.0006     361384   1. {main}() /php71.php:0
    0.0030     362576   2. FooBar->__construct() /php71.php:28
    0.0030     362976   3. do_action_ref_array() /php71.php:8

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-07-14 17:27 UTC] bwoebi@php.net
-Status: Open +Status: Analyzed
 [2016-07-14 17:27 UTC] bwoebi@php.net
(That it needs two iterations is another bug in array_slice).

The general issue here is the refcount of the reference being 1, as &$this doesn't make $this a reference; it merely fetches the $this value (due to some optimization) and references that value, resulting in a reference of refcount 1...

I have no idea how to fix that without leaking the reference or storing it on the object and checking for it in each dtor...

Simpler repro:

function ref(&$ref) {}

new class {
        function __construct() {
                $args = [&$this];
                for ($i = 0; $i < 2; $i++) {
                        $a = array_slice($args, 0, 1);
                        call_user_func_array('ref', $a);
                }
        }
};
 [2016-07-14 19:57 UTC] dmitry@php.net
PHP-7.1 doesn't allow modification of $this through references on purpose.
See https://wiki.php.net/rfc/this_var#disable_ability_to_re-assign_this_indirectly_through_reference

The simplest workaround is replacing $this with another variable.

- do_action_ref_array( 'pre_get_posts', [ &$this ] );
+ $obj = $this;
+ do_action_ref_array( 'pre_get_posts', [ &$obj ] );
 [2016-07-14 20:45 UTC] dominikschilling+php at gmail dot com
Thanks for link to the RFC. I think I'm missing the point why "re-assign $this" == "modification of $this" which is basically "$this = 'something'" vs. "$this->something = 'something'".

For example the following script is still working and also a modification of $this, although it's another context:

class C {
  public $b;
  function foo(){
    $this->b = 'a';
  }
}
$x = new C;
$x->foo();
var_dump($x->b); // Prints a


> That it needs two iterations is another bug in array_slice

Does that now mean that it should already fail on the first iteration?
 [2016-07-14 21:33 UTC] bwoebi@php.net
Yes, it should already fail at the first iteration I think.

Modification of $this means the content of the $this variable itself, not the object.
I.e. $this must never become another object or scalar.

@dmitry yeah, that's a workaround, but it's only a workaround for the bug… the bug needs to be fixed somehow though before 7.1.0. I don't find this broken behavior acceptable to be shipped in the release.
 [2016-07-15 08:47 UTC] dmitry@php.net
The problem is not related to $this handling at all.

Test script:
---------------
<?php
function ref(&$a) {
	var_dump($a);
}
$b = 1;
$args = [&$b];
unset($b);
for ($i = 0; $i < 2; $i++) {
	$a = array_slice($args, 0, 1);
	call_user_func_array('ref', $a);
}
?>

Expected result:
----------------
int(1)
int(1)

Actual result:
--------------
int(1)
PHP Warning:  Parameter 1 to ref() expected to be a reference, value given in %s on line %d

I think this warning doesn't make a lot of sense at all, or at least it shouldn't prevent execution of the called function.

Fixing this is a new minor BC break.
 [2016-07-15 20:06 UTC] nikic@php.net
@dmitry: This warning/behavior has been around since forever, I don't understand why we should change it now. call_user_func_array() can't create a reference into the array, so we can only do a by-reference pass if the element is already a reference. If there's no existing reference, the call will not work correctly. (If the by-reference passing does not actually matter, then one should not use by-reference passing...)
 [2016-08-09 12:20 UTC] dominikschilling+php at gmail dot com
I'm curious what the status of this bug is.

I've added the scripts to 3v4l.org to compare the output of different PHP versions:

Script from comment 1: https://3v4l.org/MCmnj, warning exists in 7.1.0alpha2 - 7.1.0beta1
Script from comment 2: https://3v4l.org/WECs1, 7.0.0 - 7.1.0beta1 produces only one warning, 5.3.0 - 5.6.24 has two

Another (unrelated?) script: https://3v4l.org/fESuN#v530, I noticed this because I got the (valid) warning after upgrading to 7.0.9.
 [2016-08-23 09:18 UTC] dmitry@php.net
Automatic comment on behalf of dmitry@zend.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=62ab40bc9642798a9af0435ef3284a4f5d9bf6d4
Log: Added tests and NEWS entry Fixed bug #72598 (Reference is lost after array_slice())
 [2016-08-23 09:18 UTC] dmitry@php.net
-Status: Analyzed +Status: Closed
 [2016-10-17 10:09 UTC] bwoebi@php.net
Automatic comment on behalf of dmitry@zend.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=62ab40bc9642798a9af0435ef3284a4f5d9bf6d4
Log: Added tests and NEWS entry Fixed bug #72598 (Reference is lost after array_slice())
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Aug 29 15:01:52 2017 UTC