Commit 5e6eb032 authored by Giulio Camuffo's avatar Giulio Camuffo Committed by Pekka Paalanen

server: add a safer signal type and port wl_display to it

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.
Signed-off-by: default avatarGiulio Camuffo <giulio.camuffo@kdab.com>
Reviewed-by: Pekka Paalanen's avatarPekka Paalanen <pekka.paalanen@collabora.co.uk>
parent 23d8bc5a
......@@ -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
......
......@@ -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
......@@ -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;
......@@ -489,7 +489,7 @@ wl_client_create(struct wl_display *display, int fd)
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;
......@@ -942,8 +942,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;
......@@ -1008,7 +1008,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);
......@@ -1478,7 +1478,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.
......@@ -1496,14 +1496,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
......@@ -1812,6 +1812,97 @@ wl_client_for_each_resource(struct wl_client *client,
wl_map_for_each(&client->objects, resource_iterator_helper, &context);
}
/** Initialize a wl_priv_signal object
*
* wl_priv_signal is a safer implementation of a signal type, with the same API
* as wl_signal, but kept as a private utility of libwayland-server.
* It is safer because listeners can be removed from within wl_priv_signal_emit()
* without corrupting the signal's list.
*
* Before passing a wl_priv_signal object to any other function it must be
* initialized by useing wl_priv_signal_init().
*
* \memberof wl_priv_signal
*/
void
wl_priv_signal_init(struct wl_priv_signal *signal)
{
wl_list_init(&signal->listener_list);
wl_list_init(&signal->emit_list);
}
/** Add a listener to a signal
*
* The new listener will be called when calling wl_signal_emit(). If a listener is
* added to the signal while wl_signal_emit() is running it will be called from
* the next time wl_priv_signal_emit() is called.
* To remove a listener call wl_list_remove() on its link member.
*
* \memberof wl_priv_signal
*/
void
wl_priv_signal_add(struct wl_priv_signal *signal, struct wl_listener *listener)
{
wl_list_insert(signal->listener_list.prev, &listener->link);
}
/** Get a listener added to a signal
*
* Returns the listener added to the given \a signal and with the given
* \a notify function, or NULL if there isn't any.
* Calling this function from withing wl_priv_signal_emit() is safe and will
* return the correct value.
*
* \memberof wl_priv_signal
*/
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;
}
/** Emit the signal, 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.
*/
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
......
/*
* 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) == signal->current);
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)
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment