From a22a87243ff3c1c3a376b855da0ea95fcf6bdf66 Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Sun, 7 Apr 2024 14:06:25 +0200 Subject: [PATCH] Add next handler parameter to zend_observer_remove_begin/end_handler (#13807) 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 4928592134a..49ca2873b59 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 2cb4db91475..1ef244f7baf 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 fc4d9a62c89..1b6cac19449 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 3e870de450a..9bcbd8ee6d6 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(); }