[PATCH v2] server: add wl_signal_emit_safe

[Simon Ser]

This new function allows listeners to remove themselves or any
other listener when called. This version only works if listeners
are properly removed before they are free'd.

wl_signal_emit tries to be safe but it fails: it works fine if a
handler removes its own listener, but not if it removes another
one.

It's not possible to patch wl_signal_emit directly as attempted
in [1] because some projects using libwayland directly free
destroy listeners without removing them from the list. Using this
new strategy fails in this case, causing read-after-free errors.

[1]: https://patchwork.freedesktop.org/patch/204641/

Signed-off-by: Simon Ser <***@emersion.fr>
---

Addressed Markus' comments [1].

[1]: https://lists.freedesktop.org/archives/wayland-devel/2018-July/039042.html

src/wayland-server-core.h | 3 ++
src/wayland-server.c | 50 +++++++++++++++++++++++
tests/signal-test.c | 86 +++++++++++++++++++++++++++++++++++++++
3 files changed, 139 insertions(+)

diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
index 2e725d9..4a2948a 100644
--- a/src/wayland-server-core.h
+++ b/src/wayland-server-core.h
@@ -468,6 +468,9 @@ wl_signal_emit(struct wl_signal *signal, void *data)
l->notify(l, data);
}

+void
+wl_signal_emit_safe(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 eae8d2e..3d851f4 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -1932,6 +1932,56 @@ wl_client_for_each_resource(struct wl_client *client,
wl_map_for_each(&client->objects, resource_iterator_helper, &context);
}

+static void
+handle_noop(struct wl_listener *listener, void *data) {
+ /* Do nothing */
+}
+
+/** Emits this signal, safe against removal of any listener.
+ *
+ * wl_signal_emit tries to be safe but it fails: it works fine if a handler
+ * removes its own listener, but not if it removes another one.
+ *
+ * \note This function can only be used if listeners are properly removed before
+ * being free'd.
+ *
+ * \param signal The signal object that will emit the signal
+ * \param data The data that will be emitted with the signal
+ *
+ * \sa wl_signal_emit()
+ *
+ * \memberof wl_signal
+ */
+WL_EXPORT void
+wl_signal_emit_safe(struct wl_signal *signal, void *data) {
+ struct wl_listener cursor;
+ struct wl_listener end;
+
+ /* Add two special markers: one cursor and one end marker. This way, we know
+ * that we've already called listeners on the left of the cursor and that we
+ * don't want to call listeners on the right of the end marker. The 'it'
+ * function can remove any element it wants from the list without troubles.
+ * 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. */
+ wl_list_insert(&signal->listener_list, &cursor.link);
+ cursor.notify = handle_noop;
+ wl_list_insert(signal->listener_list.prev, &end.link);
+ end.notify = handle_noop;
+
+ while (cursor.link.next != &end.link) {
+ struct wl_list *pos = cursor.link.next;
+ struct wl_listener *l = wl_container_of(pos, l, link);
+
+ wl_list_remove(&cursor.link);
+ wl_list_insert(pos, &cursor.link);
+
+ l->notify(l, data);
+ }
+
+ wl_list_remove(&cursor.link);
+ wl_list_remove(&end.link);
+}
+
/** \cond INTERNAL */

/** Initialize a wl_priv_signal object
diff --git a/tests/signal-test.c b/tests/signal-test.c
index 7bbaa9f..dc762a4 100644
--- a/tests/signal-test.c
+++ b/tests/signal-test.c
@@ -115,3 +115,89 @@ TEST(signal_emit_to_more_listeners)

assert(3 * counter == count);
}
+
+struct signal
+{
+ struct wl_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_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_signal_emit_safe(&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_signal_add(&signal.signal, &signal.l1);
+
+ signal.current = &signal.l1;
+ CHECK_EMIT(1)
+ CHECK_EMIT(0)
+
+ INIT
+ wl_signal_add(&signal.signal, &signal.l1);
+ wl_signal_add(&signal.signal, &signal.l2);
+
+ CHECK_EMIT(2)
+ CHECK_EMIT(1)
+
+ INIT
+ wl_signal_add(&signal.signal, &signal.l1);
+ wl_signal_add(&signal.signal, &signal.l2);
+
+ signal.current = &signal.l2;
+ CHECK_EMIT(1)
+ CHECK_EMIT(1)
+
+ INIT
+ wl_signal_add(&signal.signal, &signal.l1);
+ wl_signal_add(&signal.signal, &signal.l2);
+ wl_signal_add(&signal.signal, &signal.l3);
+
+ signal.current = &signal.l1;
+ CHECK_EMIT(3)
+ CHECK_EMIT(2)
+
+ INIT
+ wl_signal_add(&signal.signal, &signal.l1);
+ wl_signal_add(&signal.signal, &signal.l2);
+ wl_signal_add(&signal.signal, &signal.l3);
+
+ signal.current = &signal.l2;
+ CHECK_EMIT(2)
+ CHECK_EMIT(2)
+
+ INIT
+ wl_signal_add(&signal.signal, &signal.l1);
+ wl_signal_add(&signal.signal, &signal.l2);
+ wl_signal_add(&signal.signal, &signal.l3);
+
+ signal.current = &signal.l3;
+ CHECK_EMIT(2)
+ CHECK_EMIT(2)
+}

[Derek Foreman]

Post by Simon Ser
This new function allows listeners to remove themselves or any
other listener when called. This version only works if listeners
are properly removed before they are free'd.
wl_signal_emit tries to be safe but it fails: it works fine if a
handler removes its own listener, but not if it removes another
one.
It's not possible to patch wl_signal_emit directly as attempted
in [1] because some projects using libwayland directly free
destroy listeners without removing them from the list. Using this
new strategy fails in this case, causing read-after-free errors.
[1]: https://patchwork.freedesktop.org/patch/204641/

Hi Simon,

Is there intent to use this internally in libwayland (or in weston?)
somewhere in a further patch?

Since there's no way to know from the callback whether the signal is
being emit "safely" or not, I'm a little concerned that developers might
get confused about what style of callback they're writing. Or people
will see "safe" and just lazily use that everywhere, even for destroy
signals...

Also, it looks at a glance like all of the struct members and such
touched by this function are in public headers? I think the safe emit
function doesn't strictly need to be in libwayland at all?

I don't really mean to block this work, just wondering what follows next.

Some tiny comments inlined further down.

Post by Simon Ser
---
Addressed Markus' comments [1].
[1]: https://lists.freedesktop.org/archives/wayland-devel/2018-July/039042.html
src/wayland-server-core.h | 3 ++
src/wayland-server.c | 50 +++++++++++++++++++++++
tests/signal-test.c | 86 +++++++++++++++++++++++++++++++++++++++
3 files changed, 139 insertions(+)
diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
index 2e725d9..4a2948a 100644
--- a/src/wayland-server-core.h
+++ b/src/wayland-server-core.h
@@ -468,6 +468,9 @@ wl_signal_emit(struct wl_signal *signal, void *data)
l->notify(l, data);
}
+void
+wl_signal_emit_safe(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 eae8d2e..3d851f4 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -1932,6 +1932,56 @@ wl_client_for_each_resource(struct wl_client *client,
wl_map_for_each(&client->objects, resource_iterator_helper, &context);
}
+static void
+handle_noop(struct wl_listener *listener, void *data) {

I think wayland coding style is to put the { on a new line by itself.

Post by Simon Ser
+ /* Do nothing */
+}
+
+/** Emits this signal, safe against removal of any listener.
+ *
+ * wl_signal_emit tries to be safe but it fails: it works fine if a handler
+ * removes its own listener, but not if it removes another one.
+ *
+ * \note This function can only be used if listeners are properly removed before
+ * being free'd.
+ *
+ * \param signal The signal object that will emit the signal
+ * \param data The data that will be emitted with the signal
+ *
+ * \sa wl_signal_emit()
+ *
+ * \memberof wl_signal
+ */
+WL_EXPORT void
+wl_signal_emit_safe(struct wl_signal *signal, void *data) {

Same as above.

No further complaints. :)

