[PATCH wayland] server: use a safer signal type for the wl_resource destruction signals
Giulio Camuffo
giuliocamuffo at gmail.com
Mon Dec 5 15:20:22 UTC 2016
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
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
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;
struct wl_client *client;
void *data;
int version;
wl_dispatcher_func_t dispatcher;
+ struct wl_priv_signal destroy_signal;
};
struct wl_protocol_logger {
@@ -457,7 +462,7 @@ wl_client_create(struct wl_display *display, int fd)
if (client == NULL)
return NULL;
- wl_signal_init(&client->resource_created_signal);
+ wl_priv_signal_init(&client->resource_created_signal);
client->display = display;
client->source = wl_event_loop_add_fd(display->loop, fd,
WL_EVENT_READABLE,
@@ -480,13 +485,13 @@ wl_client_create(struct wl_display *display, int fd)
if (wl_map_insert_at(&client->objects, 0, 0, NULL) < 0)
goto err_map;
- wl_signal_init(&client->destroy_signal);
+ wl_priv_signal_init(&client->destroy_signal);
if (bind_display(client, display) < 0)
goto err_map;
wl_list_insert(display->client_list.prev, &client->link);
- wl_signal_emit(&display->create_client_signal, client);
+ wl_priv_signal_emit(&display->create_client_signal, client);
return client;
@@ -604,7 +609,8 @@ destroy_resource(void *element, void *data)
struct wl_client *client = resource->client;
uint32_t flags;
- wl_signal_emit(&resource->destroy_signal, resource);
+ wl_signal_emit(&resource->deprecated_destroy_signal, resource);
+ wl_priv_signal_emit(&resource->destroy_signal, resource);
flags = wl_map_lookup_flags(&client->objects, resource->object.id);
if (resource->destroy)
@@ -716,14 +722,14 @@ WL_EXPORT void
wl_resource_add_destroy_listener(struct wl_resource *resource,
struct wl_listener * listener)
{
- wl_signal_add(&resource->destroy_signal, listener);
+ wl_priv_signal_add(&resource->destroy_signal, listener);
}
WL_EXPORT struct wl_listener *
wl_resource_get_destroy_listener(struct wl_resource *resource,
wl_notify_func_t notify)
{
- return wl_signal_get(&resource->destroy_signal, notify);
+ return wl_priv_signal_get(&resource->destroy_signal, notify);
}
/** Retrieve the interface name (class) of a resource object.
@@ -742,14 +748,14 @@ WL_EXPORT void
wl_client_add_destroy_listener(struct wl_client *client,
struct wl_listener *listener)
{
- wl_signal_add(&client->destroy_signal, listener);
+ wl_priv_signal_add(&client->destroy_signal, listener);
}
WL_EXPORT struct wl_listener *
wl_client_get_destroy_listener(struct wl_client *client,
wl_notify_func_t notify)
{
- return wl_signal_get(&client->destroy_signal, notify);
+ return wl_priv_signal_get(&client->destroy_signal, notify);
}
WL_EXPORT void
@@ -757,7 +763,7 @@ wl_client_destroy(struct wl_client *client)
{
uint32_t serial = 0;
- wl_signal_emit(&client->destroy_signal, client);
+ wl_priv_signal_emit(&client->destroy_signal, client);
wl_client_flush(client);
wl_map_for_each(&client->objects, destroy_resource, &serial);
@@ -919,8 +925,8 @@ wl_display_create(void)
wl_list_init(&display->registry_resource_list);
wl_list_init(&display->protocol_loggers);
- wl_signal_init(&display->destroy_signal);
- wl_signal_init(&display->create_client_signal);
+ wl_priv_signal_init(&display->destroy_signal);
+ wl_priv_signal_init(&display->create_client_signal);
display->id = 1;
display->serial = 0;
@@ -982,7 +988,7 @@ wl_display_destroy(struct wl_display *display)
struct wl_socket *s, *next;
struct wl_global *global, *gnext;
- wl_signal_emit(&display->destroy_signal, display);
+ wl_priv_signal_emit(&display->destroy_signal, display);
wl_list_for_each_safe(s, next, &display->socket_list, link) {
wl_socket_destroy(s);
@@ -1409,7 +1415,7 @@ WL_EXPORT void
wl_display_add_destroy_listener(struct wl_display *display,
struct wl_listener *listener)
{
- wl_signal_add(&display->destroy_signal, listener);
+ wl_priv_signal_add(&display->destroy_signal, listener);
}
/** Registers a listener for the client connection signal.
@@ -1427,14 +1433,14 @@ WL_EXPORT void
wl_display_add_client_created_listener(struct wl_display *display,
struct wl_listener *listener)
{
- wl_signal_add(&display->create_client_signal, listener);
+ wl_priv_signal_add(&display->create_client_signal, listener);
}
WL_EXPORT struct wl_listener *
wl_display_get_destroy_listener(struct wl_display *display,
wl_notify_func_t notify)
{
- return wl_signal_get(&display->destroy_signal, notify);
+ return wl_priv_signal_get(&display->destroy_signal, notify);
}
WL_EXPORT void
@@ -1490,7 +1496,8 @@ wl_resource_create(struct wl_client *client,
resource->object.interface = interface;
resource->object.implementation = NULL;
- wl_signal_init(&resource->destroy_signal);
+ wl_signal_init(&resource->deprecated_destroy_signal);
+ wl_priv_signal_init(&resource->destroy_signal);
resource->destroy = NULL;
resource->client = client;
@@ -1506,7 +1513,7 @@ wl_resource_create(struct wl_client *client,
return NULL;
}
- wl_signal_emit(&client->resource_created_signal, resource);
+ wl_priv_signal_emit(&client->resource_created_signal, resource);
return resource;
}
@@ -1694,7 +1701,7 @@ WL_EXPORT void
wl_client_add_resource_created_listener(struct wl_client *client,
struct wl_listener *listener)
{
- wl_signal_add(&client->resource_created_signal, listener);
+ wl_priv_signal_add(&client->resource_created_signal, listener);
}
struct wl_resource_iterator_context {
@@ -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);
+ }
+}
+
/** \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
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <assert.h>
+
+#include "test-runner.h"
+#include "wayland-private.h"
+
+static void
+signal_notify(struct wl_listener *listener, void *data)
+{
+ /* only increase counter*/
+ ++(*((int *) data));
+}
+
+TEST(signal_init)
+{
+ struct wl_priv_signal signal;
+
+ wl_priv_signal_init(&signal);
+
+ /* Test if listeners' list is initialized */
+ assert(&signal.listener_list == signal.listener_list.next
+ && "Maybe wl_priv_signal implementation changed?");
+ assert(signal.listener_list.next == signal.listener_list.prev
+ && "Maybe wl_priv_signal implementation changed?");
+}
+
+TEST(signal_add_get)
+{
+ struct wl_priv_signal signal;
+
+ /* we just need different values of notify */
+ struct wl_listener l1 = {.notify = (wl_notify_func_t) 0x1};
+ struct wl_listener l2 = {.notify = (wl_notify_func_t) 0x2};
+ struct wl_listener l3 = {.notify = (wl_notify_func_t) 0x3};
+ /* one real, why not */
+ struct wl_listener l4 = {.notify = signal_notify};
+
+ wl_priv_signal_init(&signal);
+
+ wl_priv_signal_add(&signal, &l1);
+ wl_priv_signal_add(&signal, &l2);
+ wl_priv_signal_add(&signal, &l3);
+ wl_priv_signal_add(&signal, &l4);
+
+ assert(wl_priv_signal_get(&signal, signal_notify) == &l4);
+ assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x3) == &l3);
+ assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x2) == &l2);
+ assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x1) == &l1);
+
+ /* get should not be destructive */
+ assert(wl_priv_signal_get(&signal, signal_notify) == &l4);
+ assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x3) == &l3);
+ assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x2) == &l2);
+ assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x1) == &l1);
+}
+
+TEST(signal_emit_to_one_listener)
+{
+ int count = 0;
+ int counter;
+
+ struct wl_priv_signal signal;
+ struct wl_listener l1 = {.notify = signal_notify};
+
+ wl_priv_signal_init(&signal);
+ wl_priv_signal_add(&signal, &l1);
+
+ for (counter = 0; counter < 100; counter++)
+ wl_priv_signal_emit(&signal, &count);
+
+ assert(counter == count);
+}
+
+TEST(signal_emit_to_more_listeners)
+{
+ int count = 0;
+ int counter;
+
+ struct wl_priv_signal signal;
+ struct wl_listener l1 = {.notify = signal_notify};
+ struct wl_listener l2 = {.notify = signal_notify};
+ struct wl_listener l3 = {.notify = signal_notify};
+
+ wl_priv_signal_init(&signal);
+ wl_priv_signal_add(&signal, &l1);
+ wl_priv_signal_add(&signal, &l2);
+ wl_priv_signal_add(&signal, &l3);
+
+ for (counter = 0; counter < 100; counter++)
+ wl_priv_signal_emit(&signal, &count);
+
+ assert(3 * counter == count);
+}
+
+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);
+
+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)
+
+ INIT
+ wl_priv_signal_add(&signal.signal, &signal.l1);
+ wl_priv_signal_add(&signal.signal, &signal.l2);
+
+ signal.current = &signal.l2;
+ CHECK_EMIT(1)
+ CHECK_EMIT(1)
+
+ INIT
+ wl_priv_signal_add(&signal.signal, &signal.l1);
+ wl_priv_signal_add(&signal.signal, &signal.l2);
+ wl_priv_signal_add(&signal.signal, &signal.l3);
+
+ signal.current = &signal.l1;
+ CHECK_EMIT(3)
+ CHECK_EMIT(2)
+
+ INIT
+ wl_priv_signal_add(&signal.signal, &signal.l1);
+ wl_priv_signal_add(&signal.signal, &signal.l2);
+ wl_priv_signal_add(&signal.signal, &signal.l3);
+
+ signal.current = &signal.l2;
+ CHECK_EMIT(2)
+ CHECK_EMIT(2)
+
+ INIT
+ wl_priv_signal_add(&signal.signal, &signal.l1);
+ wl_priv_signal_add(&signal.signal, &signal.l2);
+ wl_priv_signal_add(&signal.signal, &signal.l3);
+
+ signal.current = &signal.l3;
+ CHECK_EMIT(2)
+ CHECK_EMIT(2)
+}
+
+static void notify_readd(struct wl_listener *l, void *data)
+{
+ struct signal *signal = data;
+ if (signal->current) {
+ wl_list_remove(&signal->current->link);
+ wl_list_init(&signal->current->link);
+ wl_priv_signal_add(&signal->signal, signal->current);
+ }
+ signal->count++;
+}
+
+static void notify_empty(struct wl_listener *l, void *data)
+{
+ struct signal *signal = data;
+ signal->count++;
+}
+
+TEST(signal_readd_listener)
+{
+ /* Readding a listener is supported, that is it doesn't trigger an
+ * infinite loop or other weird things, but if in a listener you
+ * readd another listener, that will not be fired in the current
+ * signal emission. */
+
+ test_set_timeout(4);
+
+ struct signal signal;
+
+ signal.l1.notify = notify_readd;
+ signal.l2.notify = notify_readd;
+
+ INIT
+ wl_priv_signal_add(&signal.signal, &signal.l1);
+
+ signal.current = &signal.l1;
+ CHECK_EMIT(1)
+ CHECK_EMIT(1)
+
+ INIT
+ wl_priv_signal_add(&signal.signal, &signal.l1);
+
+ signal.current = &signal.l2;
+ CHECK_EMIT(1)
+ signal.current = NULL;
+ CHECK_EMIT(2)
+
+ INIT
+ wl_priv_signal_add(&signal.signal, &signal.l2);
+
+ signal.current = &signal.l1;
+ CHECK_EMIT(1)
+ /* l2 was added before l1, so l2 is fired first, which by readding l1
+ * removes it from the current list that is being fired, so 1 is correct */
+ CHECK_EMIT(1)
+
+ INIT
+ wl_priv_signal_add(&signal.signal, &signal.l1);
+ wl_priv_signal_add(&signal.signal, &signal.l2);
+
+ signal.l1.notify = notify_empty;
+ signal.current = &signal.l2;
+ CHECK_EMIT(2)
+ CHECK_EMIT(2)
+
+ INIT
+ wl_priv_signal_add(&signal.signal, &signal.l1);
+ wl_priv_signal_add(&signal.signal, &signal.l2);
+
+ signal.l1.notify = notify_empty;
+ signal.current = &signal.l1;
+ CHECK_EMIT(2)
+ /* same as before, by readding l1 in the first emit, it now is fired
+ * after l2, so on the second emit it is not fired at all. */
+ 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);
+
+ 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);
+ signal->count++;
+}
+
+TEST(signal_get_listener)
+{
+ test_set_timeout(4);
+
+ struct signal signal;
+
+ signal.l1.notify = notify_addandget;
+ signal.l2.notify = notify_get;
+
+ INIT
+ wl_priv_signal_add(&signal.signal, &signal.l1);
+
+ signal.current = &signal.l2;
+ CHECK_EMIT(1)
+
+ INIT
+ wl_priv_signal_add(&signal.signal, &signal.l2);
+
+ signal.current = &signal.l2;
+ CHECK_EMIT(1)
+
+ INIT
+ signal.l1.notify = notify_get;
+ signal.l2.notify = notify_empty;
+ wl_priv_signal_add(&signal.signal, &signal.l1);
+ wl_priv_signal_add(&signal.signal, &signal.l2);
+
+ CHECK_EMIT(2)
+
+ INIT
+ signal.l1.notify = notify_empty;
+ signal.l2.notify = notify_get;
+ wl_priv_signal_add(&signal.signal, &signal.l1);
+ wl_priv_signal_add(&signal.signal, &signal.l2);
+
+ signal.current = &signal.l1;
+ CHECK_EMIT(2)
+}
--
2.10.2
More information about the wayland-devel
mailing list