[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2] event: fix two event-handling bugs



On Fri, Jan 21, 2011 at 01:06:58PM -0700, Eric Blake wrote:
> The first bug has been present since before the time that commit
> f8a519 (Dec 2008) tried to make the dispatch loop re-entrant.
> 
> Dereferencing eventLoop.handles outside the lock risks crashing, since
> any other thread could have reallocated the array in the meantime.
> It's a narrow race window, however, and one that would have most
> likely resulted in passing bogus data to the callback rather than
> actually causing a segv, which is probably why it has gone undetected
> this long.
> 
> The second is a regression introduced in commit e6b68d7 (Nov 2010).
> 
> Prior to that point, handlesAlloc was always a multiple of
> EVENT_ALLOC_EXTENT (10), and was an int (so even if the subtraction
> had been able to wrap, a negative value would be less than the count
> not try to free the handles array).  But after that point,
> VIR_RESIZE_N made handlesAlloc grow geometrically (with a pattern of
> 10, 20, 30, 45 for the handles array) but still freed in multiples of
> EVENT_ALLOC_EXTENT; and the count changed to size_t.  Which means that
> after 31 handles have been created, then 30 handles destroyed,
> handlesAlloc is 5 while handlesCount is 1, and since (size_t)(1 - 5)
> 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.
> 
> Nuking live handles puts libvirtd in an inconsistent state, and was
> easily reproducible by starting and then stopping 60 faqemu guests.
> It may also be the root cause of crashes like this:
> https://bugzilla.redhat.com/show_bug.cgi?id=670848
> 
> * daemon/event.c (virEventDispatchHandles): Cache data while
> inside the lock, as the array might be reallocated once outside.
> (virEventCleanupTimeouts, virEventCleanupHandles): Avoid integer
> wrap-around causing us to delete the entire array while entries
> are still active.
> * tests/eventtest.c (mymain): Expose the bug.
> ---
> 
> v2: fix timeoutsAlloc, which had same bug as handlesAlloc. Avoid
> memory leak when all handles are deregistered, and free in larger
> chunks to match allocating in larger chunks.  Break some long lines.
> Add testsuite exposure.
> 
> > > It is probably quite dificult, but is there any way this
> > > codepath could be validated under the eventtest program,
> > > even if it would run running eventtest under valgrind to
> > > actually detect the flaw.
> > I'm looking into that.  As is, timeoutsAlloc also has the same bug, so
> > v2 of the patch is coming up, after I (re-)audit the entire file for all
> > uses of eventLoop to ensure that reads and modifications are only done
> > inside locks, and that modifications don't fall foul of integer wraparound.
> 
> Re-audit complete, the timeoutsAlloc bug was the only other one
> that I found.
> 
> And modifying the testsuite was doable (I won't say easy, since it
> took me several hours) - merely bump the limit up to 31 to trigger
> the allocation to be a non-multiple of the de-allocation, then make
> sure enough virEventRunOnce gets called to free things back to the
> point of nuking the live array (freeing only 10 at a time meant
> that I had to add a for loop; it was figuring that out that took
> me the longest).  I've verified that the changes to eventtest.c
> in isolation reliably cause a core dump.
> 
>  daemon/event.c    |   42 +++++++++++++++++++++++-----------------
>  tests/eventtest.c |   54 +++++++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 64 insertions(+), 32 deletions(-)
> 
> diff --git a/daemon/event.c b/daemon/event.c
> index 89ca9f0..4c97fb9 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
> @@ -458,14 +458,13 @@ static int virEventDispatchHandles(int nfds, struct pollfd *fds) {
> 
>          if (fds[n].revents) {
>              virEventHandleCallback cb = eventLoop.handles[i].cb;
> +            int watch = eventLoop.handles[i].watch;
>              void *opaque = eventLoop.handles[i].opaque;
>              int hEvents = virPollEventToEventHandleType(fds[n].revents);
>              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);
> +                        fds[n].fd, watch, fds[n].revents, opaque);
>              virMutexUnlock(&eventLoop.lock);
> -            (cb)(eventLoop.handles[i].watch,
> -                 fds[n].fd, hEvents, opaque);
> +            (cb)(watch, fds[n].fd, hEvents, opaque);
>              virMutexLock(&eventLoop.lock);
>          }
>      }
> @@ -480,6 +479,7 @@ static int virEventDispatchHandles(int nfds, struct pollfd *fds) {
>   */
>  static int virEventCleanupTimeouts(void) {
>      int i;
> +    size_t gap;
>      DEBUG("Cleanup %zu", eventLoop.timeoutsCount);
> 
>      /* Remove deleted entries, shuffling down remaining
> @@ -491,24 +491,27 @@ static int virEventCleanupTimeouts(void) {
>              continue;
>          }
> 
> -        EVENT_DEBUG("Purging timeout %d with id %d", i, eventLoop.timeouts[i].timer);
> +        EVENT_DEBUG("Purging timeout %d with id %d", i,
> +                    eventLoop.timeouts[i].timer);
>          if (eventLoop.timeouts[i].ff)
>              (eventLoop.timeouts[i].ff)(eventLoop.timeouts[i].opaque);
> 
>          if ((i+1) < eventLoop.timeoutsCount) {
>              memmove(eventLoop.timeouts+i,
>                      eventLoop.timeouts+i+1,
> -                    sizeof(struct virEventTimeout)*(eventLoop.timeoutsCount-(i+1)));
> +                    sizeof(struct virEventTimeout)*(eventLoop.timeoutsCount
> +                                                    -(i+1)));
>          }
>          eventLoop.timeoutsCount--;
>      }
> 
>      /* Release some memory if we've got a big chunk free */
> -    if ((eventLoop.timeoutsAlloc - EVENT_ALLOC_EXTENT) > eventLoop.timeoutsCount) {
> -        EVENT_DEBUG("Releasing %zu out of %zu timeout slots used, releasing %d",
> -                   eventLoop.timeoutsCount, eventLoop.timeoutsAlloc, EVENT_ALLOC_EXTENT);
> -        VIR_SHRINK_N(eventLoop.timeouts, eventLoop.timeoutsAlloc,
> -                     EVENT_ALLOC_EXTENT);
> +    gap = eventLoop.timeoutsAlloc - eventLoop.timeoutsCount;
> +    if (eventLoop.timeoutsCount == 0 ||
> +        (gap > eventLoop.timeoutsCount && gap > EVENT_ALLOC_EXTENT)) {
> +        EVENT_DEBUG("Found %zu out of %zu timeout slots used, releasing %zu",
> +                    eventLoop.timeoutsCount, eventLoop.timeoutsAlloc, gap);
> +        VIR_SHRINK_N(eventLoop.timeouts, eventLoop.timeoutsAlloc, gap);
>      }
>      return 0;
>  }
> @@ -519,6 +522,7 @@ static int virEventCleanupTimeouts(void) {
>   */
>  static int virEventCleanupHandles(void) {
>      int i;
> +    size_t gap;
>      DEBUG("Cleanup %zu", eventLoop.handlesCount);
> 
>      /* Remove deleted entries, shuffling down remaining
> @@ -536,17 +540,19 @@ static int virEventCleanupHandles(void) {
>          if ((i+1) < eventLoop.handlesCount) {
>              memmove(eventLoop.handles+i,
>                      eventLoop.handles+i+1,
> -                    sizeof(struct virEventHandle)*(eventLoop.handlesCount-(i+1)));
> +                    sizeof(struct virEventHandle)*(eventLoop.handlesCount
> +                                                   -(i+1)));
>          }
>          eventLoop.handlesCount--;
>      }
> 
>      /* Release some memory if we've got a big chunk free */
> -    if ((eventLoop.handlesAlloc - EVENT_ALLOC_EXTENT) > eventLoop.handlesCount) {
> -        EVENT_DEBUG("Releasing %zu out of %zu handles slots used, releasing %d",
> -                   eventLoop.handlesCount, eventLoop.handlesAlloc, EVENT_ALLOC_EXTENT);
> -        VIR_SHRINK_N(eventLoop.handles, eventLoop.handlesAlloc,
> -                     EVENT_ALLOC_EXTENT);
> +    gap = eventLoop.handlesAlloc - eventLoop.handlesCount;
> +    if (eventLoop.handlesCount == 0 ||
> +        (gap > eventLoop.handlesCount && gap > EVENT_ALLOC_EXTENT)) {
> +        EVENT_DEBUG("Found %zu out of %zu handles slots used, releasing %zu",
> +                    eventLoop.handlesCount, eventLoop.handlesAlloc, gap);
> +        VIR_SHRINK_N(eventLoop.handles, eventLoop.handlesAlloc, gap);
>      }
>      return 0;
>  }
> diff --git a/tests/eventtest.c b/tests/eventtest.c
> index 067e365..93317be 100644
> --- a/tests/eventtest.c
> +++ b/tests/eventtest.c
> @@ -1,7 +1,7 @@
>  /*
>   * eventtest.c: Test the libvirtd event loop impl
>   *
> - * Copyright (C) 2009 Red Hat, Inc.
> + * Copyright (C) 2009, 2011 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -33,8 +33,8 @@
>  #include "util.h"
>  #include "../daemon/event.h"
> 
> -#define NUM_FDS 5
> -#define NUM_TIME 5
> +#define NUM_FDS 31
> +#define NUM_TIME 31
> 
>  static struct handleInfo {
>      int pipeFD[2];
> @@ -147,11 +147,14 @@ verifyFired(const char *name, int handle, int timer)
>      for (i = 0 ; i < NUM_FDS ; i++) {
>          if (handles[i].fired) {
>              if (i != handle) {
> -                virtTestResult(name, 1, "Handle %d fired, but expected %d\n", i, handle);
> +                virtTestResult(name, 1,
> +                               "Handle %d fired, but expected %d\n", i,
> +                               handle);
>                  return EXIT_FAILURE;
>              } else {
>                  if (handles[i].error != EV_ERROR_NONE) {
> -                    virtTestResult(name, 1, "Handle %d fired, but had error %d\n", i,
> +                    virtTestResult(name, 1,
> +                                   "Handle %d fired, but had error %d\n", i,
>                                     handles[i].error);
>                      return EXIT_FAILURE;
>                  }
> @@ -159,13 +162,17 @@ verifyFired(const char *name, int handle, int timer)
>              }
>          } else {
>              if (i == handle) {
> -                virtTestResult(name, 1, "Handle %d should have fired, but didn't\n", handle);
> +                virtTestResult(name, 1,
> +                               "Handle %d should have fired, but didn't\n",
> +                               handle);
>                  return EXIT_FAILURE;
>              }
>          }
>      }
>      if (handleFired != 1 && handle != -1) {
> -        virtTestResult(name, 1, "Something wierd happened, expecting handle %d\n", handle);
> +        virtTestResult(name, 1,
> +                       "Something weird happened, expecting handle %d\n",
> +                       handle);
>          return EXIT_FAILURE;
>      }
> 
> @@ -173,11 +180,13 @@ verifyFired(const char *name, int handle, int timer)
>      for (i = 0 ; i < NUM_TIME ; i++) {
>          if (timers[i].fired) {
>              if (i != timer) {
> -                virtTestResult(name, 1, "Timer %d fired, but expected %d\n", i, timer);
> +                virtTestResult(name, 1,
> +                               "Timer %d fired, but expected %d\n", i, timer);
>                  return EXIT_FAILURE;
>              } else {
>                  if (timers[i].error != EV_ERROR_NONE) {
> -                    virtTestResult(name, 1, "Timer %d fired, but had error %d\n", i,
> +                    virtTestResult(name, 1,
> +                                   "Timer %d fired, but had error %d\n", i,
>                                     timers[i].error);
>                      return EXIT_FAILURE;
>                  }
> @@ -185,13 +194,17 @@ verifyFired(const char *name, int handle, int timer)
>              }
>          } else {
>              if (i == timer) {
> -                virtTestResult(name, 1, "Timer %d should have fired, but didn't\n", timer);
> +                virtTestResult(name, 1,
> +                               "Timer %d should have fired, but didn't\n",
> +                               timer);
>                  return EXIT_FAILURE;
>              }
>          }
>      }
>      if (timerFired != 1 && timer != -1) {
> -        virtTestResult(name, 1, "Something wierd happened, expecting timer %d\n", timer);
> +        virtTestResult(name, 1,
> +                       "Something weird happened, expecting timer %d\n",
> +                       timer);
>          return EXIT_FAILURE;
>      }
>      return EXIT_SUCCESS;
> @@ -217,7 +230,8 @@ finishJob(const char *name, int handle, int timer)
>      waitTime.tv_sec += 5;
>      rc = 0;
>      while (!eventThreadJobDone && rc == 0)
> -        rc = pthread_cond_timedwait(&eventThreadJobCond, &eventThreadMutex, &waitTime);
> +        rc = pthread_cond_timedwait(&eventThreadJobCond, &eventThreadMutex,
> +                                    &waitTime);
>      if (rc != 0) {
>          virtTestResult(name, 1, "Timed out waiting for pipe event\n");
>          return EXIT_FAILURE;
> @@ -426,13 +440,25 @@ mymain(int argc, char **argv)
>      if (finishJob("Deleted during dispatch", -1, 2) != EXIT_SUCCESS)
>          return EXIT_FAILURE;
> 
> -    for (i = 0 ; i < NUM_FDS ; i++)
> +    for (i = 0 ; i < NUM_FDS - 1 ; i++)
>          virEventRemoveHandleImpl(handles[i].watch);
> -    for (i = 0 ; i < NUM_TIME ; i++)
> +    for (i = 0 ; i < NUM_TIME - 1 ; i++)
>          virEventRemoveTimeoutImpl(timers[i].timer);
> 
>      resetAll();
> 
> +    /* Make sure the last handle still works several times in a row.  */
> +    for (i = 0; i < 4; i++) {
> +        startJob();
> +        if (safewrite(handles[NUM_FDS - 1].pipeFD[1], &one, 1) != 1)
> +            return EXIT_FAILURE;
> +        if (finishJob("Simple write", NUM_FDS - 1, -1) != EXIT_SUCCESS)
> +            return EXIT_FAILURE;
> +
> +        resetAll();
> +    }
> +
> +
>      /* Final test, register same FD twice, once with no
>       * events, and make sure the right callback runs */
>      handles[0].pipeFD[0] = handles[1].pipeFD[0];

ACK

Daniel


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]