[PATCH wayland] server: allow removing any wl_listener from a signal when emitted
Pekka Paalanen
ppaalanen at gmail.com
Tue Sep 6 10:11:35 UTC 2016
On Tue, 6 Sep 2016 10:06:16 +0300
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> 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(-)
Hi,
just to summarize the long IRC discussions we had today:
- The fundamental problem is that wl_list_for_each_safe() is safe ONLY
against removing the current item. Particularly if you remove the
item next to current, the iteration code will get confused. This
manifests in the wl_signal, where one handler may destroy something
else that then needs to unsubscribe from the same signal.
- The approach in this patch is not quite safe against adding new
listeners from a handler function, since it might lead to an endless
loop in a pathological remove+add case.
- A modification to fix that by using a temporary list (and actually
this patch too) would break wl_signal_get() arbitrarily.
- A completely safe list iteration does have its uses: Weston's pointer
focus needs to re-set when either the wl_surface or the view gets
destroyed, but wl_surface destruction may trigger immediate view
destruction which then needs to unsubscribe from the wl_surface since
it cannot know the wl_surface is already destroying.
- Fixing wl_signal does not seem possible, given all the ABI stability
requirements, particularly being able to build pieces (libs, plugins)
of a compositor against different libwayland-server versions. This is
because all the wl_signal stuff is inline in the header.
- wl_signal emissions are fragile wrt. to the listener registering
order. This patch is trying to address a Weston issue that should
have existed for a long time, but only became a problem recently. The
code practically always uses one arbitrary ordering, which means no
other ordering is ever tested. Even a simple patch can make the
subtle change in the ordering, breaking surprising things. I'm not
sure whether purposefully relying on specific order is for better or
worse, but at least it could be documented if intentional.
- Since this is mostly about wl_resource destroy listeners, we could
just fix it for wl_resource to have completely safe listener
operations even during destruction (maybe?) without breaking or
adding any API/ABI. It would still leave wl_signal as is.
- We could add a different, opaque implementation similar to wl_signal.
Move wl_resource to use it, and let users of libwayland-server
migrate to it (or not) as they wish. OTOH, why should
libwayland-server offer these bits? It's not a generic misc toolkit
lib...
Did I forget something?
Looking at libwayland-server, we have these things:
src/wayland-server.c=451=wl_client_create(struct wl_display *display, int fd)
src/wayland-server.c:489: wl_signal_emit(&display->create_client_signal, client);
src/wayland-server.c=601=destroy_resource(void *element, void *data)
src/wayland-server.c:607: wl_signal_emit(&resource->destroy_signal, resource);
src/wayland-server.c=756=wl_client_destroy(struct wl_client *client)
src/wayland-server.c:760: wl_signal_emit(&client->destroy_signal, client);
src/wayland-server.c=980=wl_display_destroy(struct wl_display *display)
src/wayland-server.c:985: wl_signal_emit(&display->destroy_signal, display);
src/wayland-server.c=1476=wl_resource_create(struct wl_client *client,
src/wayland-server.c:1509: wl_signal_emit(&client->resource_created_signal, resource);
These are all the wl_signal_emit() calls. All the API to add listeners
for these, and the few cases that also offer a query function, are
opaque functions (not inline).
A proposal
I believe we could have a private and safe variant of wl_signal used
internally for all these. We would still use struct wl_listener as is,
and be completely ABI compatible.
wl_signal would be left as is. If someone needs a wl_signal
implementation that is completely safe against the list manipulation
during iterating, they should use their own. Libwayland API does not
pass wl_signals around. It's not libwayland-server's job to offer these
misc generic tools any more than it needs in its own API. We could even
deprecate wl_signal if we like, but it's not worth the hassle to
actually remove it.
This means that we would probably copy the safe implementation to
Weston, but I think that would be for the better. Libweston at least
can break its ABI still.
One culprit here is that the deprecated definition of wl_resource
contains a wl_signal member. I'm not sure what to do with that.
Anyway, there is an exposed bug right now in Weston, isn't there? We
probably need to find a temporary fix for it until we can fix
libwayland-server. It's too late in the release cycle to fix
libwayland-server right now, and releasing a broken Weston wouldn't be
nice.
Thanks,
pq
PS. Excellent to include tests! Maybe the final solution can be based
on this patch.
> 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);
> +}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160906/40dbb70f/attachment.sig>
More information about the wayland-devel
mailing list