Re: [Qemu-devel] [PATCH v2 3/3] qapi: Fix memleak in string visitors on int lists

[Eric Blake]

On 06/01/2016 01:47 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Commit 7f8f9ef1 introduced the ability to store a list of
>> integers as a sorted list of ranges, but when merging ranges,
>> it leaks one or more ranges.  It was also using range_get_last()
>> incorrectly within range_compare() (a range is a start/end pair,
>> but range_get_last() is for start/len pairs), and will also
>> mishandle a range ending in UINT64_MAX (remember, we document
>> that no range covers 2**64 bytes, but that ranges that end on
>> UINT64_MAX have end < begin).
>>

>>
>> -    if (!list) {
>> -        list = g_list_insert_sorted(list, data, range_compare);
>> -        return list;
>> +    /* Range lists require no empty ranges */
>> +    assert(data->begin < data->end || (data->begin && !data->end));
> 
> Consider { begin = 0, end = 0 }.
> 
> Since zero @end means 2^64, this encodes the (non-empty) range
> 0..2^64-1.

Or else it means an uninitialized range.  My argument is that no range
can contain 2^64 bytes, and therefore the only possible range that would
be that large (0..2^64-1) is unrepresentable, therefore, if end == 0,
begin must be non-zero for the range to be valid as an initialized range.

> 
> range.h's comment
> 
>  * Notes:
>  *   - ranges must not wrap around 0, but can include the last byte ~0x0LL.
>  *   - this can not represent a full 0 to ~0x0LL range.
> 
> appears to be wrong.  The actual limitation is "can't represent ranges
> wrapping around zero, and can't represent the empty range starting at
> zero."  Would you like to correct it?

I'm not sure what corrections it needs, though.

> 
> I'm afraid range.h is too clever by half.

Unfortunately true.

> 
>> +
>> +    for (l = list; l && range_compare(l->data, data) < 0; l = l->next) {
>> +        /* Skip all list elements strictly less than data */
>>      }
> 
> Let's put the comment before the loop.  It describes the whole loop.
> Also makes the emptiness of the body more obvious.

Sure.

> 
> I think I could fix up things on commit (assuming we agree on what needs
> fixing).
> 

Adding other authors of range.h for their opinions...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature