[go: up one dir, main page]
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

op_arrays with temporary run_time_cache leak memory when observed #8082

Closed
bwoebi opened this issue Feb 11, 2022 · 0 comments
Closed

op_arrays with temporary run_time_cache leak memory when observed #8082

bwoebi opened this issue Feb 11, 2022 · 0 comments
Labels
Bug Category: Engine Status: Verified

Comments

@bwoebi
Copy link
Member
bwoebi commented Feb 11, 2022 •
edited
Loading

Description

The following code leaks memory when observed:

<?php

$closure = function() { /* observe this */ };
$object = new class {};
$mem = memory_get_usage();
while (true) {
    if ($mem != memory_get_usage()) {
        var_dump($mem = memory_get_usage());
    }
    $closure->call($object);
}

To reproduce, run with php -dzend_test.observer.enabled=1 -dzend_test.observer.observe_all=1 -dzend_test.observer.show_output=0 and see memory consumption explode.

PHP Version

PHP8.0 - master

Analysis

Observing op_arrays arena allocates some memory, which is never freed, even if the specific op_array (or more precisely run_time_cache) is freed.

We are constrained by the fact that run_time_cache is caller managed memory currently. Doing something else is an ABI or API break and not viable in PHP 8.0 or 8.1.

The only solution which comes to my mind is allocating enough space in the run_time_cache (e.g. at its end) to fit handlers there, i.e. 16 bytes (two pointers, start and end) per registered observing initializer instead of arena allocating that memory. This would be achieved by an increase of cache_size by the appropriate amount then.
This should also be safe regarding the application interface, especially given that the structs are not exported.

For PHP 8.2 one might consider a cleaner solution then.
bwoebi added a commit to bwoebi/php-src that referenced this issue Feb 12, 2022
@bwoebi
Fix phpGH-8082: Prevent leaking memory on observed transient run_time…
887d983 
…_caches

This is achieved by tracking the observers on the run_time_cache (with a fixed amount of slots, 2 for each observer).
That way round, if the run_time_cache is freed all associated observer data is as well.

This approach has been chosen, as to avoid any ABI or API breakage.
Future versions may for example choose to provide a hookable API for run_time_cache freeing or similar.
bwoebi added a commit to bwoebi/php-src that referenced this issue Feb 12, 2022
@bwoebi
Fix phpGH-8082: Prevent leaking memory on observed transient run_time…
277598c 
…_caches

This is achieved by tracking the observers on the run_time_cache (with a fixed amount of slots, 2 for each observer).
That way round, if the run_time_cache is freed all associated observer data is as well.

This approach has been chosen, as to avoid any ABI or API breakage.
Future versions may for example choose to provide a hookable API for run_time_cache freeing or similar.
bwoebi added a commit to bwoebi/php-src that referenced this issue Feb 12, 2022
@bwoebi
Fix phpGH-8082: Prevent leaking memory on observed transient run_time…
0364279 
…_caches

This is achieved by tracking the observers on the run_time_cache (with a fixed amount of slots, 2 for each observer).
That way round, if the run_time_cache is freed all associated observer data is as well.

This approach has been chosen, as to avoid any ABI or API breakage.
Future versions may for example choose to provide a hookable API for run_time_cache freeing or similar.
bwoebi added a commit to bwoebi/php-src that referenced this issue Feb 12, 2022
@bwoebi
Fix phpGH-8082: Prevent leaking memory on observed transient run_time…
cb80c30 
…_caches

This is achieved by tracking the observers on the run_time_cache (with a fixed amount of slots, 2 for each observer).
That way round, if the run_time_cache is freed all associated observer data is as well.

This approach has been chosen, as to avoid any ABI or API breakage.
Future versions may for example choose to provide a hookable API for run_time_cache freeing or similar.
bwoebi added a commit to bwoebi/php-src that referenced this issue Feb 12, 2022
@bwoebi
Fix phpGH-8082: Prevent leaking memory on observed transient run_time…
7c91d6f 
…_caches

This is achieved by tracking the observers on the run_time_cache (with a fixed amount of slots, 2 for each observer).
That way round, if the run_time_cache is freed all associated observer data is as well.

This approach has been chosen, as to avoid any ABI or API breakage.
Future versions may for example choose to provide a hookable API for run_time_cache freeing or similar.
@bwoebi bwoebi mentioned this issue Feb 12, 2022
Closed
@bwoebi bwoebi closed this as completed in e6cf583 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Category: Engine Status: Verified
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@cmb69 @bwoebi and others