From f6082c2f63c2a1f15f41ce762dd6b9f69afa6ab9 Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Mon, 25 Mar 2024 23:39:54 +0100 Subject: [PATCH] Add next handler parameter to zend_observer_remove_begin/end_handler The usage of the current API within an observer handler leads to bugs like https://bugs.xdebug.org/view.php?id=2232. Given two observer handlers, next to each other. The first one is executed. It removes itself. The second observer handler gets moved to the place of the first. The first one returns. The handler in the second slot is fetched, but is now NULL, because the it's now in the slot of the first observer; i.e. the second handler is skipped and the begin/end symmetry guarantee is violated. Providing the next handler to the caller is a zero-cost way to avoid any impact in the paths of zend_observe_fcall_begin/end. Signed-off-by: Bob Weinand --- UPGRADING.INTERNALS | 4 ++++ Zend/zend_observer.c | 17 ++++++++++++----- Zend/zend_observer.h | 4 ++-- ext/zend_test/observer.c | 5 +++-- 4 files changed, 21 insertions(+), 9 deletions(-) diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS index 3038a5e718d59..7b04d83bd8c2e 100644 --- a/UPGRADING.INTERNALS +++ b/UPGRADING.INTERNALS @@ -77,6 +77,10 @@ PHP 8.4 INTERNALS UPGRADE NOTES void(*)(void *, size_t) to void(*)(void *, size_t ZEND_FILE_LINE_DC ZEND_FILE_LINE_ORIG_DC) +* zend_observer_remove_begin_handler() and zend_observer_remove_end_handler() + got each a new parameter returning an observer which must be called, if the + removal happened during observer execution. + ======================== 2. Build system changes diff --git a/Zend/zend_observer.c b/Zend/zend_observer.c index 2cb4db914758a..1ef244f7baf83 100644 --- a/Zend/zend_observer.c +++ b/Zend/zend_observer.c @@ -147,7 +147,12 @@ static void zend_observer_fcall_install(zend_execute_data *execute_data) } } -static bool zend_observer_remove_handler(void **first_handler, void *old_handler) { +/* We need to provide the ability to retrieve the handler which will move onto the position the current handler was. + * The fundamental problem is that, if a handler is removed while it's being executed, it will move handlers around: + * the previous next handler is now at the place where the current handler was. + * Hence, the next handler executed will be the one after the next handler. + * Callees must thus invoke the next handler themselves, with the same arguments they were passed. */ +static bool zend_observer_remove_handler(void **first_handler, void *old_handler, void **next_handler) { size_t registered_observers = zend_observers_fcall_list.count; void **last_handler = first_handler + registered_observers - 1; @@ -155,11 +160,13 @@ static bool zend_observer_remove_handler(void **first_handler, void *old_handler if (*cur_handler == old_handler) { if (registered_observers == 1 || (cur_handler == first_handler && cur_handler[1] == NULL)) { *cur_handler = ZEND_OBSERVER_NOT_OBSERVED; + *next_handler = NULL; } else { if (cur_handler != last_handler) { memmove(cur_handler, cur_handler + 1, sizeof(cur_handler) * (last_handler - cur_handler)); } *last_handler = NULL; + *next_handler = *cur_handler; } return true; } @@ -184,8 +191,8 @@ ZEND_API void zend_observer_add_begin_handler(zend_function *function, zend_obse } } -ZEND_API bool zend_observer_remove_begin_handler(zend_function *function, zend_observer_fcall_begin_handler begin) { - return zend_observer_remove_handler((void **)&ZEND_OBSERVER_DATA(function), begin); +ZEND_API bool zend_observer_remove_begin_handler(zend_function *function, zend_observer_fcall_begin_handler begin, zend_observer_fcall_begin_handler *next) { + return zend_observer_remove_handler((void **)&ZEND_OBSERVER_DATA(function), begin, (void **)next); } ZEND_API void zend_observer_add_end_handler(zend_function *function, zend_observer_fcall_end_handler end) { @@ -200,9 +207,9 @@ ZEND_API void zend_observer_add_end_handler(zend_function *function, zend_observ *end_handler = end; } -ZEND_API bool zend_observer_remove_end_handler(zend_function *function, zend_observer_fcall_end_handler end) { +ZEND_API bool zend_observer_remove_end_handler(zend_function *function, zend_observer_fcall_end_handler end, zend_observer_fcall_end_handler *next) { size_t registered_observers = zend_observers_fcall_list.count; - return zend_observer_remove_handler((void **)&ZEND_OBSERVER_DATA(function) + registered_observers, end); + return zend_observer_remove_handler((void **)&ZEND_OBSERVER_DATA(function) + registered_observers, end, (void **)next); } static inline zend_execute_data **prev_observed_frame(zend_execute_data *execute_data) { diff --git a/Zend/zend_observer.h b/Zend/zend_observer.h index fc4d9a62c8905..1b6cac194493e 100644 --- a/Zend/zend_observer.h +++ b/Zend/zend_observer.h @@ -62,9 +62,9 @@ ZEND_API void zend_observer_fcall_register(zend_observer_fcall_init); // Call during runtime, but only if you have used zend_observer_fcall_register. // You must not have more than one begin and one end handler active at the same time. Remove the old one first, if there is an existing one. ZEND_API void zend_observer_add_begin_handler(zend_function *function, zend_observer_fcall_begin_handler begin); -ZEND_API bool zend_observer_remove_begin_handler(zend_function *function, zend_observer_fcall_begin_handler begin); +ZEND_API bool zend_observer_remove_begin_handler(zend_function *function, zend_observer_fcall_begin_handler begin, zend_observer_fcall_begin_handler *next); ZEND_API void zend_observer_add_end_handler(zend_function *function, zend_observer_fcall_end_handler end); -ZEND_API bool zend_observer_remove_end_handler(zend_function *function, zend_observer_fcall_end_handler end); +ZEND_API bool zend_observer_remove_end_handler(zend_function *function, zend_observer_fcall_end_handler end, zend_observer_fcall_end_handler *next); 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 diff --git a/ext/zend_test/observer.c b/ext/zend_test/observer.c index 3e870de450a5a..9bcbd8ee6d66d 100644 --- a/ext/zend_test/observer.c +++ b/ext/zend_test/observer.c @@ -297,8 +297,9 @@ static ZEND_INI_MH(zend_test_observer_OnUpdateCommaList) if (stage != PHP_INI_STAGE_STARTUP && stage != PHP_INI_STAGE_ACTIVATE && stage != PHP_INI_STAGE_DEACTIVATE && stage != PHP_INI_STAGE_SHUTDOWN) { ZEND_HASH_FOREACH_STR_KEY(*p, funcname) { if ((func = zend_hash_find_ptr(EG(function_table), funcname))) { - zend_observer_remove_begin_handler(func, observer_begin); - zend_observer_remove_end_handler(func, observer_end); + void *old_handler; + zend_observer_remove_begin_handler(func, observer_begin, (zend_observer_fcall_begin_handler *)&old_handler); + zend_observer_remove_end_handler(func, observer_end, (zend_observer_fcall_end_handler *)&old_handler); } } ZEND_HASH_FOREACH_END(); }