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: None
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: dominikschilling+php at gmail dot com
New email:
PHP Version: OS:

 

 [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

Pull Requests

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-2024 The PHP Group
All rights reserved.
Last updated: Thu Nov 21 11:01:29 2024 UTC