From 69671582e7723e1b8be1e77d17280a9c405da035 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 4 Mar 2023 23:22:05 +0100 Subject: [PATCH] Fix GH-8646: Memory leak PHP FPM 8.1 Fixes GH-8646 See https://github.com/php/php-src/issues/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. --- Zend/zend.c | 28 +++++++++++++++++++ ext/zend_test/test.c | 6 +++++ ext/zend_test/test.stub.php | 2 ++ ext/zend_test/test_arginfo.h | 10 ++++--- sapi/fpm/tests/gh8646.phpt | 52 ++++++++++++++++++++++++++++++++++++ 5 files changed, 95 insertions(+), 3 deletions(-) create mode 100644 sapi/fpm/tests/gh8646.phpt diff --git a/Zend/zend.c b/Zend/zend.c index bd2a04e2ba9d8..b14b2c701a5b2 100644 --- a/Zend/zend.c +++ b/Zend/zend.c @@ -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"); diff --git a/ext/zend_test/test.c b/ext/zend_test/test.c index 6c5bc119ab31b..ecfef09c6f7ed 100644 --- a/ext/zend_test/test.c +++ b/ext/zend_test/test.c @@ -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); diff --git a/ext/zend_test/test.stub.php b/ext/zend_test/test.stub.php index 9a49d31fa97d8..bc6a9d54e3bfb 100644 --- a/ext/zend_test/test.stub.php +++ b/ext/zend_test/test.stub.php @@ -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 { diff --git a/ext/zend_test/test_arginfo.h b/ext/zend_test/test_arginfo.h index ebc4076daca82..5a7cc67b06d30 100644 --- a/ext/zend_test/test_arginfo.h +++ b/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() @@ -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() @@ -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); @@ -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 }; diff --git a/sapi/fpm/tests/gh8646.phpt b/sapi/fpm/tests/gh8646.phpt new file mode 100644 index 0000000000000..c35b57ec916c2 --- /dev/null +++ b/sapi/fpm/tests/gh8646.phpt @@ -0,0 +1,52 @@ +--TEST-- +GH-8646 (Memory leak PHP FPM 8.1) +--EXTENSIONS-- +zend_test +--SKIPIF-- + +--FILE-- +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-- +