From e6cf583160f2054da92426e69143e80ee28e8e16 Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Sat, 12 Feb 2022 01:26:23 +0100 Subject: [PATCH] Fix GH-8082: Prevent leaking memory on observed transient run_time_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. --- NEWS | 2 + Zend/zend.c | 2 - Zend/zend_extensions.c | 11 +- Zend/zend_extensions.h | 1 + Zend/zend_observer.c | 202 +++++++++---------- Zend/zend_observer.h | 1 + ext/zend_test/tests/observer_bug81430_1.phpt | 2 +- ext/zend_test/tests/observer_bug81430_2.phpt | 2 +- main/main.c | 3 + 9 files changed, 112 insertions(+), 114 deletions(-) diff --git a/NEWS b/NEWS index c74365d2e07..9b35952df51 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,8 @@ PHP NEWS - Core: . Fixed Haiku ZTS build. (David Carlier) + . Fixed bug GH-8082 (op_arrays with temporary run_time_cache leak memory + when observed). (Bob) - GD: . Fixed libpng warning when loading interlaced images. (Brett) diff --git a/Zend/zend.c b/Zend/zend.c index cbd5aef42b9..cb245bb7704 100644 --- a/Zend/zend.c +++ b/Zend/zend.c @@ -1215,8 +1215,6 @@ ZEND_API void zend_deactivate(void) /* {{{ */ /* we're no longer executing anything */ EG(current_execute_data) = NULL; - zend_observer_deactivate(); - zend_try { shutdown_scanner(); } zend_end_try(); diff --git a/Zend/zend_extensions.c b/Zend/zend_extensions.c index 4d4b1ffe09b..c3efc0719bc 100644 --- a/Zend/zend_extensions.c +++ b/Zend/zend_extensions.c @@ -267,8 +267,17 @@ ZEND_API int zend_get_resource_handle(const char *module_name) ZEND_API int zend_get_op_array_extension_handle(const char *module_name) { + int handle = zend_op_array_extension_handles++; zend_add_system_entropy(module_name, "zend_get_op_array_extension_handle", &zend_op_array_extension_handles, sizeof(int)); - return zend_op_array_extension_handles++; + return handle; +} + +ZEND_API int zend_get_op_array_extension_handles(const char *module_name, int handles) +{ + int handle = zend_op_array_extension_handles; + zend_op_array_extension_handles += handles; + zend_add_system_entropy(module_name, "zend_get_op_array_extension_handle", &zend_op_array_extension_handles, sizeof(int)); + return handle; } ZEND_API zend_extension *zend_get_extension(const char *extension_name) diff --git a/Zend/zend_extensions.h b/Zend/zend_extensions.h index 9f96d3b0fad..195d508c3f3 100644 --- a/Zend/zend_extensions.h +++ b/Zend/zend_extensions.h @@ -115,6 +115,7 @@ extern ZEND_API int zend_op_array_extension_handles; ZEND_API int zend_get_resource_handle(const char *module_name); ZEND_API int zend_get_op_array_extension_handle(const char *module_name); +ZEND_API int zend_get_op_array_extension_handles(const char *module_name, int handles); ZEND_API void zend_extension_dispatch_message(int message, void *arg); END_EXTERN_C() diff --git a/Zend/zend_observer.c b/Zend/zend_observer.c index b970acd85c8..f3477381078 100644 --- a/Zend/zend_observer.c +++ b/Zend/zend_observer.c @@ -31,28 +31,36 @@ #define ZEND_OBSERVABLE_FN(fn_flags) \ (!(fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE)) -typedef struct _zend_observer_fcall_data { - // points after the last handler - zend_observer_fcall_handlers *end; - // a variadic array using "struct hack" - zend_observer_fcall_handlers handlers[1]; -} zend_observer_fcall_data; - zend_llist zend_observers_fcall_list; zend_llist zend_observer_error_callbacks; int zend_observer_fcall_op_array_extension; -ZEND_TLS zend_arena *fcall_handlers_arena; ZEND_TLS zend_execute_data *first_observed_frame; ZEND_TLS zend_execute_data *current_observed_frame; // Call during minit/startup ONLY -ZEND_API void zend_observer_fcall_register(zend_observer_fcall_init init) { - if (!ZEND_OBSERVER_ENABLED) { - /* We don't want to get an extension handle unless an ext installs an observer */ +ZEND_API void zend_observer_fcall_register(zend_observer_fcall_init init) +{ + zend_llist_add_element(&zend_observers_fcall_list, &init); +} + +// Called by engine before MINITs +ZEND_API void zend_observer_startup(void) +{ + zend_llist_init(&zend_observers_fcall_list, sizeof(zend_observer_fcall_init), NULL, 1); + zend_llist_init(&zend_observer_error_callbacks, sizeof(zend_observer_error_cb), NULL, 1); + + zend_observer_fcall_op_array_extension = -1; +} + +ZEND_API void zend_observer_post_startup(void) +{ + if (zend_observers_fcall_list.count) { + /* We don't want to get an extension handle unless an ext installs an observer + * Allocate each a begin and an end pointer */ zend_observer_fcall_op_array_extension = - zend_get_op_array_extension_handle("Zend Observer"); + zend_get_op_array_extension_handles("Zend Observer", (int) zend_observers_fcall_list.count * 2); /* ZEND_CALL_TRAMPOLINE has SPEC(OBSERVER) but zend_init_call_trampoline_op() * is called before any extensions have registered as an observer. So we @@ -62,123 +70,98 @@ ZEND_API void zend_observer_fcall_register(zend_observer_fcall_init init) { /* ZEND_HANDLE_EXCEPTION also has SPEC(OBSERVER) and no observer extensions * exist when zend_init_exception_op() is called. */ ZEND_VM_SET_OPCODE_HANDLER(EG(exception_op)); - ZEND_VM_SET_OPCODE_HANDLER(EG(exception_op)+1); - ZEND_VM_SET_OPCODE_HANDLER(EG(exception_op)+2); + ZEND_VM_SET_OPCODE_HANDLER(EG(exception_op) + 1); + ZEND_VM_SET_OPCODE_HANDLER(EG(exception_op) + 2); } - zend_llist_add_element(&zend_observers_fcall_list, &init); } -// Called by engine before MINITs -ZEND_API void zend_observer_startup(void) { - zend_llist_init(&zend_observers_fcall_list, sizeof(zend_observer_fcall_init), NULL, 1); - zend_llist_init(&zend_observer_error_callbacks, sizeof(zend_observer_error_cb), NULL, 1); - - zend_observer_fcall_op_array_extension = -1; -} - -ZEND_API void zend_observer_activate(void) { - if (ZEND_OBSERVER_ENABLED) { - fcall_handlers_arena = zend_arena_create(4096); - } else { - fcall_handlers_arena = NULL; - } +ZEND_API void zend_observer_activate(void) +{ first_observed_frame = NULL; current_observed_frame = NULL; } -ZEND_API void zend_observer_deactivate(void) { - if (fcall_handlers_arena) { - zend_arena_destroy(fcall_handlers_arena); - } +ZEND_API void zend_observer_deactivate(void) +{ + // now empty and unused, but kept for ABI compatibility } -ZEND_API void zend_observer_shutdown(void) { +ZEND_API void zend_observer_shutdown(void) +{ zend_llist_destroy(&zend_observers_fcall_list); zend_llist_destroy(&zend_observer_error_callbacks); } -static void zend_observer_fcall_install(zend_execute_data *execute_data) { - zend_llist_element *element; +static void zend_observer_fcall_install(zend_execute_data *execute_data) +{ zend_llist *list = &zend_observers_fcall_list; zend_function *function = execute_data->func; zend_op_array *op_array = &function->op_array; - if (fcall_handlers_arena == NULL) { - return; - } - ZEND_ASSERT(function->type != ZEND_INTERNAL_FUNCTION); - zend_llist handlers_list; - zend_llist_init(&handlers_list, sizeof(zend_observer_fcall_handlers), NULL, 0); - for (element = list->head; element; element = element->next) { + ZEND_ASSERT(RUN_TIME_CACHE(op_array)); + zend_observer_fcall_begin_handler *begin_handlers = (zend_observer_fcall_begin_handler *)&ZEND_OBSERVER_DATA(op_array); + zend_observer_fcall_end_handler *end_handlers = (zend_observer_fcall_end_handler *)begin_handlers + list->count, *end_handlers_start = end_handlers; + + *begin_handlers = ZEND_OBSERVER_NOT_OBSERVED; + *end_handlers = ZEND_OBSERVER_NOT_OBSERVED; + + for (zend_llist_element *element = list->head; element; element = element->next) { zend_observer_fcall_init init; memcpy(&init, element->data, sizeof init); zend_observer_fcall_handlers handlers = init(execute_data); - if (handlers.begin || handlers.end) { - zend_llist_add_element(&handlers_list, &handlers); + if (handlers.begin) { + *(begin_handlers++) = handlers.begin; + } + if (handlers.end) { + *(end_handlers++) = handlers.end; } } - - ZEND_ASSERT(RUN_TIME_CACHE(op_array)); - void *ext; - if (handlers_list.count) { - size_t size = sizeof(zend_observer_fcall_data) + (handlers_list.count - 1) * sizeof(zend_observer_fcall_handlers); - zend_observer_fcall_data *fcall_data = zend_arena_alloc(&fcall_handlers_arena, size); - zend_observer_fcall_handlers *handlers = fcall_data->handlers; - for (element = handlers_list.head; element; element = element->next) { - memcpy(handlers++, element->data, sizeof *handlers); - } - fcall_data->end = handlers; - ext = fcall_data; - } else { - ext = ZEND_OBSERVER_NOT_OBSERVED; + + // end handlers are executed in reverse order + for (--end_handlers; end_handlers_start < end_handlers; --end_handlers, ++end_handlers_start) { + zend_observer_fcall_end_handler tmp = *end_handlers; + *end_handlers = *end_handlers_start; + *end_handlers_start = tmp; } - - ZEND_OBSERVER_DATA(op_array) = ext; - zend_llist_destroy(&handlers_list); } static void ZEND_FASTCALL _zend_observe_fcall_begin(zend_execute_data *execute_data) { - zend_op_array *op_array; - uint32_t fn_flags; - zend_observer_fcall_data *fcall_data; - zend_observer_fcall_handlers *handlers, *end; - if (!ZEND_OBSERVER_ENABLED) { return; } - op_array = &execute_data->func->op_array; - fn_flags = op_array->fn_flags; + zend_op_array *op_array = &execute_data->func->op_array; + uint32_t fn_flags = op_array->fn_flags; if (!ZEND_OBSERVABLE_FN(fn_flags)) { return; } - fcall_data = ZEND_OBSERVER_DATA(op_array); - if (!fcall_data) { + zend_observer_fcall_begin_handler *handler = (zend_observer_fcall_begin_handler *)&ZEND_OBSERVER_DATA(op_array); + if (!*handler) { zend_observer_fcall_install(execute_data); - fcall_data = ZEND_OBSERVER_DATA(op_array); } - ZEND_ASSERT(fcall_data); - if (fcall_data == ZEND_OBSERVER_NOT_OBSERVED) { + zend_observer_fcall_begin_handler *possible_handlers_end = handler + zend_observers_fcall_list.count; + + zend_observer_fcall_end_handler *end_handler = (zend_observer_fcall_end_handler *)possible_handlers_end; + if (*end_handler != ZEND_OBSERVER_NOT_OBSERVED) { + if (first_observed_frame == NULL) { + first_observed_frame = execute_data; + } + current_observed_frame = execute_data; + } + + if (*handler == ZEND_OBSERVER_NOT_OBSERVED) { return; } - if (first_observed_frame == NULL) { - first_observed_frame = execute_data; - } - current_observed_frame = execute_data; - - end = fcall_data->end; - for (handlers = fcall_data->handlers; handlers != end; ++handlers) { - if (handlers->begin) { - handlers->begin(execute_data); - } - } + do { + (*handler)(execute_data); + } while (++handler != possible_handlers_end && *handler != NULL); } ZEND_API void ZEND_FASTCALL zend_observer_generator_resume(zend_execute_data *execute_data) @@ -194,43 +177,48 @@ ZEND_API void ZEND_FASTCALL zend_observer_fcall_begin(zend_execute_data *execute } } -ZEND_API void ZEND_FASTCALL zend_observer_fcall_end( - zend_execute_data *execute_data, - zval *return_value) +static inline bool zend_observer_is_skipped_frame(zend_execute_data *execute_data) { + zend_function *func = execute_data->func; + + if (!func || func->type == ZEND_INTERNAL_FUNCTION || !ZEND_OBSERVABLE_FN(func->common.fn_flags)) { + return true; + } + + zend_observer_fcall_end_handler end_handler = (&ZEND_OBSERVER_DATA(&func->op_array))[zend_observers_fcall_list.count]; + if (end_handler == NULL || end_handler == ZEND_OBSERVER_NOT_OBSERVED) { + return true; + } + + return false; +} + +ZEND_API void ZEND_FASTCALL zend_observer_fcall_end(zend_execute_data *execute_data, zval *return_value) { zend_function *func = execute_data->func; - zend_observer_fcall_data *fcall_data; - zend_observer_fcall_handlers *handlers, *end; if (!ZEND_OBSERVER_ENABLED || !ZEND_OBSERVABLE_FN(func->common.fn_flags)) { return; } - fcall_data = (zend_observer_fcall_data*)ZEND_OBSERVER_DATA(&func->op_array); + zend_observer_fcall_end_handler *handler = (zend_observer_fcall_end_handler *)&ZEND_OBSERVER_DATA(&func->op_array) + zend_observers_fcall_list.count; // TODO: Fix exceptions from generators // ZEND_ASSERT(fcall_data); - if (!fcall_data || fcall_data == ZEND_OBSERVER_NOT_OBSERVED) { + if (!*handler || *handler == ZEND_OBSERVER_NOT_OBSERVED) { return; } - handlers = fcall_data->end; - end = fcall_data->handlers; - while (handlers-- != end) { - if (handlers->end) { - handlers->end(execute_data, return_value); - } - } + zend_observer_fcall_end_handler *possible_handlers_end = handler + zend_observers_fcall_list.count; + do { + (*handler)(execute_data, return_value); + } while (++handler != possible_handlers_end && *handler != NULL); if (first_observed_frame == execute_data) { first_observed_frame = NULL; current_observed_frame = NULL; } else { zend_execute_data *ex = execute_data->prev_execute_data; - while (ex && (!ex->func || ex->func->type == ZEND_INTERNAL_FUNCTION - || !ZEND_OBSERVABLE_FN(ex->func->common.fn_flags) - || !ZEND_OBSERVER_DATA(&ex->func->op_array) - || ZEND_OBSERVER_DATA(&ex->func->op_array) == ZEND_OBSERVER_NOT_OBSERVED)) { + while (ex && zend_observer_is_skipped_frame(ex)) { ex = ex->prev_execute_data; } current_observed_frame = ex; @@ -246,7 +234,6 @@ ZEND_API void zend_observer_fcall_end_all(void) } ex = ex->prev_execute_data; } - current_observed_frame = NULL; } ZEND_API void zend_observer_error_register(zend_observer_error_cb cb) @@ -256,11 +243,8 @@ ZEND_API void zend_observer_error_register(zend_observer_error_cb cb) void zend_observer_error_notify(int type, const char *error_filename, uint32_t error_lineno, zend_string *message) { - zend_llist_element *element; - zend_observer_error_cb callback; - - for (element = zend_observer_error_callbacks.head; element; element = element->next) { - callback = *(zend_observer_error_cb *) (element->data); + for (zend_llist_element *element = zend_observer_error_callbacks.head; element; element = element->next) { + zend_observer_error_cb callback = *(zend_observer_error_cb *) (element->data); callback(type, error_filename, error_lineno, message); } } diff --git a/Zend/zend_observer.h b/Zend/zend_observer.h index cb29729ec45..2a2fce07611 100644 --- a/Zend/zend_observer.h +++ b/Zend/zend_observer.h @@ -56,6 +56,7 @@ typedef zend_observer_fcall_handlers (*zend_observer_fcall_init)(zend_execute_da ZEND_API void zend_observer_fcall_register(zend_observer_fcall_init); ZEND_API void zend_observer_startup(void); // Called by engine before MINITs +ZEND_API void zend_observer_post_startup(void); // Called by engine after MINITs ZEND_API void zend_observer_activate(void); ZEND_API void zend_observer_deactivate(void); ZEND_API void zend_observer_shutdown(void); diff --git a/ext/zend_test/tests/observer_bug81430_1.phpt b/ext/zend_test/tests/observer_bug81430_1.phpt index cac53ef70cb..1d1e3c4256e 100644 --- a/ext/zend_test/tests/observer_bug81430_1.phpt +++ b/ext/zend_test/tests/observer_bug81430_1.phpt @@ -1,7 +1,7 @@ --TEST-- Bug #81430 (Attribute instantiation frame accessing invalid frame pointer) --EXTENSIONS-- -zend_test +zend-test --INI-- memory_limit=20M zend_test.observer.enabled=1 diff --git a/ext/zend_test/tests/observer_bug81430_2.phpt b/ext/zend_test/tests/observer_bug81430_2.phpt index 4d56248a80f..a575fb9d7a0 100644 --- a/ext/zend_test/tests/observer_bug81430_2.phpt +++ b/ext/zend_test/tests/observer_bug81430_2.phpt @@ -1,7 +1,7 @@ --TEST-- Bug #81430 (Attribute instantiation leaves dangling execute_data pointer) --EXTENSIONS-- -zend_test +zend-test --INI-- memory_limit=20M zend_test.observer.enabled=1 diff --git a/main/main.c b/main/main.c index 073d260ac68..7bd5400760f 100644 --- a/main/main.c +++ b/main/main.c @@ -2280,6 +2280,9 @@ int php_module_startup(sapi_module_struct *sf, zend_module_entry *additional_mod module->version = PHP_VERSION; module->info_func = PHP_MINFO(php_core); } + + /* freeze the list of observer fcall_init handlers */ + zend_observer_post_startup(); /* Extensions that add engine hooks after this point do so at their own peril */ zend_finalize_system_id();