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.

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

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

Comments

@bwoebi
Copy link
Member

bwoebi commented Feb 11, 2022

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
…_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
…_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
…_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
…_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
…_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 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
Projects
None yet
Development

No branches or pull requests

3 participants
@cmb69 @bwoebi and others