[PATCH v2 2/3] server: Add special case destroy signal emitter

[Derek Foreman]

In the past much code (weston, efl/enlightenment, mutter) has
freed structures containing wl_listeners from destroy handlers
without first removing the listener from the signal.  As the
destroy notifier only fires once, this has largely gone
unnoticed until recently.

Other code does not (Qt, wlroots) - and removes itself from
the signal before free.

If somehow a destroy signal is listened to by code from both
kinds of callers, those that free will corrupt the lists for
those that don't, and Bad Things will happen.

To avoid these bad things, remove every item from the signal list
during destroy emit, and put it in a list all its own.  This way
whether the listener is removed or not has no impact on the
following emits.

Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
---

Changes since v1:
 In v1 I went through some ugly steps to ensure wl_priv_signal_get()
 worked.  It seems this is actually a useless requirement, and the
 code is prettier without it.

 src/wayland-private.h |  3 +++
 src/wayland-server.c  | 46 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/src/wayland-private.h b/src/wayland-private.h
index 12b9032..29516ec 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -253,6 +253,9 @@ wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify);
 void
 wl_priv_signal_emit(struct wl_priv_signal *signal, void *data);
 
+void
+wl_priv_signal_final_emit(struct wl_priv_signal *signal, void *data);
+
 void
 wl_connection_close_fds_in(struct wl_connection *connection, int max);
 
diff --git a/src/wayland-server.c b/src/wayland-server.c
index eb1e500..8c3b800 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -682,7 +682,7 @@ destroy_resource(void *element, void *data, uint32_t flags)
 	/* Don't emit the new signal for deprecated resources, as that would
 	 * access memory outside the bounds of the deprecated struct */
 	if (!resource_is_deprecated(resource))
-		wl_priv_signal_emit(&resource->destroy_signal, resource);
+		wl_priv_signal_final_emit(&resource->destroy_signal, resource);
 
 	if (resource->destroy)
 		resource->destroy(resource);
@@ -841,7 +841,7 @@ wl_client_destroy(struct wl_client *client)
 {
 	uint32_t serial = 0;
 
-	wl_priv_signal_emit(&client->destroy_signal, client);
+	wl_priv_signal_final_emit(&client->destroy_signal, client);
 
 	wl_client_flush(client);
 	wl_map_for_each(&client->objects, destroy_resource, &serial);
@@ -1089,7 +1089,7 @@ wl_display_destroy(struct wl_display *display)
 	struct wl_socket *s, *next;
 	struct wl_global *global, *gnext;
 
-	wl_priv_signal_emit(&display->destroy_signal, display);
+	wl_priv_signal_final_emit(&display->destroy_signal, display);
 
 	wl_list_for_each_safe(s, next, &display->socket_list, link) {
 		wl_socket_destroy(s);
@@ -2025,6 +2025,46 @@ wl_priv_signal_emit(struct wl_priv_signal *signal, void *data)
 	}
 }
 
+/** Emit the signal for the last time, calling all the installed listeners
+ *
+ * Iterate over all the listeners added to this \a signal and call
+ * their \a notify function pointer, passing on the given \a data.
+ * Removing or adding a listener from within wl_priv_signal_emit()
+ * is safe, as is freeing the structure containing the listener.
+ *
+ * A large body of external code assumes it's ok to free a destruction
+ * listener without removing that listener from the list.  Mixing code
+ * that acts like this and code that doesn't will result in list
+ * corruption.
+ *
+ * We resolve this by removing each item from the list and isolating it
+ * in another list.  We discard it completely after firing the notifier.
+ * This should allow interoperability between code that unlinks its
+ * destruction listeners and code that just frees structures they're in.
+ *
+ */
+void
+wl_priv_signal_final_emit(struct wl_priv_signal *signal, void *data)
+{
+	struct wl_listener *l;
+	struct wl_list *pos;
+
+	/* During a destructor notifier isolate every list item before
+	 * notifying.  This renders harmless the long standing misuse
+	 * of freeing listeners without removing them, but allows
+	 * callers that do choose to remove them to interoperate with
+	 * ones that don't. */
+	while (!wl_list_empty(&signal->listener_list)) {
+		pos = signal->listener_list.next;
+		l = wl_container_of(pos, l, link);
+
+		wl_list_remove(pos);
+		wl_list_init(pos);
+
+		l->notify(l, data);
+	}
+}
+
 /** \endcond INTERNAL */
 
 /** \cond */ /* Deprecated functions below. */
-- 
2.14.3