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

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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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