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

[Andreas Ericsson]

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