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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list