[libvirt] [PATCH] event: fix two event-handling bugs

[Daniel Veillard]

On Thu, Jan 20, 2011 at 05:01:13PM -0700, Eric Blake wrote:
> Regression introduced in commit e6b68d7.
> 
> Prior to that point, handlesAlloc was always a multiple of
> EVENT_ALLOC_EXTENT, and was an integer (so even if the
> subtraction wrapped, a negative value was less than the
> count and did not try to free the handles array).  But after
> that point, VIR_RESIZE_N made handlesAlloc grow geometrically,
> so handlesAlloc could be 49 when handlesCount was 47, while
> still freeing off only 10 at a time, and eventually reach
> handlesAlloc 7 and handlesCount 1.  Since (size_t)(7 - 10) is
> indeed greater than 1, this then tried to free 10 elements,
> which had the awful effect of nuking the handles array while
> there were still live handles.
> 
> Which leads to crashes like this:
> https://bugzilla.redhat.com/show_bug.cgi?id=670848
> 
> * daemon/event.c (virEventDispatchHandles): Cache watch while the
> lock is still held, as eventLoop.handles might be relocated
> outside the lock.
> (virEventCleanupHandles): Avoid integer wrap-around causing us to
> delete the entire handles array while handles are still active.
> ---
> 
> Thanks to Dave Allan for letting me use his faqemu setup for testing
> this.  Basically, starting 60 faqemu followed by stopping them all
> was a reliable way to trigger the handle cleanup integer wraparound.
> 
>  daemon/event.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/daemon/event.c b/daemon/event.c
> index 89ca9f0..9cad466 100644
> --- a/daemon/event.c
> +++ b/daemon/event.c
> @@ -1,7 +1,7 @@
>  /*
>   * event.c: event loop for monitoring file handles
>   *
> - * Copyright (C) 2007, 2010 Red Hat, Inc.
> + * Copyright (C) 2007, 2010-2011 Red Hat, Inc.
>   * Copyright (C) 2007 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -435,6 +435,7 @@ static int virEventDispatchTimeouts(void) {
>   */
>  static int virEventDispatchHandles(int nfds, struct pollfd *fds) {
>      int i, n;
> +    int watch;
>      DEBUG("Dispatch %d", nfds);
> 
>      /* NB, use nfds not eventLoop.handlesCount, because new
> @@ -463,9 +464,9 @@ static int virEventDispatchHandles(int nfds, struct pollfd *fds) {
>              EVENT_DEBUG("Dispatch n=%d f=%d w=%d e=%d %p", i,
>                          fds[n].fd, eventLoop.handles[i].watch,
>                          fds[n].revents, eventLoop.handles[i].opaque);
> +            watch = eventLoop.handles[i].watch;
>              virMutexUnlock(&eventLoop.lock);
> -            (cb)(eventLoop.handles[i].watch,
> -                 fds[n].fd, hEvents, opaque);
> +            (cb)(watch, fds[n].fd, hEvents, opaque);
>              virMutexLock(&eventLoop.lock);
>          }
>      }
> @@ -542,9 +543,10 @@ static int virEventCleanupHandles(void) {
>      }
> 
>      /* Release some memory if we've got a big chunk free */
> -    if ((eventLoop.handlesAlloc - EVENT_ALLOC_EXTENT) > eventLoop.handlesCount) {
> +    if (eventLoop.handlesAlloc - eventLoop.handlesCount > EVENT_ALLOC_EXTENT) {
>          EVENT_DEBUG("Releasing %zu out of %zu handles slots used, releasing %d",
> -                   eventLoop.handlesCount, eventLoop.handlesAlloc, EVENT_ALLOC_EXTENT);
> +                    eventLoop.handlesCount, eventLoop.handlesAlloc,
> +                    EVENT_ALLOC_EXTENT);
>          VIR_SHRINK_N(eventLoop.handles, eventLoop.handlesAlloc,
>                       EVENT_ALLOC_EXTENT);
>      }

  ACK, fairly nasty !

   thanks for chasing this down !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/