On Thu, 22 Feb 2018 16:02:49 -0600
Derek Foreman <der...@osg.samsung.com> 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 <der...@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);

Just a quick fly-by comment; is there a reason to actually have a head
in the personal list for 'pos'? Wouldn't wl_list_init(pos) be enough?

Or would you want keep e.g. checks like
wl_list_empty(&foofoo_destroy_listener->link) working? Not sure how that
would be useful... you got called, so obviously the listener is in a
list.

The idea seems fine to me, but I didn't look very hard.


Thanks,
pq

> +
> +             l->notify(l, data);
> +     }
> +}
> +
>  /** \endcond INTERNAL */
>  
>  /** \cond */ /* Deprecated functions below. */

Attachment: pgp9Me12FsQ3R.pgp
Description: OpenPGP digital signature

_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to