[PATCH] event-loop: Allow source dispatches to remove other sources
Jonas Ådahl
jadahl at gmail.com
Tue Mar 20 06:28:10 PDT 2012
On Tue, Mar 20, 2012 at 2:22 PM, Andreas Ericsson <ae at op5.se> 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 marked as such by its
> removal function and ignored while processing input. We free() it
> only when we run the post_dispatch_check() loop.
>
> We also handle the case where multiple dispatchers want to remove
> the same source several times by refusing to add it to the post
> dispatch check list more than once.
>
> Signed-off-by: Andreas Ericsson <ae at op5.se>
> ---
> src/event-loop.c | 60 +++++++++++++++++++++++++++++++++++++++++++++---------
> 1 files changed, 50 insertions(+), 10 deletions(-)
>
> diff --git a/src/event-loop.c b/src/event-loop.c
> index 2dfe0ae..3d19715 100644
> --- a/src/event-loop.c
> +++ b/src/event-loop.c
> @@ -37,6 +37,7 @@
>
> struct wl_event_loop {
> int epoll_fd;
> + int dispatching;
> struct wl_list check_list;
> struct wl_list idle_list;
> };
> @@ -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;
> @@ -76,6 +78,17 @@ wl_event_source_fd_dispatch(struct wl_event_source *source,
> return fd_source->func(fd_source->fd, mask, fd_source->base.data);
> }
>
> +
> +#define wl_event_source_safe_destroy(source) \
> + if (source->loop->dispatching) { \
> + /* mustn't add same source twice to post-dispatch list */ \
> + if (!source->destroyed) \
> + wl_event_source_check(source); \
> + source->destroyed = 1; \
> + } else { \
> + free(source); \
> + }
> +
> static int
> wl_event_source_fd_remove(struct wl_event_source *source)
> {
> @@ -85,7 +98,16 @@ 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.
> + */
> + wl_event_source_safe_destroy(source);
>
> return epoll_ctl(loop->epoll_fd, EPOLL_CTL_DEL, fd, NULL);
> }
> @@ -104,7 +126,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;
>
> @@ -179,7 +204,7 @@ wl_event_source_timer_remove(struct wl_event_source *source)
> (struct wl_event_source_timer *) source;
>
> close(timer_source->fd);
> - free(source);
> + wl_event_source_safe_destroy(source);
> return 0;
> }
>
> @@ -196,7 +221,7 @@ wl_event_loop_add_timer(struct wl_event_loop *loop,
> struct wl_event_source_timer *source;
> struct epoll_event ep;
>
> - source = malloc(sizeof *source);
> + source = calloc(1, sizeof *source);
> if (source == NULL)
> return NULL;
>
> @@ -278,7 +303,7 @@ wl_event_source_signal_remove(struct wl_event_source *source)
> (struct wl_event_source_signal *) source;
>
> close(signal_source->fd);
> - free(source);
> + wl_event_source_safe_destroy(source);
> return 0;
> }
>
> @@ -297,7 +322,7 @@ wl_event_loop_add_signal(struct wl_event_loop *loop,
> struct epoll_event ep;
> sigset_t mask;
>
> - source = malloc(sizeof *source);
> + source = calloc(1, sizeof *source);
> if (source == NULL)
> return NULL;
>
> @@ -340,7 +365,7 @@ struct wl_event_source_idle {
> static int
> wl_event_source_idle_remove(struct wl_event_source *source)
> {
> - free(source);
> + wl_event_source_safe_destroy(source);
>
> return 0;
> }
> @@ -357,7 +382,7 @@ wl_event_loop_add_idle(struct wl_event_loop *loop,
> {
> struct wl_event_source_idle *source;
>
> - source = malloc(sizeof *source);
> + source = calloc(1, sizeof *source);
> if (source == NULL)
> return NULL;
>
> @@ -394,7 +419,7 @@ wl_event_loop_create(void)
> {
> struct wl_event_loop *loop;
>
> - loop = malloc(sizeof *loop);
> + loop = calloc(1, sizeof *loop);
> if (loop == NULL)
> return NULL;
>
> @@ -425,8 +450,13 @@ post_dispatch_check(struct wl_event_loop *loop)
>
> ep.events = 0;
> n = 0;
> - wl_list_for_each_safe(source, next, &loop->check_list, link)
> + wl_list_for_each_safe(source, next, &loop->check_list, link) {
> + if (source->destroyed) {
> + free(source);
> + continue;
> + }
Should you also not remove source from loop->check_list?
> n += source->interface->dispatch(source, &ep);
> + }
>
> return n;
> }
> @@ -457,10 +487,20 @@ wl_event_loop_dispatch(struct wl_event_loop *loop, int timeout)
> if (count < 0)
> return -1;
> n = 0;
> + loop->dispatching = 1;
> for (i = 0; i < count; i++) {
> source = ep[i].data.ptr;
> + if (source->destroyed) {
> + /*
> + * An earlier dispatch removed this source, so it's
> + * no longer there. We ignore its input and let the
> + * post_dispatch_check() free() it.
> + */
> + continue;
> + }
> n += source->interface->dispatch(source, &ep[i]);
> }
> + loop->dispatching = 0;
>
> while (n > 0)
> n = post_dispatch_check(loop);
> --
> 1.7.4.4
>
I took the liberty of creating another patch based on the idea from
your first patch but adding an extra interface function so that the
logic for removing / queuing in the generic removal function.
Depending on what method best fits the solution, I'd be happy to
verify that the crash issue I've seen is resolved; however, I wouldn't
be able to do so until later tonight or tomorrow.
Jonas
More information about the wayland-devel
mailing list