[libvirt] [PATCH] usb: Correct test for virUSBDeviceListSteal

[Laine Stump]

On 07/05/2013 11:37 AM, Laine Stump wrote:
> On 07/05/2013 02:23 AM, Gonglei (Arei) wrote:
>> In the for loop, the if condition is always true, and will execute memmove.
>> But it will cause the list->devs[i+1] overflow while i equals list->count-1.


BTW, does that actually cause a failure? Although it's true that the 2nd
element of memmove will be 1 element past the end of the allocated
memory, the count of items to move in that case will be 0, so there
should never actually be any attempt to dereference the pointer. (Are
you maybe seeing a failure with valgrind or something?)


>>
>> Signed-off-by: Gonglei <arei.gonglei at huawei.com>
>> ---
>>  src/util/virusb.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/util/virusb.c b/src/util/virusb.c
>> index d34e44f..30d0b12 100644
>> --- a/src/util/virusb.c
>> +++ b/src/util/virusb.c
>> @@ -497,7 +497,7 @@ virUSBDeviceListSteal(virUSBDeviceListPtr list,
>>  
>>          ret = list->devs[i];
>>  
>> -        if (i != list->count--)
>> +        if (i != --list->count)
>>              memmove(&list->devs[i],
>>                      &list->devs[i+1],
>>                      sizeof(*list->devs) * (list->count - i));
> This function is a good candidate for switching to VIR_DELETE_ELEMENT()
> instead. This will eliminate the bug that you found while making the
> code much shorter. I have a patch for that sitting around, I'll rebase
> it and post it.
>
> (I actually have 15 patches that replace memmove+VIR_REALLOC() with
> VIR_DELETE_ELEMENT and VIR_INSERT_ELEMENT. I had written them as
> examples of VIR_*_ELEMENT and posted them, but was too nervous of
> potential regression to push them (or even nag about reviews).
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>