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

[Andreas Ericsson]

On 03/20/2012 01:19 PM, Ander Conselvan de Oliveira wrote:
> 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.
> 

Right. Mea culpa for starting to head down the "use an indexed
array" path and not reverting fully. Thanks for catching it.

>> @@ -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.
> 

We can't mark the source as destroyed while we retain it in the epoll
set (we don't need to check that deletion worked, since the only errors
we can get from deletion is that the filedescriptor requested for
deletion isn't in the set, which is also a sort of success, from our
point of view), or we might end up polling it for input later again.

I chose to use a macro instead and add the very simple code to all the
interface removers. That should cover all the cases.

> 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.
> 

Right. I guess that could happen if multiple dispatchers want to
remove the same event from the event-loop in the same dispatcher
run. I'll cover it in the next version of the patch.

>> @@ -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.
> 

Right. I'll amend the patch to use calloc() everywhere then, unless
explicit initialization is actually preferred for some reason. I
didn't find any coding-style document, so I just went with what I
usually use when allocating struct pointers.

-- 
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.