[PATCH] event-loop: Allow source dispatches to remove other sources

[Ander Conselvan de Oliveira]

Hi,

See comments below.

On 03/20/2012 12:31 PM, Andreas Ericsson wrote:
> When a dispatch for sourceA wishes to remove sourceB and sourceB
> has input that isn't yet processed, we would run into the dreaded
> "undefined behaviour" previously.
>
> With this patch, the destroyed source is ignored while processing
> input and is later free()'d in the post_dispatch_check() loop.
>
> Signed-off-by: Andreas Ericsson<ae at op5.se>
> ---
>
> I actually haven't managed to build wayland yet, and since I'm stuck
> with an nvidia videocard it's not likely to happen anytime in the
> near future either, so this patch is, unfortunately, totally untested.
>
>   src/event-loop.c |   42 ++++++++++++++++++++++++++++++++++++++----
>   1 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/src/event-loop.c b/src/event-loop.c
> index 2dfe0ae..02d5163 100644
> --- a/src/event-loop.c
> +++ b/src/event-loop.c
> @@ -37,6 +37,7 @@
>
>   struct wl_event_loop {
>   	int epoll_fd;
> +	int *source_flags;
>   	struct wl_list check_list;
>   	struct wl_list idle_list;
>   };

This field is not used anywhere. I guess you meant to add 'int 
dispatching;' instead that is used below.

> @@ -48,6 +49,7 @@ struct wl_event_source_interface {
>   };
>
>   struct wl_event_source {
> +	int destroyed;
>   	struct wl_event_source_interface *interface;
>   	struct wl_event_loop *loop;
>   	struct wl_list link;
> @@ -85,7 +87,21 @@ wl_event_source_fd_remove(struct wl_event_source *source)
>   	int fd;
>
>   	fd = fd_source->fd;
> -	free(source);
> +
> +	/*
> +	 * a running dispatch can remove another source, which
> +	 * will cause crashes if that other source is also queued
> +	 * for dispatch, since we'll grab the source pointer from
> +	 * the epoll event. If that happens, we have to mark the
> +	 * source as destroyed here instead of free()'ing so we
> +	 * know not to touch it later.
> +	 */
> +	if (loop->dispatching) {
> +		source->destroyed = 1;
> +		wl_event_source_check(source);
> +	} else {
> +		free(source);
> +	}
>
>   	return epoll_ctl(loop->epoll_fd, EPOLL_CTL_DEL, fd, NULL);
>   }

This logic should be in wl_event_source_remove() so that it also covers 
signal source, timer sources, etc. I'm not sure how you would do 
EPOLL_CTL_DEL from there though. Maybe there could be a disable hook, so 
you call disable early but remove only on post dispatch check.

Also, wl_event_source_check() does not check if source is already in the 
check list. If the same item is inserted twice you end up with a 
corrupted list.

> @@ -104,7 +120,10 @@ wl_event_loop_add_fd(struct wl_event_loop *loop,
>   	struct wl_event_source_fd *source;
>   	struct epoll_event ep;
>
> -	source = malloc(sizeof *source);
> +	if (fd<  0)
> +		return NULL;
> +
> +	source = calloc(1, sizeof *source);
>   	if (source == NULL)
>   		return NULL;

All of wl_event_loop_add_{signal,timer,idle} need to initialize 
source->destroyed.


Cheers,
Ander