Thanks,
Derek

Post by Simon Ser
+ struct wl_listener cursor;
+ struct wl_listener end;
+
+ /* Add two special markers: one cursor and one end marker. This way, we know
+ * that we've already called listeners on the left of the cursor and that we
+ * don't want to call listeners on the right of the end marker. The 'it'
+ * function can remove any element it wants from the list without troubles.
+ * 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. */
+ wl_list_insert(&signal->listener_list, &cursor.link);
+ cursor.notify = handle_noop;
+ wl_list_insert(signal->listener_list.prev, &end.link);
+ end.notify = handle_noop;
+
+ while (cursor.link.next != &end.link) {
+ struct wl_list *pos = cursor.link.next;
+ struct wl_listener *l = wl_container_of(pos, l, link);
+
+ wl_list_remove(&cursor.link);
+ wl_list_insert(pos, &cursor.link);
+
+ l->notify(l, data);
+ }
+
+ wl_list_remove(&cursor.link);
+ wl_list_remove(&end.link);
+}
+
/** \cond INTERNAL */
/** Initialize a wl_priv_signal object
diff --git a/tests/signal-test.c b/tests/signal-test.c
index 7bbaa9f..dc762a4 100644
--- a/tests/signal-test.c
+++ b/tests/signal-test.c
@@ -115,3 +115,89 @@ TEST(signal_emit_to_more_listeners)
assert(3 * counter == count);
}
+
+struct signal
+{
+ struct wl_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_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_signal_emit_safe(&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_signal_add(&signal.signal, &signal.l1);
+
+ signal.current = &signal.l1;
+ CHECK_EMIT(1)
+ CHECK_EMIT(0)
+
+ INIT
+ wl_signal_add(&signal.signal, &signal.l1);
+ wl_signal_add(&signal.signal, &signal.l2);
+
+ CHECK_EMIT(2)
+ CHECK_EMIT(1)
+
+ INIT
+ wl_signal_add(&signal.signal, &signal.l1);
+ wl_signal_add(&signal.signal, &signal.l2);
+
+ signal.current = &signal.l2;
+ CHECK_EMIT(1)
+ CHECK_EMIT(1)
+
+ INIT
+ wl_signal_add(&signal.signal, &signal.l1);
+ wl_signal_add(&signal.signal, &signal.l2);
+ wl_signal_add(&signal.signal, &signal.l3);
+
+ signal.current = &signal.l1;
+ CHECK_EMIT(3)
+ CHECK_EMIT(2)
+
+ INIT
+ wl_signal_add(&signal.signal, &signal.l1);
+ wl_signal_add(&signal.signal, &signal.l2);
+ wl_signal_add(&signal.signal, &signal.l3);
+
+ signal.current = &signal.l2;
+ CHECK_EMIT(2)
+ CHECK_EMIT(2)
+
+ INIT
+ wl_signal_add(&signal.signal, &signal.l1);
+ wl_signal_add(&signal.signal, &signal.l2);
+ wl_signal_add(&signal.signal, &signal.l3);
+
+ signal.current = &signal.l3;
+ CHECK_EMIT(2)
+ CHECK_EMIT(2)
+}

[Simon Ser]

Post by Derek Foreman

Post by Simon Ser
This new function allows listeners to remove themselves or any
other listener when called. This version only works if listeners
are properly removed before they are free'd.
wl_signal_emit tries to be safe but it fails: it works fine if a
handler removes its own listener, but not if it removes another
one.
It's not possible to patch wl_signal_emit directly as attempted
in 1 because some projects using libwayland directly free
destroy listeners without removing them from the list. Using this
new strategy fails in this case, causing read-after-free errors.

Hi Simon,
Is there intent to use this internally in libwayland (or in weston?)
somewhere in a further patch?
Since there's no way to know from the callback whether the signal is
being emit "safely" or not, I'm a little concerned that developers might
get confused about what style of callback they're writing. Or people
will see "safe" and just lazily use that everywhere, even for destroy
signals...
Also, it looks at a glance like all of the struct members and such
touched by this function are in public headers? I think the safe emit
function doesn't strictly need to be in libwayland at all?
I don't really mean to block this work, just wondering what follows next.
Some tiny comments inlined further down.

Hi,

Thanks for you review.

I have no plans to make libwayland or weston use this function in a future
patch. It should be used by everybody who's sure users correctly remove
destroy listeners (or doesn't care to break its ABI).

Yeah, this function doesn't need to be in libwayland per se, in fact we already
have it in wlroots and use it everywhere. I just thought users would benefit
from having a safe version of wl_signal_emit in libwayland, because it's easy
to remove multiple listeners from a destroy handler once you have multiple
destroy signals and inter-dependant structs. It doesn't _need_ to be in
libwayland, but putting it in libwayland would allow people to make sure they
don't run into issues without needing to copy-paste the function from wlroots.

People could use it without understanding why it's safe, but I prefer to have
users running into issues because they forget to remove the destroy listener
than having users running into issues because they use multiple wl_signals.

What do you think? In the end it's not that a big deal, I just think it would
be nice to have.

Simon