Skip to content

Commit

Permalink
Fix GH-8646: Memory leak PHP FPM 8.1
Browse files Browse the repository at this point in the history
Fixes GH-8646
See #8646 for thorough discussion.

Interned strings that hold class entries can get a corresponding slot in map_ptr for the CE cache.
map_ptr works like a bump allocator: there is a counter which increases to allocate the next slot in the map.

For class name strings in non-opcache we have:
  - on startup: permanent + interned
  - on request: interned
For class name strings in opcache we have:
  - on startup: permanent + interned
  - on request: either not interned at all, which we can ignore because they won't get a CE cache entry
                or they were already permanent + interned
                or we get a new permanent + interned string in the opcache persistence code

Notice that the map_ptr layout always has the permanent strings first, and the request strings after.
In non-opcache, a request string may get a slot in map_ptr, and that interned request string
gets destroyed at the end of the request. The corresponding map_ptr slot can thereafter never be used again.
This causes map_ptr to keep reallocating to larger and larger sizes.

We solve it as follows:
We can check whether we had any interned request strings, which only happens in non-opcache.
If we have any, we reset map_ptr to the last permanent string.
We can't lose any permanent strings because of map_ptr's layout.

Closes GH-10783.
  • Loading branch information
nielsdos committed Mar 7, 2023
1 parent cfe1aab commit ff62d11
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 3 deletions.
1 change: 1 addition & 0 deletions NEWS
Expand Up @@ -6,6 +6,7 @@ PHP NEWS
. Added optional support for max_execution_time in ZTS/Linux builds
(Kévin Dunglas)
. Fixed use-after-free in recursive AST evaluation. (ilutov)
. Fixed bug GH-8646 (Memory leak PHP FPM 8.1). (nielsdos)

- FTP:
. Propagate success status of ftp_close(). (nielsdos)
Expand Down
28 changes: 28 additions & 0 deletions Zend/zend.c
Expand Up @@ -1286,6 +1286,34 @@ ZEND_API void zend_deactivate(void) /* {{{ */

zend_destroy_rsrc_list(&EG(regular_list));

/* See GH-8646: https://github.com/php/php-src/issues/8646
*
* Interned strings that hold class entries can get a corresponding slot in map_ptr for the CE cache.
* map_ptr works like a bump allocator: there is a counter which increases to allocate the next slot in the map.
*
* For class name strings in non-opcache we have:
* - on startup: permanent + interned
* - on request: interned
* For class name strings in opcache we have:
* - on startup: permanent + interned
* - on request: either not interned at all, which we can ignore because they won't get a CE cache entry
* or they were already permanent + interned
* or we get a new permanent + interned string in the opcache persistence code
*
* Notice that the map_ptr layout always has the permanent strings first, and the request strings after.
* In non-opcache, a request string may get a slot in map_ptr, and that interned request string
* gets destroyed at the end of the request. The corresponding map_ptr slot can thereafter never be used again.
* This causes map_ptr to keep reallocating to larger and larger sizes.
*
* We solve it as follows:
* We can check whether we had any interned request strings, which only happens in non-opcache.
* If we have any, we reset map_ptr to the last permanent string.
* We can't lose any permanent strings because of map_ptr's layout.
*/
if (zend_hash_num_elements(&CG(interned_strings)) > 0) {
zend_map_ptr_reset();
}

#if GC_BENCH
fprintf(stderr, "GC Statistics\n");
fprintf(stderr, "-------------\n");
Expand Down
6 changes: 6 additions & 0 deletions ext/zend_test/test.c
Expand Up @@ -321,6 +321,12 @@ static ZEND_FUNCTION(zend_test_parameter_with_attribute)
RETURN_LONG(1);
}

static ZEND_FUNCTION(zend_get_map_ptr_last)
{
ZEND_PARSE_PARAMETERS_NONE();
RETURN_LONG(CG(map_ptr_last));
}

static zend_object *zend_test_class_new(zend_class_entry *class_type)
{
zend_object *obj = zend_objects_new(class_type);
Expand Down
2 changes: 2 additions & 0 deletions ext/zend_test/test.stub.php
Expand Up @@ -115,6 +115,8 @@ function zend_test_parameter_with_attribute(string $parameter): int {}
function zend_get_current_func_name(): string {}

function zend_call_method(string $class, string $method, mixed $arg1 = UNKNOWN, mixed $arg2 = UNKNOWN): mixed {}

function zend_get_map_ptr_last(): int {}
}

