[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