php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #81027 WeakReference will cause memory leaks
Submitted: 2021-05-10 14:04 UTC Modified: 2021-08-12 10:52 UTC
Votes:2
Avg. Score:4.0 ± 1.0
Reproduced:1 of 1 (100.0%)
Same Version:1 (100.0%)
Same OS:1 (100.0%)
From: gy dot dlcs at gmail dot com Assigned:
Status: Analyzed Package: Unknown/Other Function
PHP Version: 8.0.6 OS: Linux
Private report: No CVE-ID: None
 [2021-05-10 14:04 UTC] gy dot dlcs at gmail dot com
Description:
------------
Just like the test script, When you Uncomment the `!$wr->get();` line, You will see the memory leaks in a few seconds.

Test script:
---------------
<?php
ini_set('memory_limit', '16M');

while (true) {
    $a = new stdClass();
    $a->a = $a;

    $wr = WeakReference::create($a);
    unset($a);

    // Uncomment next line, memory will not leak
    // $wr->get();

    // Uncomment next line, memory will leak
    // !$wr->get();
}


Expected result:
----------------
PHP Fatal error:  Allowed memory size of 16777216 bytes exhausted (tried to allocate 20480 bytes) in test.php on line 9



Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2021-05-10 14:09 UTC] gy dot dlcs at gmail dot com
php 7.4 and 8.0 will be affected
 [2021-05-10 14:58 UTC] cmb@php.net
-Status: Open +Status: Not a bug -Assigned To: +Assigned To: cmb
 [2021-05-10 14:58 UTC] cmb@php.net
Sorry, but your problem does not imply a bug in PHP itself.  For a
list of more appropriate places to ask for help using PHP, please
visit http://www.php.net/support.php as this bug system is not the
appropriate forum for asking support questions.  Due to the volume
of reports we can not explain in detail here why your report is not
a bug.  The support channels will be able to provide an explanation
for you.

Thank you for your interest in PHP.
 [2021-05-10 15:15 UTC] nikic@php.net
I can't reproduce this.
 [2021-05-10 15:31 UTC] gy dot dlcs at gmail dot com
emmm, I reproduced it with a clean container, this is the video:

https://user-images.githubusercontent.com/14296262/117684531-ab093080-b1e7-11eb-8db8-f9dc4405bbf4.mp4
 [2021-05-10 15:43 UTC] cmb@php.net
-Status: Not a bug +Status: Feedback
 [2021-05-10 15:43 UTC] cmb@php.net
For me there is no difference whether ther is `$wr->get()`,
`!$wr->get()`, or neither; memory consumption increases until GC
kicks in.

Are there any leaks reported when you're running under valgrind
(set USE_ZEND_ALLOC=0)?
 [2021-05-10 16:56 UTC] gy dot dlcs at gmail dot com
I create a clean VM on Digitalocean, and reproduced it, this is the video and valgrind test result:

https://github.com/LuttyYang/php-bug-81027
 [2021-05-11 06:28 UTC] krakjoe@php.net
I can't reproduce this either.

In addition, I see no possible path to a leak.
 [2021-05-11 09:26 UTC] cmb@php.net
-Assigned To: cmb +Assigned To:
 [2021-05-11 09:26 UTC] cmb@php.net
Thanks for the valgrind report, but unfortunately this is useless.
The warnings at the top are bogus, and everything after the SIGINT
is irrelevant.  Sorry for requesting it!

I wouldn't know how to proceed with this issue.
 [2021-05-13 11:11 UTC] gy dot dlcs at gmail dot com
The above test script takes about 1 minute to reproduce the problem, Have you waited long enough?
 [2021-05-14 12:08 UTC] dharman@php.net
I can't reproduce it with 8.0.3 but I can reproduce it with the current snapshot from GIT
 [2021-08-12 10:14 UTC] nikic@php.net
Okay, I clearly didn't wait long enough last time. There is indeed a leak here, but a very slow one.

I used this modified script:

<?php
ini_set('memory_limit', '8M');

$m = memory_get_usage();
for ($i = 0; ; $i++) {
    $m2 = memory_get_usage();
    if ($m2 > $m) {
        $m = $m2;
        echo "$i: ", $m, "\n";
    }

    $a = new stdClass();
    $a->a = $a;

    $wr = WeakReference::create($a);
    unset($a);

    // Uncomment next line, memory will not leak
    //$wr->get();

    // Uncomment next line, memory will leak
    !$wr->get();
}


This shows that on every GC cycle (every 10000 iterations) memory usage increases by 560 bytes:

...
5449999: 6453552
5459999: 6454112
5469999: 6454672
5479999: 6455232
...
 [2021-08-12 10:30 UTC] nikic@php.net
-Status: Open +Status: Analyzed
 [2021-08-12 10:30 UTC] nikic@php.net
Simpler reproducer:

// We should have enough iterations to trigger a GC run. 
for ($i = 0; $i < 10000; $i++) {
    $a = new stdClass();
    $a->a = $a;

    $wr = WeakReference::create($a);
    unset($a);

    // The WeakReference::get() result should be used, but immediately destroyed. 
    !$wr->get();
}

This will detect the leak:

/home/nikic/php/php-src/Zend/zend_objects.c(186) :  Freeing 0x00007ffa5bc09c30 (40 bytes), script=/home/nikic/php/php-src/t415.php

The problem is as follows: Cycle GC is triggered when destroying the $this of the WeakReference::get() call. At that point the object is stored inside the call return value, so it cannot be GCed. Then the return value gets destroyed, but as it is TMPVAL, the assumption is that it does not need to be rooted. As such, the cycle leaks.

We should be able to address this in the same way as the unserialize() return value, which can also return a dead cycle: We need to explicitly root the object in WeakReference::get().
 [2021-08-12 10:52 UTC] nikic@php.net
> We should be able to address this in the same way as the unserialize() return value, which can also return a dead cycle: We need to explicitly root the object in WeakReference::get().

While that might address another case, it doesn't help here: We perform GC when releasing $this, which will drop the root anyway. We'd have to re-add the root after GC like we do for live TMPVARs, but I don't think we can easily do this for  the result variable of the active opcode, because we do not know whether or not it has been initialized yet.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Wed Oct 09 07:01:28 2024 UTC