Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Combination of phpdbg and Generator method causes memory leak #12837

Open
ngyuki opened this issue Nov 30, 2023 · 8 comments · May be fixed by #17303
Open

Combination of phpdbg and Generator method causes memory leak #12837

ngyuki opened this issue Nov 30, 2023 · 8 comments · May be fixed by #17303
Assignees
@nielsdos
Labels
Bug Category: Generators SAPI: phpdbg Status: Verified

Comments

@ngyuki
Copy link

ngyuki commented Nov 30, 2023

Description

Run phpdbg on object has generator method causes memory leaks.
It does not occur when run with php command.

The following code:

<?php
class A
{
    private $s;

    public function __construct()
    {
        $this->s = str_repeat('a', 1000000);
    }

    public function iterate()
    {
        yield 1;
    }
}

for ($i=0; $i<1000000; $i++) {
    $arr = iterator_to_array((new A())->iterate());
}

var_dump('ok', memory_get_peak_usage(true));

Resulted in this output:

$ phpdbg -qrr zz.php
[PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 1003520 bytes) in /w/zz.php on line 8]

But I expected this output instead:

$ php zz.php
string(2) "ok"
int(2097152)

PHP Version

PHP 8.3.0

Operating System

Debian 12
@iluuu1994
Copy link
Member

/cc @bwoebi

@SakiTakamachi
Copy link
Member

This is not a memory leak, an error that used up all of the memory available to php. Most likely, the original code was designed to use relatively memory, and phpdbg used more memory, causing the memory limit to be exceeded.

Try adding this at the beginning of your php code. I think the error will probably no longer occur.

<?php

ini_set('memory_limit', '512M');

@SakiTakamachi
Copy link
Member

SakiTakamachi commented Nov 30, 2023 •
edited
Loading

Oops, the case of using phpdbg, you may should set the memory limit on command args.

phpdbg -qrr -d memory_limit=512M zz.php

@ngyuki
Copy link
Author

ngyuki commented Nov 30, 2023

No, it may be that for some reason object is not freed by garbage collector.

<?php
class A
{
    public function __construct()
    {
        var_dump('__construct');
    }

    public function __destruct()
    {
        var_dump('__destruct');
    }

    public function iterate()
    {
        yield 1;
    }
}

$arr = iterator_to_array((new A())->iterate()); gc_collect_cycles();
$arr = iterator_to_array((new A())->iterate()); gc_collect_cycles();
$arr = iterator_to_array((new A())->iterate()); gc_collect_cycles();

var_dump('exit');
$ php zz.php
string(11) "__construct"
string(10) "__destruct"
string(11) "__construct"
string(10) "__destruct"
string(11) "__construct"
string(10) "__destruct"
string(4) "exit"

$ phpdbg -qrr zz.php
string(11) "__construct"
string(11) "__construct"
string(11) "__construct"
string(4) "exit"
string(10) "__destruct"
string(10) "__destruct"
string(10) "__destruct"

@iluuu1994
Copy link
Member

There's some special handling of generators in phpdbg. It's possible that it keeps some reference around, preventing them from being freed. That does seem like a bug to me, but maybe there's a reason why the generators are kept around until the end of the script in this case. I don't know, hopefully @bwoebi can answer.

@ngyuki
Copy link
Author

ngyuki commented Nov 30, 2023

Also, this problem does not occur if generator is function rather than object method.

<?php
class A
{
    public function __construct()
    {
        var_dump('__construct');
    }

    public function __destruct()
    {
        var_dump('__destruct');
    }

    public function iterate()
    {
        yield 1;
    }
}

function iterate()
{
    yield 1;
}

$arr = iterator_to_array(iterate(new A())); gc_collect_cycles();
$arr = iterator_to_array(iterate(new A())); gc_collect_cycles();
$arr = iterator_to_array(iterate(new A())); gc_collect_cycles();

var_dump('exit');
$ phpdbg -qrr zz.php
string(11) "__construct"
string(10) "__destruct"
string(11) "__construct"
string(10) "__destruct"
string(11) "__construct"
string(10) "__destruct"
string(4) "exit"

@ranvis
Copy link
Contributor

ranvis commented Dec 8, 2023

|| UNEXPECTED(zend_execute_ex != execute_ex) causing an extra addref to the class instance: d1dd474

@nielsdos
Copy link
Member

Nice find! I think the original fix is simply wrong and we should just avoid the double destruction by checking for ZEND_CALL_TOP. I'll make a PR soon.

@nielsdos nielsdos self-assigned this Dec 30, 2024
nielsdos added a commit to nielsdos/php-src that referenced this issue Dec 30, 2024
@nielsdos
Fix phpGH-12837: Combination of phpdbg and Generator method causes me…
296c1f3 
…mory leak

