8000 memory leak when using fopen with a stream wrapper · Issue #8548 · php/php-src · GitHub
[go: up one dir, main page]
Skip to content

memory leak when using fopen with a stream wrapper #8548

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
Closed
manueldimmler opened this issue May 13, 2022 · 12 comments
Closed

memory leak when using fopen with a stream wrapper #8548

manueldimmler opened this issue May 13, 2022 · 12 comments
Assignees
@iluuu1994
Labels
Bug Category: Streams Status: Verified

Comments

@manueldimmler
Copy link

Description

The following code takes up a lot of memory and keeps increasing in a endless loop:

#!/usr/bin/php
<?php

class Wrapper
{

    public function stream_open(string $path, string $mode, int $options): bool
    {
        return true;
    }

}

$filename = tempnam(__DIR__, '');
touch($filename);

$wrapper = new Wrapper();
$counter = 0;
while ($counter < 1_000_000) {
    $counter++;
    stream_wrapper_register('symfony', Wrapper::class);
    fopen('symfony://' . $filename, 'r');
    stream_wrapper_unregister('symfony');
}

memory_leak_01

With a small change, the problem disappears:

#!/usr/bin/php
<?php

class Wrapper
{

    public function stream_open(string $path, string $mode, int $options): bool
    {
        return true;
    }

}

$filename = tempnam(__DIR__, '');
touch($filename);

$wrapper = new Wrapper();
$counter = 0;
stream_wrapper_register('symfony', Wrapper::class);
while ($counter < 1_000_000) {
    $counter++;
    fopen('symfony://' . $filename, 'r');
}
stream_wrapper_unregister('symfony');

memory_leak_02

The memory leak was described in the following ticket: symfony/symfony#46256

PHP Version

PHP 8.1.5 (cli)

Operating System

Ubuntu 20.04
@cmb69
Copy link
Member
cmb69 commented May 16, 2022

The problem is that stream_wrapper_register() allocates a resouce, but that is not deleted in stream_wrapper_unregister() (but only during shutdown).

Note that even when this will be fixed, it's still not a good idea to repeatly register and unregister stream wrappers, because of potential resource ID depletion.

@iluuu1994 iluuu1994 self-assigned this May 19, 2022
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue May 19, 2022
@iluuu1994
Fix stream_wrapper_unregister() resource leak
d3952a1 
Closes phpGH-8548
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue May 19, 2022
@iluuu1994
Fix stream_wrapper_unregister() resource leak
914bf04 
Closes phpGH-8548
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue May 20, 2022
@iluuu1994
Fix stream_wrapper_unregister() resource leak
baa8ad6 
Closes phpGH-8548
@iluuu1994 iluuu1994 mentioned this issue May 20, 2022
Closed
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue May 20, 2022
@iluuu1994
Fix stream_wrapper_unregister() resource leak
e0b2ce5 
Closes phpGH-8548
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue May 20, 2022
@iluuu1994
Fix stream_wrapper_unregister() resource leak
c47bab2 
Closes phpGH-8548
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue May 21, 2022
@iluuu1994
Fix stream_wrapper_unregister() resource leak
6f1db1a 
Closes phpGH-8548
@iluuu1994 iluuu1994 8000 closed this as completed in a5a89cc May 21, 2022
@olsavmic
Copy link
Contributor
olsavmic commented Aug 1, 2022 •
edited
Loading

@iluuu1994 Is there any possibility of backporting/releasing this in 8.0.*/8.1.*?

Thanks!

@iluuu1994
Copy link
Member

@olsavmic Since this is a rather theoretical and old leak I decided not to. Are you hitting this issue?

@olsavmic
Copy link
Contributor
olsavmic commented Aug 1, 2022

Yes, pretty heavily due to symfony/http-client (symfony/symfony#46256). We have switched to guzzle client implementation which does not suffer from this issue so it's no longer critical for us but it's still an issue.

I will suggest a singleton-like approach for the stream_wrapper_registrater call in the Symfony repository but I don't see into the internals of streams and linux resource ids. I'd assume that resource id is freed after unregistration but perhaps that's not the case?

@iluuu1994
Copy link
Member

Oh, I wasn't aware Symfony actually unregisters the stream wrapper on every call.

@nicolas-grekas Is unregistering really necessary or could Symfony check if the stream wrapper has already been registered?

If not we should probably be able to back-port a5a89cc and c019421 since there aren't any public ABI breaks. @cmb69 Would that be ok for you?

@cmb69
Copy link
Member
cmb69 commented Aug 1, 2022

I'm not hard against back-porting, but I really think this should be fixed in userland; repeatedly registering and unregistering stream wrappers is not a good idea anyway, because of potential resource ID depletion.

@nicolas-grekas
Copy link
Contributor

The goal of this code is to not leak anything in the global scope (aka not allow anyone but internal code to fopen() that kind of streams.) I didn't know this had any effect on resource ids. I'll look for changing this.

@cmb69
Copy link
Member
cmb69 commented Aug 1, 2022

I didn't know this had any effect on resource ids.

All resources (regardless of kind) are internally stored in a hash table (see get_resources()), and are basically managed with append ($a[] = …) and unset operations. As of PHP 8.1.0, resource ID depletion is actually no longer a concern, but prior to that version, there couldn't be more than INT_MAX resources, what could cause real issues for long running processes.

@nicolas-grekas
Copy link
Contributor

Sure, but what's the relationship with repeatedly registering and unregistering stream wrappers?

@cmb69
Copy link
Member
cmb69 commented Aug 1, 2022

stream_wrapper_register() registers a new resource internally:

php-src/main/streams/userspace.c

Line 500 in 50bd8ba

rsrc = zend_register_resource(uwrap, le_protocols);

@cmb69
Copy link
Member
cmb69 commented Aug 1, 2022

stream_wrapper_register() registers a new resource internally:

php-src/main/streams/userspace.c

Line 500 in 50bd8ba

rsrc = zend_register_resource(uwrap, le_protocols);

PS: might be worth documenting this.

@nicolas-grekas nicolas-grekas mentioned this issue Aug 1, 2022
Merged
@nicolas-grekas
Copy link
Contributor

Thanks for the hints. This should be fixed for Symfony in symfony/symfony#47143

nicolas-grekas added a commit to symfony/symfony that referenced this issue Aug 1, 2022
@nicolas-grekas
bug #47143 [HttpClient] Fix memory leak when using StreamWrapper (nic…
25c2131 
…olas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpClient] Fix memory leak when using StreamWrapper

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #46256
| License       | MIT
| Doc PR        | -

As hinted in php/php-src#8548

Commits
-------

16d0176 [HttpClient] Fix memory leak when using StreamWrapper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@iluuu1994 iluuu1994

Labels
Bug Category: Streams Status: Verified
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix stream_wrapper_unregister() resource leak iluuu1994/php-src
5 participants
@nicolas-grekas @iluuu1994 @cmb69 @olsavmic @manueldimmler
0