[PATCH v2] server: add wl_signal_emit_safe

[Derek Foreman]

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