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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list