[RFC wayland] server: Add special case destroy signal emitter
Markus Ongyerth
wl at ongy.net
Fri Feb 23 09:44:13 UTC 2018
Hi, I have a v2 RFC _emit_final based on your idea.
It passes `make check` of libwayland and weston.
It also passes the remove-without-free test I sent in another mail.
---
(This patch isn't quite intended to be merged, but to get feedback on
the approach)
This adds a wl_priv_signal_emit_final and a wl_signal_emit_final.
The `_final` emit functions have the additional asumption that every
listener currently in the list will be removed after the _emit. Either
gracefully, or simply free()d under the asumption that a destroy signal
will not be emitted more than once.
These functions take advantage of this and allow to use both tyles in
the same signal list.
It obsoletes the wl_priv_signal for destroy signals.
Non destroy wl_priv_signals currently have a stronger guarantee than
normal signals, which forces us to either keep the type around, or use
this as destroy signal path, and another safe version for other signals.
---
src/wayland-private.h | 5 +++++
src/wayland-server.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/src/wayland-private.h b/src/wayland-private.h
index 12b9032..76adb32 100644
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -250,6 +250,11 @@ wl_priv_signal_add(struct wl_priv_signal *signal, struct wl_listener *listener);
struct wl_listener *
wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify);
+void
+wl_signal_emit_final(struct wl_list *signal, void *data);
+
+void
+wl_priv_signal_emit_final(struct wl_priv_signal *signal, void *data);
void
wl_priv_signal_emit(struct wl_priv_signal *signal, void *data);
diff --git a/src/wayland-server.c b/src/wayland-server.c
index eb1e500..62fc71a 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_emit_final(&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_emit_final(&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_emit_final(&display->destroy_signal, display);
wl_list_for_each_safe(s, next, &display->socket_list, link) {
wl_socket_destroy(s);
@@ -1992,6 +1992,50 @@ wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify)
return NULL;
}
+/**
+ * This emits a signal under the asumpton that every listener in it will be
+ * removed from the list in some way.
+ *
+ * It supports listeners that remove themselves without issues.
+ *
+ * The situation where a listener is only free()d, but not removed from the
+ * list is also supported, to not break existing library users.
+ *
+ * The notify function can remove arbitrary elements from the wl_signal, and
+ * the wl_signal will be in a valid state at all times (s.t. later elements in
+ * the list not beeing free()d without removal, but that broke at any time).
+ *
+ * The idea is based on wl_priv_signal_emit, we iterate until a list is empty.
+ * In this case we don't need the emit_list, since we know our actual signal
+ * list will be empty in the end.
+ */
+void
+wl_signal_emit_final(struct wl_list *signal, void *data)
+{
+ struct wl_listener *l;
+ struct wl_list *pos;
+
+ while (!wl_list_empty(signal)) {
+ pos = signal->next;
+
+ /* Remove and ensure validity of position element */
+ wl_list_remove(pos);
+ wl_list_init(pos);
+
+ l = wl_container_of(pos, l, link);
+ l->notify(l, data);
+ }
+}
+
+/**
+ * see wl_signal_emit_final for more details
+ */
+void
+wl_priv_signal_emit_final(struct wl_priv_signal *signal, void *data)
+{
+ wl_signal_emit_final(&signal->listener_list, data);
+}
+
/** Emit the signal, calling all the installed listeners
*
* Iterate over all the listeners added to this \a signal and call
--
2.16.2
On 2018/2月/22 04:02, Derek Foreman wrote:
> 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>
> ---
>
> src/wayland-private.h | 3 +++
> src/wayland-server.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 49 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..df5c16a 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,49 @@ 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;
> +
> + wl_list_insert_list(&signal->emit_list, &signal->listener_list);
> +
> + /* 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->emit_list)) {
> + wl_list_init(&signal->listener_list);
> + pos = signal->emit_list.next;
> + l = wl_container_of(pos, l, link);
> +
> + wl_list_remove(pos);
> + wl_list_insert(&signal->listener_list, pos);
> +
> + l->notify(l, data);
> + }
> +}
> +
> /** \endcond INTERNAL */
>
> /** \cond */ /* Deprecated functions below. */
> --
> 2.14.3
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180223/dc0131aa/attachment.sig>
More information about the wayland-devel
mailing list