[libvirt] [PATCH] usb: Correct test for virUSBDeviceListSteal
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. Signed-off-by: Gonglei <arei.gonglei@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)); -- 1.7.3.1 Best Regards, -Gonglei
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.
Signed-off-by: Gonglei <arei.gonglei@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).
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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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.
Signed-off-by: Gonglei <arei.gonglei@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.
Posted here: https://www.redhat.com/archives/libvir-list/2013-July/msg00336.html (I forgot to mention it in that patch, but it assumes Michal's "eliminate virReportOOMError()" patches have all been pushed.)
participants (2)
-
Gonglei (Arei) -
Laine Stump