[PATCH wayland] server: use a safer signal type for the wl_resource destruction signals
Pekka Paalanen
ppaalanen at gmail.com
Thu Dec 15 08:15:58 UTC 2016
On Wed, 14 Dec 2016 14:41:21 +0200
Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 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;
> > };
...
> > /** \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;
> > }
...
>
> 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.
Hi,
after sleeping the night, realized something I missed, so I need to at
least temporarily withdraw my R-b.
If someone was still using the deprecated public form of struct
wl_resource, then the code in libwayland-server cannot touch *at all*
the new fields added in the private definition. If it did, it would
read and write arbitrary application data causing corruption.
I have not yet reviewed what this affects, or if we already broke it,
but I suspect we need to write more tests to ensure we do not currupt
data like that. I'll see if I could have some time for that, but I am
also leaving for winter holidays after Wednesday next week, and will be
away until Jan 12.
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/20161215/2ac7e45a/attachment.sig>
More information about the wayland-devel
mailing list