[PATCH wayland] server: use a safer signal type for the wl_resource destruction signals
Pekka Paalanen
ppaalanen at gmail.com
Wed Dec 14 12:41:21 UTC 2016
On Mon, 5 Dec 2016 16:20:22 +0100
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> wl_list_for_each_safe, which is 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, which was hit
> in weston on resource destruction, which emits a signal.
> This commit adds a new version of wl_signal, called wl_priv_signal,
> which is private in wayland-server.c and which does not have this problem.
> The old wl_signal cannot be improved without breaking backwards compatibility.
> ---
> Makefile.am | 4 +
> src/wayland-private.h | 18 +++
> src/wayland-server.c | 110 ++++++++++++----
> tests/newsignal-test.c | 337 +++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 445 insertions(+), 24 deletions(-)
> create mode 100644 tests/newsignal-test.c
Hi,
this patch fixes the following issue I have been having:
Start weston/x11 or weston/wayland, run kcachegrind (Qt4) via Xwayland,
hover pointer over any kcachegrind button that shows a tooltip, move
pointer away, repeat a couple of times. Sooner rather than later Weston
gets into a busyloop and freezes.
That no longer happens.
Tested-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
The new tests not only include the old signal tests, you also added
tests for the exact case we were tripping over: removing the next
callback from current callback while iterating the list of callbacks.
It is missing the case of a callback removing the callback added and
called just before the current one.
You also add tests for removing and adding a listener from a callback
to the same signal. Interesting behavioral guarantees. You test for
get() returning non-NULL in various cases too, though it would be much
better if it could test for the correct pointer rather than just
non-NULL.
Anyway, I find the tests adequate. You could also add yourself to the
copyright holders, this is significant new code IMO.
You missed one place that is still using the old wl_signal:
event-loop.c, struct wl_event_loop, destroy_signal. Luckily that seems
to be a completely private definition, and the implementation can be
switched in a follow-up patch.
> diff --git a/Makefile.am b/Makefile.am
> index d78a0ca..d0c8bd3 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -159,6 +159,7 @@ built_test_programs = \
> socket-test \
> queue-test \
> signal-test \
> + newsignal-test \
> resources-test \
> message-test \
> headers-test \
> @@ -226,6 +227,9 @@ queue_test_SOURCES = tests/queue-test.c
> queue_test_LDADD = libtest-runner.la
> signal_test_SOURCES = tests/signal-test.c
> signal_test_LDADD = libtest-runner.la
> +# wayland-server.c is needed here to access wl_priv_* functions
> +newsignal_test_SOURCES = tests/newsignal-test.c src/wayland-server.c
> +newsignal_test_LDADD = libtest-runner.la
The other alternative would be to put wl_priv_*() into wayland-util.c.
That one gets built into a static helper lib, so the test programs
would get the definitions.
> resources_test_SOURCES = tests/resources-test.c
> resources_test_LDADD = libtest-runner.la
> message_test_SOURCES = tests/message-test.c
> diff --git a/src/wayland-private.h b/src/wayland-private.h
> index 676b181..434cb04 100644
> --- a/src/wayland-private.h
> +++ b/src/wayland-private.h
> @@ -35,6 +35,7 @@
> #define WL_HIDE_DEPRECATED 1
>
> #include "wayland-util.h"
> +#include "wayland-server-core.h"
>
> /* Invalid memory address */
> #define WL_ARRAY_POISON_PTR (void *) 4
> @@ -233,4 +234,21 @@ zalloc(size_t s)
> return calloc(1, s);
> }
>
> +struct wl_priv_signal {
> + struct wl_list listener_list;
> + struct wl_list emit_list;
> +};
> +
> +void
> +wl_priv_signal_init(struct wl_priv_signal *signal);
> +
> +void
> +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_priv_signal_emit(struct wl_priv_signal *signal, void *data);
> +
> #endif
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 9d7d9c1..dae3c1d 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -78,10 +78,10 @@ struct wl_client {
> uint32_t mask;
> struct wl_list link;
> struct wl_map objects;
> - struct wl_signal destroy_signal;
> + struct wl_priv_signal destroy_signal;
> struct ucred ucred;
> int error;
> - struct wl_signal resource_created_signal;
> + struct wl_priv_signal resource_created_signal;
> };
>
> struct wl_display {
> @@ -97,8 +97,8 @@ struct wl_display {
> struct wl_list client_list;
> struct wl_list protocol_loggers;
>
> - struct wl_signal destroy_signal;
> - struct wl_signal create_client_signal;
> + struct wl_priv_signal destroy_signal;
> + struct wl_priv_signal create_client_signal;
>
> struct wl_array additional_shm_formats;
> };
> @@ -117,11 +117,16 @@ struct wl_resource {
> struct wl_object object;
> wl_resource_destroy_func_t destroy;
> struct wl_list link;
> - struct wl_signal destroy_signal;
> + /* Unfortunately some users of libwayland (e.g. mesa) still use the
> + * deprecated wl_resource struct, even if creating it with the new
> + * wl_resource_create(). So we cannot change the layout of the struct
> + * unless after the data field. */
> + struct wl_signal deprecated_destroy_signal;
Right. I see that you are keeping the deprecated signal still fully
working. That's excellent.
> struct wl_client *client;
> void *data;
> int version;
> wl_dispatcher_func_t dispatcher;
> + struct wl_priv_signal destroy_signal;
> };
...
> @@ -1743,6 +1750,60 @@ wl_client_for_each_resource(struct wl_client *client,
> wl_map_for_each(&client->objects, resource_iterator_helper, &context);
> }
>
> +void
> +wl_priv_signal_init(struct wl_priv_signal *signal)
> +{
> + wl_list_init(&signal->listener_list);
> + wl_list_init(&signal->emit_list);
> +}
> +
> +void
> +wl_priv_signal_add(struct wl_priv_signal *signal, struct wl_listener *listener)
> +{
> + wl_list_insert(signal->listener_list.prev, &listener->link);
> +}
> +
> +struct wl_listener *
> +wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify)
> +{
> + struct wl_listener *l;
> +
> + wl_list_for_each(l, &signal->listener_list, link)
> + if (l->notify == notify)
> + return l;
> + wl_list_for_each(l, &signal->emit_list, link)
> + if (l->notify == notify)
> + return l;
> +
> + return NULL;
> +}
> +
> +void
> +wl_priv_signal_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);
> + wl_list_init(&signal->listener_list);
> +
> + /* 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(&signal->emit_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);
> + }
> +}
> +
Yup, the implementation looks correct.
The only thing lacking is documentation.
I suspect this also makes emitting the same signal from its own
callback also work ok, but I'd rather not document that as something
that works or has defined behaviour. I would not want to encourage such
code.
> /** \cond */ /* Deprecated functions below. */
>
> uint32_t
> @@ -1767,7 +1828,8 @@ wl_client_add_resource(struct wl_client *client,
> }
>
> resource->client = client;
> - wl_signal_init(&resource->destroy_signal);
> + wl_signal_init(&resource->deprecated_destroy_signal);
> + wl_priv_signal_init(&resource->destroy_signal);
>
> return resource->object.id;
> }
> diff --git a/tests/newsignal-test.c b/tests/newsignal-test.c
> new file mode 100644
> index 0000000..5275537
> --- /dev/null
> +++ b/tests/newsignal-test.c
> @@ -0,0 +1,337 @@
> +/*
> + * Copyright © 2013 Marek Chalupa
> + *
...
> +struct signal
> +{
> + struct wl_priv_signal signal;
> + struct wl_listener l1, l2, l3;
> + int count;
> + struct wl_listener *current;
> +};
> +
> +static void notify_remove(struct wl_listener *l, void *data)
> +{
> + struct signal *sig = data;
> + wl_list_remove(&sig->current->link);
> + wl_list_init(&sig->current->link);
> + sig->count++;
> +}
> +
> +#define INIT \
> + wl_priv_signal_init(&signal.signal); \
> + wl_list_init(&signal.l1.link); \
> + wl_list_init(&signal.l2.link); \
> + wl_list_init(&signal.l3.link);
> +#define CHECK_EMIT(expected) \
> + signal.count = 0; \
> + wl_priv_signal_emit(&signal.signal, &signal); \
> + assert(signal.count == expected);
Is there any reason to have these as macros that even implicitly use
local variables?
> +
> +TEST(signal_remove_listener)
> +{
> + test_set_timeout(4);
> +
> + struct signal signal;
> +
> + signal.l1.notify = notify_remove;
> + signal.l2.notify = notify_remove;
> + signal.l3.notify = notify_remove;
> +
> + INIT
> + wl_priv_signal_add(&signal.signal, &signal.l1);
> +
> + signal.current = &signal.l1;
> + CHECK_EMIT(1)
> + CHECK_EMIT(0)
> +
> + INIT
> + wl_priv_signal_add(&signal.signal, &signal.l1);
> + wl_priv_signal_add(&signal.signal, &signal.l2);
> +
> + CHECK_EMIT(2)
> + CHECK_EMIT(1)
...
> +static void notify_addandget(struct wl_listener *l, void *data)
> +{
> + struct signal *signal = data;
> + wl_list_remove(&signal->current->link);
> + wl_list_init(&signal->current->link);
> + wl_priv_signal_add(&signal->signal, signal->current);
> +
> + assert(wl_priv_signal_get(&signal->signal, signal->current->notify) != NULL);
Too bad it seems to be a bit awkward to ensure we get the correct
pointer from get().
> +
> + signal->count++;
> +}
> +
> +static void notify_get(struct wl_listener *l, void *data)
> +{
> + struct signal *signal = data;
> + assert(wl_priv_signal_get(&signal->signal, signal->current->notify) != NULL);
The same as above.
> + signal->count++;
> +}
All my comments are minor and I would be happy to merge this patch as
is. If no-one comments in a day or two, I will land this.
When I do, I will add your S-o-b which you agreed on IRC, and my test
case into the commit message.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161214/377a9f38/attachment.sig>
More information about the wayland-devel
mailing list