Re: [libvirt] [PATCH] usb: Correct test for virUSBDeviceListSteal
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list