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. 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. > > 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. Excellant usage of fake qemu > > 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 Daniel -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list