[PATCH wayland] server: allow removing any wl_listener from a signal when emitted

Giulio Camuffo giuliocamuffo at gmail.com
Tue Sep 6 07:06:16 UTC 2016


wl_list_for_each_safe, which was used by wl_signal_emit is not really
safe. If a signal has two listeners, and the first one removes and
re-inits the second one, it would enter an infinite loop.
This commit adds a wl_list_iterate_safe() function which is used by
wl_signal_emit() and adds a test confirming it works.
wl_signal_emit had to be de-inlined because we need an iterator
function, which should stay private.
---
 src/wayland-server-core.h | 17 ++---------
 src/wayland-server.c      | 21 ++++++++++++++
 src/wayland-util.c        | 29 +++++++++++++++++++
 src/wayland-util.h        | 29 +++++++++++--------
 tests/signal-test.c       | 72 +++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 142 insertions(+), 26 deletions(-)

diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
index 2c215e4..a0a3ead 100644
--- a/src/wayland-server-core.h
+++ b/src/wayland-server-core.h
@@ -341,21 +341,8 @@ wl_signal_get(struct wl_signal *signal, wl_notify_func_t notify)
 	return NULL;
 }
 
-/** Emits this signal, notifying all registered listeners.
- *
- * \param signal The signal object that will emit the signal
- * \param data The data that will be emitted with the signal
- *
- * \memberof wl_signal
- */
-static inline void
-wl_signal_emit(struct wl_signal *signal, void *data)
-{
-	struct wl_listener *l, *next;
-
-	wl_list_for_each_safe(l, next, &signal->listener_list, link)
-		l->notify(l, data);
-}
+void
+wl_signal_emit(struct wl_signal *signal, void *data);
 
 typedef void (*wl_resource_destroy_func_t)(struct wl_resource *resource);
 
diff --git a/src/wayland-server.c b/src/wayland-server.c
index 9ecfd97..93e838a 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -1742,6 +1742,27 @@ wl_client_for_each_resource(struct wl_client *client,
 	wl_map_for_each(&client->objects, resource_iterator_helper, &context);
 }
 
+static enum wl_iterator_result
+notify_listener(struct wl_list *link, void *data)
+{
+	struct wl_listener *l = wl_container_of(link, l, link);
+	l->notify(l, data);
+	return WL_ITERATOR_CONTINUE;
+}
+
+/** Emits this signal, notifying all registered listeners.
+ *
+ * \param signal The signal object that will emit the signal
+ * \param data The data that will be emitted with the signal
+ *
+ * \memberof wl_signal
+ */
+WL_EXPORT void
+wl_signal_emit(struct wl_signal *signal, void *data)
+{
+	wl_list_iterate_safe(&signal->listener_list, notify_listener, data);
+}
+
 /** \cond */ /* Deprecated functions below. */
 
 uint32_t
diff --git a/src/wayland-util.c b/src/wayland-util.c
index 639ccf8..c6193b7 100644
--- a/src/wayland-util.c
+++ b/src/wayland-util.c
@@ -93,6 +93,35 @@ wl_list_insert_list(struct wl_list *list, struct wl_list *other)
 }
 
 WL_EXPORT void
