[v2] server: add wl_signal_emit_safe

Submitted by emersion on Aug. 8, 2018, noon

Details

Message ID xuJYDmTZ5poEY5lm9Z8s2QNivGd9oax6E7_3Pr03F6rgDKPCn2RmbjH-Ch2kelNgiw9VoH-92eggednQcxHB9CrEjIQ3xlpBoN8H-4N5X-8=@emersion.fr
State New
Series "server: add wl_signal_emit_safe"
Headers show

Commit Message

emersion Aug. 8, 2018, noon
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 <contact@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(+)

Patch hide | download patch | download mbox

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)
+}

Comments

Derek Foreman Sept. 18, 2018, 6:43 p.m.
On 2018-08-08 07:00 AM, Simon Ser wrote:
> 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 <contact@emersion.fr>

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.

> ---
> 
> 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.

> +	/* 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

> +	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)
> +}
>
emersion Sept. 23, 2018, 7:17 p.m.
On Tuesday, September 18, 2018 8:43 PM, Derek Foreman <derek.foreman.samsung@gmail.com> wrote:
> On 2018-08-08 07:00 AM, Simon Ser wrote:
>
> > 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.
> > Signed-off-by: Simon Ser contact@emersion.fr
>
> 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