[PATCH] event-loop: Allow source dispatches to remove other sources
Andreas Ericsson
ae at op5.se
Tue Mar 20 06:59:26 PDT 2012
On 03/20/2012 02:28 PM, Jonas Ådahl wrote:
> 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?
>
No. That is done in wl_event_source_remove(), which is the only
entry-point for dispatch functions to remove sources from the
eventloop, so if we're marked as destroyed we're already gone
from that 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
--
Andreas Ericsson andreas.ericsson at op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.
More information about the wayland-devel
mailing list