+wl_list_iterate_safe(struct wl_list *list,
+		     wl_list_iterator_func_t it, void *data)
+{
+	struct wl_list temp;
+	struct wl_list *pos;
+
+	wl_list_init(&temp);
+
+	/* Take every element out of the list and put them in a temporary list.
+	 * This way, the 'it' func can remove any element it wants from the list
+	 * without troubles, because we always get the first element, not the
+	 * one after the current, which may be invalid.
+	 * wl_list_for_each_safe tries to be safe but it fails: it works fine
+	 * if the current item is removed, but not if the next one is. */
+	while (!wl_list_empty(list)) {
+		pos = list->next;
+
+		wl_list_remove(pos);
+		wl_list_insert(&temp, pos);
+
+		if (it(pos, data) == WL_ITERATOR_STOP)
+			break;
+	}
+
+	/* Now add back all the items in the original list. */
+	wl_list_insert_list(list, &temp);
+}
+
+WL_EXPORT void
 wl_array_init(struct wl_array *array)
 {
 	memset(array, 0, sizeof *array);
diff --git a/src/wayland-util.h b/src/wayland-util.h
index cacc122..34697b5 100644
--- a/src/wayland-util.h
+++ b/src/wayland-util.h
@@ -76,6 +76,17 @@ struct wl_interface {
 	const struct wl_message *events;
 };
 
+/** \enum wl_iterator_result
+ *
+ * This enum represents the return value of an iterator function.
+ */
+enum wl_iterator_result {
+	/** Stop the iteration */
+	WL_ITERATOR_STOP,
+	/** Continue the iteration */
+	WL_ITERATOR_CONTINUE
+};
+
 /** \class wl_list
  *
  * \brief doubly-linked list
@@ -139,6 +150,13 @@ wl_list_empty(const struct wl_list *list);
 void
 wl_list_insert_list(struct wl_list *list, struct wl_list *other);
 
+typedef enum wl_iterator_result (*wl_list_iterator_func_t)(struct wl_list *elm,
+							   void *data);
+
+void
+wl_list_iterate_safe(struct wl_list *list,
+		     wl_list_iterator_func_t iterator, void *data);
+
 /**
  * Retrieves a pointer to the containing struct of a given member item.
  *
@@ -311,17 +329,6 @@ typedef int (*wl_dispatcher_func_t)(const void *, void *, uint32_t,
 
 typedef void (*wl_log_func_t)(const char *, va_list) WL_PRINTF(1, 0);
 
-/** \enum wl_iterator_result
- *
- * This enum represents the return value of an iterator function.
- */
-enum wl_iterator_result {
-	/** Stop the iteration */
-	WL_ITERATOR_STOP,
-	/** Continue the iteration */
-	WL_ITERATOR_CONTINUE
-};
-
 #ifdef  __cplusplus
 }
 #endif
diff --git a/tests/signal-test.c b/tests/signal-test.c
index 7bbaa9f..1cc5883 100644
--- a/tests/signal-test.c
+++ b/tests/signal-test.c
@@ -115,3 +115,75 @@ TEST(signal_emit_to_more_listeners)
 
 	assert(3 * counter == count);
 }
+
+static void notify_remove(struct wl_listener *l, void *data)
+{
+	struct wl_listener *l2 = data;
+	wl_list_remove(&l2->link);
+	wl_list_init(&l2->link);
+}
+
+static void notify(struct wl_listener *l, void *data)
+{}
+
+#define INIT \
+	wl_signal_init(&signal); \
+	wl_list_init(&l1.link); \
+	wl_list_init(&l2.link); \
+	wl_list_init(&l3.link); \
+
+TEST(signal_remove_listener)
+{
+	test_set_timeout(4);
+
+	struct wl_signal signal;
+	struct wl_listener l1, l2, l3;
+
+	l1.notify = notify_remove;
+	l2.notify = notify;
+	l3.notify = notify;
+
+	INIT
+	wl_signal_add(&signal, &l1);
+
+	wl_signal_emit(&signal, &l1);
+	wl_signal_emit(&signal, &l1);
+
+	INIT
+	wl_signal_add(&signal, &l1);
+	wl_signal_add(&signal, &l2);
+
+	wl_signal_emit(&signal, &l1);
+	wl_signal_emit(&signal, &l1);
+
+	INIT
+	wl_signal_add(&signal, &l1);
+	wl_signal_add(&signal, &l2);
+
+	wl_signal_emit(&signal, &l2);
+	wl_signal_emit(&signal, &l2);
+
+	INIT
+	wl_signal_add(&signal, &l1);
+	wl_signal_add(&signal, &l2);
+	wl_signal_add(&signal, &l3);
+
+	wl_signal_emit(&signal, &l1);
+	wl_signal_emit(&signal, &l1);
+
+	INIT
+	wl_signal_add(&signal, &l1);
+	wl_signal_add(&signal, &l2);
+	wl_signal_add(&signal, &l3);
+
+	wl_signal_emit(&signal, &l2);
+	wl_signal_emit(&signal, &l2);
+
+	INIT
+	wl_signal_add(&signal, &l1);
+	wl_signal_add(&signal, &l2);
+	wl_signal_add(&signal, &l3);
+
+	wl_signal_emit(&signal, &l3);
+	wl_signal_emit(&signal, &l3);
+}
-- 
2.9.3



More information about the wayland-devel mailing list