Add next handler parameter to zend_observer_remove_begin/end_handler (php#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 <bobwei9@hotmail.com>

Author: bwoebi

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

Zend/zend_observer.c

@@ -147,19 +147,26 @@ 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;
 	for (void **cur_handler = first_handler; cur_handler <= last_handler; ++cur_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) {

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

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();
 	}