php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #73960 Leak with instance method calling static method with referenced return
Submitted: 2017-01-19 18:05 UTC Modified: 2017-03-20 17:19 UTC
From: highmind63 at gmail dot com Assigned: nikic
Status: Closed Package: Class/Object related
PHP Version: 7.0.15 OS: Windows 10 and Ubuntu 14.04
Private report: No CVE-ID:
Password:
Status:
Package:
Bug Type:
Summary:
From: highmind63 at gmail dot com
New email:
PHP Version: OS:

 

 [2017-01-19 18:05 UTC] highmind63 at gmail dot com
Description:
------------
Memory leaks with referenced variable return in static method called by instance method.

Test script:
---------------
<pre>
<?php
class Leak {
  static protected function &leaked(array &$array = null) {
    $array = array('one');
    return $array;
  }

  public function toLeaked($array = null) {
    $array = static::leaked($array);
    return $array;
  }

  public function testLeak() {
    echo "memory before loop: " . memory_get_usage(true) . PHP_EOL;
    for ($i = 0; $i < 5000; $i++) {
      $leak = $this->toLeaked();
    }
    echo "memory after loop: " . memory_get_usage(true) . PHP_EOL;
  }
}

$leak = new Leak();
$leak->testLeak();
?>
</pre>

Expected result:
----------------
No Leak

Actual result:
--------------
Leaks Memory:

memory before loop: 2097152
memory after loop: 4194304

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-01-19 21:38 UTC] danack@php.net
This apparently affects 7.0.x but not 7.1.x

https://3v4l.org/XBPgY
 [2017-01-21 12:50 UTC] cmb@php.net
-Status: Open +Status: Verified
 [2017-01-21 12:50 UTC] cmb@php.net
> This apparently affects 7.0.x but not 7.1.x

Running the supplied test script on `--enable-debug` builds
shows memory leaks for PHP-7.1 and master as well.
 [2017-01-21 13:03 UTC] nikic@php.net
Reduced testcase:

function &leaked(array &$array = null) {
    $array = array('one');
    return $array;
}
$array = leaked($array);
 [2017-01-21 14:18 UTC] nikic@php.net
Even simpler repro:

$array = array('one');
$array = $ref =& $array;

The problem is that in zend_assign_to_variable() in the case where LHS and RHS point to the same value we currently do not destroy the RHS if it is a VAR. Proposed patch:

diff --git a/Zend/zend_execute.h b/Zend/zend_execute.h
index 554ad28..5f0caf6 100644
--- a/Zend/zend_execute.h
+++ b/Zend/zend_execute.h
@@ -81,6 +81,10 @@ static zend_always_inline zval* zend_assign_to_variable(zval *variable_ptr, zval
                                return variable_ptr;
                        }
                        if (ZEND_CONST_COND(value_type & (IS_VAR|IS_CV), 1) && variable_ptr == value) {
+                               if (value_type == IS_VAR && ref) {
+                                       ZEND_ASSERT(GC_REFCOUNT(ref) > 1);
+                                       --GC_REFCOUNT(ref);
+                               }
                                return variable_ptr;
                        }
                        garbage = Z_COUNTED_P(variable_ptr);
 [2017-01-22 11:07 UTC] krakjoe@php.net
Nikita can you turn that into a PR with test case please, so we can get CI and some attention.
 [2017-03-10 17:23 UTC] nikic@php.net
Automatic comment on behalf of nikita.ppv@gmail.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=29ee3e3c49bd3b32219f45ea4d4f1263c3021150
Log: Fixed bug #73960
 [2017-03-10 17:23 UTC] nikic@php.net
-Status: Verified +Status: Closed
 [2017-03-20 17:09 UTC] highmind63 at gmail dot com
Is this going to be backported to 7.0.x?
 [2017-03-20 17:19 UTC] nikic@php.net
-Assigned To: +Assigned To: nikic
 [2017-03-20 17:19 UTC] nikic@php.net
The change is already in 7.0+, it will be released as part of PHP 7.0.18, see https://github.com/php/php-src/blob/PHP-7.0/NEWS.
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Aug 29 15:01:52 2017 UTC