util: fix: duplicated index at nested loop in virNVMeDeviceListCreateReAttachList

[PATCH] util: fix: duplicated index at nested loop in virNVMeDeviceListCreateReAttachList

Posted by Yi Wang 2 years, 8 months ago

From: Jia Zhou <zhou.jia2@zte.com.cn>

When loop in function virNVMeDeviceListCreateReAttachList() there may be
reused index @i, this patch fix this by using a new @j.

Signed-off-by: Jia Zhou <zhou.jia2@zte.com.cn>
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
---
 src/util/virnvme.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/util/virnvme.c b/src/util/virnvme.c
index 49102e3..b54a195 100644
--- a/src/util/virnvme.c
+++ b/src/util/virnvme.c
@@ -399,7 +399,7 @@ virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList,
                                     virNVMeDeviceListPtr toReAttachList)
 {
     g_autoptr(virPCIDeviceList) pciDevices = NULL;
-    size_t i;
+    size_t i, j;
 
     if (!(pciDevices = virPCIDeviceListNew()))
         return NULL;
@@ -412,8 +412,8 @@ virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList,
         /* Check if there is any other NVMe device with the same PCI address as
          * @d. To simplify this, let's just count how many NVMe devices with
          * the same PCI address there are on the @activeList. */
-        for (i = 0; i < activeList->count; i++) {
-            virNVMeDevicePtr other = activeList->devs[i];
+        for (j = 0; j < activeList->count; j++) {
+            virNVMeDevicePtr other = activeList->devs[j];
 
             if (!virPCIDeviceAddressEqual(&d->address, &other->address))
                 continue;
-- 
1.8.3.1

Re: [PATCH] util: fix: duplicated index at nested loop in virNVMeDeviceListCreateReAttachList

Posted by Michal Prívozník 2 years, 8 months ago

On 7/29/21 4:16 AM, Yi Wang wrote:
> From: Jia Zhou <zhou.jia2@zte.com.cn>
> 
> When loop in function virNVMeDeviceListCreateReAttachList() there may be
> reused index @i, this patch fix this by using a new @j.
> 
> Signed-off-by: Jia Zhou <zhou.jia2@zte.com.cn>
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> ---
>  src/util/virnvme.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virnvme.c b/src/util/virnvme.c
> index 49102e3..b54a195 100644
> --- a/src/util/virnvme.c
> +++ b/src/util/virnvme.c
> @@ -399,7 +399,7 @@ virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList,
>                                      virNVMeDeviceListPtr toReAttachList)
>  {
>      g_autoptr(virPCIDeviceList) pciDevices = NULL;
> -    size_t i;
> +    size_t i, j;
>  
>      if (!(pciDevices = virPCIDeviceListNew()))
>          return NULL;
> @@ -412,8 +412,8 @@ virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList,
>          /* Check if there is any other NVMe device with the same PCI address as
>           * @d. To simplify this, let's just count how many NVMe devices with
>           * the same PCI address there are on the @activeList. */
> -        for (i = 0; i < activeList->count; i++) {
> -            virNVMeDevicePtr other = activeList->devs[i];
> +        for (j = 0; j < activeList->count; j++) {
> +            virNVMeDevicePtr other = activeList->devs[j];
>  
>              if (!virPCIDeviceAddressEqual(&d->address, &other->address))
>                  continue;
> 

Ooops yes. This is a bug. However, I'd prefer that 'j' would be defined
inside the loop.

Also, I'm having difficulties applying this patch. Partly because it's
multipart e-mail and even if I extract text/plain part then git am is
still unhappy. Can you please use git send-email?

Michal

Re:[PATCH] util: fix: duplicated index at nested loop in virNVMeDeviceListCreateReAttachList

Posted by wang.yi59@zte.com.cn 2 years, 8 months ago

Hi Michal,

Thanks for your reply.

> On 7/29/21 4:16 AM, Yi Wang wrote:
> > From: Jia Zhou <zhou.jia2@zte.com.cn>
> >
> > When loop in function virNVMeDeviceListCreateReAttachList() there may be
> > reused index @i, this patch fix this by using a new @j.
> >
> > Signed-off-by: Jia Zhou <zhou.jia2@zte.com.cn>
> > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > ---
> >  src/util/virnvme.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/util/virnvme.c b/src/util/virnvme.c
> > index 49102e3..b54a195 100644
> > --- a/src/util/virnvme.c
> > +++ b/src/util/virnvme.c
> > @@ -399,7 +399,7 @@ virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList,
> >                                      virNVMeDeviceListPtr toReAttachList)
> >  {
> >      g_autoptr(virPCIDeviceList) pciDevices = NULL;
> > -    size_t i;
> > +    size_t i, j;
> >
> >      if (!(pciDevices = virPCIDeviceListNew()))
> >          return NULL;
> > @@ -412,8 +412,8 @@ virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList,
> >          /* Check if there is any other NVMe device with the same PCI address as
> >           * @d. To simplify this, let's just count how many NVMe devices with
> >           * the same PCI address there are on the @activeList. */
> > -        for (i = 0; i < activeList->count; i++) {
> > -            virNVMeDevicePtr other = activeList->devs[i];
> > +        for (j = 0; j < activeList->count; j++) {
> > +            virNVMeDevicePtr other = activeList->devs[j];
> >
> >              if (!virPCIDeviceAddressEqual(&d->address, &other->address))
> >                  continue;
> >
>
> Ooops yes. This is a bug. However, I'd prefer that 'j' would be defined
> inside the loop.
>
> Also, I'm having difficulties applying this patch. Partly because it's
> multipart e-mail and even if I extract text/plain part then git am is
> still unhappy. Can you please use git send-email?

Actually I used git send-email to send this patch, and I have forwarded this
to our IT department, but unfortunately that may take a long long time :(

Would you please apply this patch manually first? Many thanks.

> Michal

Re: [PATCH] util: fix: duplicated index at nested loop in virNVMeDeviceListCreateReAttachList

Posted by Michal Prívozník 2 years, 8 months ago

On 7/29/21 1:51 PM, wang.yi59@zte.com.cn wrote:
> Hi Michal,
> 
> Thanks for your reply.
> 
>> On 7/29/21 4:16 AM, Yi Wang wrote:
>>> From: Jia Zhou <zhou.jia2@zte.com.cn>
>>>
>>> When loop in function virNVMeDeviceListCreateReAttachList() there may be
>>> reused index @i, this patch fix this by using a new @j.
>>>
>>> Signed-off-by: Jia Zhou <zhou.jia2@zte.com.cn>
>>> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
>>> ---
>>>  src/util/virnvme.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/util/virnvme.c b/src/util/virnvme.c
>>> index 49102e3..b54a195 100644
>>> --- a/src/util/virnvme.c
>>> +++ b/src/util/virnvme.c
>>> @@ -399,7 +399,7 @@ virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList,
>>>                                      virNVMeDeviceListPtr toReAttachList)
>>>  {
>>>      g_autoptr(virPCIDeviceList) pciDevices = NULL;
>>> -    size_t i;
>>> +    size_t i, j;

This new variable can be declared inside the loop since it's needed only
there.

>>>
>>>      if (!(pciDevices = virPCIDeviceListNew()))
>>>          return NULL;
>>> @@ -412,8 +412,8 @@ virNVMeDeviceListCreateReAttachList(virNVMeDeviceListPtr activeList,
>>>          /* Check if there is any other NVMe device with the same PCI address as
>>>           * @d. To simplify this, let's just count how many NVMe devices with
>>>           * the same PCI address there are on the @activeList. */
>>> -        for (i = 0; i < activeList->count; i++) {
>>> -            virNVMeDevicePtr other = activeList->devs[i];
>>> +        for (j = 0; j < activeList->count; j++) {
>>> +            virNVMeDevicePtr other = activeList->devs[j];

This doesn't look rebased on the top of anything recent. In commit
v7.3.0-rc1~229 I've dropped internal virXXXPtr and replaced them with
virXXX *.

>>>
>>>              if (!virPCIDeviceAddressEqual(&d->address, &other->address))
>>>                  continue;
>>>
>>
>> Ooops yes. This is a bug. However, I'd prefer that 'j' would be defined
>> inside the loop.
>>
>> Also, I'm having difficulties applying this patch. Partly because it's
>> multipart e-mail and even if I extract text/plain part then git am is
>> still unhappy. Can you please use git send-email?
> 
> Actually I used git send-email to send this patch, and I have forwarded this
> to our IT department, but unfortunately that may take a long long time :(

Yeah, it would be great if this could get resolved.

> 
> Would you please apply this patch manually first? Many thanks.

I've done that. Fortunately, the change was simple enough and it fixes a
bug in the code I've written.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal