Re: [libvirt] [PATCH] event: fix two event-handling bugs
On 01/21/2011 04:15 AM, Daniel P. Berrange wrote:
> 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.
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.
I'm also updating the commit message. The problem cannot be triggered
until you hit either 31 timeouts or 31 handles (which is why I never saw
it in my earlier testing, since running 1 or 2, or even 10, VMs does not
create that many handles), because our allocation pattern was always
requesting growth by 10 (VIR_RESIZE_N uses MAX(requested, 50%) for it's
growth delta), for 10, 20, 30, 45. And since freeing always happened by
10, you didn't get integer overflow until you had hit the allocation of
45 or higher (which requires growing past 30). Very nasty bug indeed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)