namespace ZendTestNS {
Expand Down
10 changes: 7 additions & 3 deletions ext/zend_test/test_arginfo.h
@@ -1,5 +1,5 @@
/* This is a generated file, edit the .stub.php file instead.
* Stub hash: 27df6a7b48574b5c6c9a54c618fce300c7a8bd13 */
* Stub hash: 1eca5b01969498e67501a59dc69ba4c01263c4d9 */

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_test_array_return, 0, 0, IS_ARRAY, 0)
ZEND_END_ARG_INFO()
Expand Down Expand Up @@ -79,12 +79,14 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_call_method, 0, 2, IS_MIXED
ZEND_ARG_TYPE_INFO(0, arg2, IS_MIXED, 0)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_ZendTestNS2_ZendSubNS_namespaced_func, 0, 0, _IS_BOOL, 0)
ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_get_map_ptr_last, 0, 0, IS_LONG, 0)
ZEND_END_ARG_INFO()

ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class__ZendTestClass_is_object, 0, 0, IS_LONG, 0)
ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_ZendTestNS2_ZendSubNS_namespaced_func, 0, 0, _IS_BOOL, 0)
ZEND_END_ARG_INFO()

#define arginfo_class__ZendTestClass_is_object arginfo_zend_get_map_ptr_last

ZEND_BEGIN_ARG_INFO_EX(arginfo_class__ZendTestClass___toString, 0, 0, 0)
ZEND_END_ARG_INFO()

Expand Down Expand Up @@ -136,6 +138,7 @@ static ZEND_FUNCTION(zend_get_unit_enum);
static ZEND_FUNCTION(zend_test_parameter_with_attribute);
static ZEND_FUNCTION(zend_get_current_func_name);
static ZEND_FUNCTION(zend_call_method);
static ZEND_FUNCTION(zend_get_map_ptr_last);
static ZEND_FUNCTION(namespaced_func);
static ZEND_METHOD(_ZendTestClass, is_object);
static ZEND_METHOD(_ZendTestClass, __toString);
Expand Down Expand Up @@ -173,6 +176,7 @@ static const zend_function_entry ext_functions[] = {
ZEND_FE(zend_test_parameter_with_attribute, arginfo_zend_test_parameter_with_attribute)
ZEND_FE(zend_get_current_func_name, arginfo_zend_get_current_func_name)
ZEND_FE(zend_call_method, arginfo_zend_call_method)
ZEND_FE(zend_get_map_ptr_last, arginfo_zend_get_map_ptr_last)
ZEND_NS_FE("ZendTestNS2\\ZendSubNS", namespaced_func, arginfo_ZendTestNS2_ZendSubNS_namespaced_func)
ZEND_FE_END
};
Expand Down
52 changes: 52 additions & 0 deletions sapi/fpm/tests/gh8646.phpt
@@ -0,0 +1,52 @@
--TEST--
GH-8646 (Memory leak PHP FPM 8.1)
--EXTENSIONS--
zend_test
--SKIPIF--
<?php include "skipif.inc"; ?>
--FILE--
<?php

require_once "tester.inc";

$cfg = <<<EOT
[global]
error_log = {{FILE:LOG}}
[unconfined]
listen = {{ADDR}}
pm = dynamic
pm.max_children = 5
pm.start_servers = 1
pm.min_spare_servers = 1
pm.max_spare_servers = 3
EOT;

$code = <<<EOT
<?php
class MyClass {}
echo zend_get_map_ptr_last();
EOT;

$tester = new FPM\Tester($cfg, $code);
$tester->start();
$tester->expectLogStartNotices();
$map_ptr_last_values = [];
for ($i = 0; $i < 10; $i++) {
$map_ptr_last_values[] = (int) $tester->request()->getBody();
}
// Ensure that map_ptr_last did not increase
var_dump(count(array_unique($map_ptr_last_values, SORT_REGULAR)) === 1);
$tester->terminate();
$tester->expectLogTerminatingNotices();
$tester->close();

?>
Done
--EXPECT--
bool(true)
Done
--CLEAN--
<?php
require_once "tester.inc";
FPM\Tester::clean();
?>

0 comments on commit ff62d11

Please sign in to comment.