The fix for bug #72523 (d1dd474) added a check on `zend_execute_ex` to
add an extra refcount to `This`. This is necessary because the VM does a
re-entry here:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4294-L4299

and then cleans up `This` here:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4358-L4360

So if we don't add the refcount, we destroy `This` both in the VM and in
`zend_generator_close`.

The reason this causes a leak in phpdbg is because it changes the
`zend_execute_ex` temporarily back to the default here for `ZEND_DO_FCALL`:
https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/sapi/phpdbg/phpdbg_prompt.c#L1820-L1827
which means that we only execute `OBJ_RELEASE` on the `This` object once
in `zend_generator_close` instead of also doing it in the
`ZEND_DO_FCALL` handler.

To solve this, we make sure the check for the re-entrancy is
consistently done by checking the `ZEND_CALL_TOP` flag instead of
relying solely on `execute_ex` pointer:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4298
nielsdos added a commit to nielsdos/php-src that referenced this issue Dec 30, 2024
@nielsdos
Fix phpGH-12837: Combination of phpdbg and Generator method causes me…
1dd30cc 
…mory leak

The fix for bug #72523 (d1dd474) added a check on `zend_execute_ex` to
add an extra refcount to `This`. This is necessary because the VM does a
re-entry here:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4294-L4299

and then cleans up `This` here:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4358-L4360

So if we don't add the refcount, we destroy `This` both in the VM and in
`zend_generator_close`.

The reason this causes a leak in phpdbg is because it changes the
`zend_execute_ex` temporarily back to the default here for `ZEND_DO_FCALL`:
https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/sapi/phpdbg/phpdbg_prompt.c#L1820-L1827
which means that we only execute `OBJ_RELEASE` on the `This` object once
in `zend_generator_close` instead of also doing it in the
`ZEND_DO_FCALL` handler.

To solve this, we make sure the check for the re-entrancy is
consistently done by checking the `ZEND_CALL_TOP` flag instead of
relying solely on `execute_ex` pointer:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4298
@nielsdos nielsdos linked a pull request Dec 30, 2024 that will close this issue
Fix GH-12837: Combination of phpdbg and Generator method causes memory leak #17303
Open
nielsdos added a commit to nielsdos/php-src that referenced this issue Dec 30, 2024
@nielsdos
Fix phpGH-12837: Combination of phpdbg and Generator method causes me…
c9f7459 
…mory leak

The fix for bug #72523 (d1dd474) added a check on `zend_execute_ex` to
add an extra refcount to `This`. This is necessary because the VM does a
re-entry here:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4294-L4299

and then cleans up `This` here:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4358-L4360

So if we don't add the refcount, we destroy `This` both in the VM and in
`zend_generator_close`.

The reason this causes a leak in phpdbg is because it changes the
`zend_execute_ex` temporarily back to the default here for `ZEND_DO_FCALL`:
https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/sapi/phpdbg/phpdbg_prompt.c#L1820-L1827
which means that we only execute `OBJ_RELEASE` on the `This` object once
in `zend_generator_close` instead of also doing it in the
`ZEND_DO_FCALL` handler.

To solve this, we make sure the check for the re-entrancy is
consistently done by checking the `ZEND_CALL_TOP` flag instead of
relying solely on `execute_ex` pointer:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4298
nielsdos added a commit to nielsdos/php-src that referenced this issue Dec 30, 2024
@nielsdos
Fix phpGH-12837: Combination of phpdbg and Generator method causes me…
4d72bd7 
…mory leak

The fix for bug #72523 (d1dd474) added a check on `zend_execute_ex` to
add an extra refcount to `This`. This is necessary because the VM does a
re-entry here:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4294-L4299

and then cleans up `This` here:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4358-L4360

So if we don't add the refcount, we destroy `This` both in the VM and in
`zend_generator_close`.

The reason this causes a leak in phpdbg is because it changes the
`zend_execute_ex` temporarily back to the default here for `ZEND_DO_FCALL`:
https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/sapi/phpdbg/phpdbg_prompt.c#L1820-L1827
which means that we only execute `OBJ_RELEASE` on the `This` object once
in `zend_generator_close` instead of also doing it in the
`ZEND_DO_FCALL` handler.

To solve this, we make sure the check for the re-entrancy is
consistently done by checking the `ZEND_CALL_TOP` flag instead of
relying solely on `execute_ex` pointer:

https://github.com/php/php-src/blob/ecb90c1db7efe9d87c07e31336185fc393281035/Zend/zend_vm_def.h#L4298
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nielsdos nielsdos

Labels
Bug Category: Generators SAPI: phpdbg Status: Verified
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix GH-12837: Combination of phpdbg and Generator method causes memory leak nielsdos/php-src
6 participants
@ranvis @iluuu1994 @ngyuki @nielsdos @Girgias @SakiTakamachi