[v1] vhost-user-scsi: Fix memleaks in vus_proc_req()

[PATCH] vhost-user-scsi: Fix memleaks in vus_proc_req()

Posted by Alex Chen 5 years, 2 months ago

The 'elem' is allocated memory in vu_queue_pop(), and it's memory should be
freed in all error branchs after vu_queue_pop().
In addition, in order to free 'elem' memory outside of while(1) loop, move the
definition of 'elem' to the begin of vus_proc_req().

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Alex Chen <alex.chen@huawei.com>
---
 contrib/vhost-user-scsi/vhost-user-scsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c b/contrib/vhost-user-scsi/vhost-user-scsi.c
index 0f9ba4b2a2..4639440a70 100644
--- a/contrib/vhost-user-scsi/vhost-user-scsi.c
+++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
@@ -232,6 +232,7 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
     VugDev *gdev;
     VusDev *vdev_scsi;
     VuVirtq *vq;
+    VuVirtqElement *elem = NULL;
 
     assert(vu_dev);
 
@@ -248,7 +249,6 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
     g_debug("Got kicked on vq[%d]@%p", idx, vq);
 
     while (1) {
-        VuVirtqElement *elem;
         VirtIOSCSICmdReq *req;
         VirtIOSCSICmdResp *rsp;
 
@@ -288,6 +288,7 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
 
         free(elem);
     }
+    free(elem);
 }
 
 static void vus_queue_set_started(VuDev *vu_dev, int idx, bool started)
-- 
2.19.1

Re: [PATCH] vhost-user-scsi: Fix memleaks in vus_proc_req()

Posted by Raphael Norwitz 5 years, 2 months ago

On Tue, Nov 24, 2020 at 9:50 AM Alex Chen <alex.chen@huawei.com> wrote:
>
> The 'elem' is allocated memory in vu_queue_pop(), and it's memory should be
> freed in all error branchs after vu_queue_pop().

s/branchs/branches

> In addition, in order to free 'elem' memory outside of while(1) loop, move the
> definition of 'elem' to the begin of vus_proc_req().

s/begin/beginning

>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Alex Chen <alex.chen@huawei.com>

Other than spelling in the commit message, looks good to me.

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> ---
>  contrib/vhost-user-scsi/vhost-user-scsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c b/contrib/vhost-user-scsi/vhost-user-scsi.c
> index 0f9ba4b2a2..4639440a70 100644
> --- a/contrib/vhost-user-scsi/vhost-user-scsi.c
> +++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
> @@ -232,6 +232,7 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
>      VugDev *gdev;
>      VusDev *vdev_scsi;
>      VuVirtq *vq;
> +    VuVirtqElement *elem = NULL;
>
>      assert(vu_dev);
>
> @@ -248,7 +249,6 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
>      g_debug("Got kicked on vq[%d]@%p", idx, vq);
>
>      while (1) {
> -        VuVirtqElement *elem;
>          VirtIOSCSICmdReq *req;
>          VirtIOSCSICmdResp *rsp;
>
> @@ -288,6 +288,7 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
>
>          free(elem);
>      }
> +    free(elem);
>  }
>
>  static void vus_queue_set_started(VuDev *vu_dev, int idx, bool started)
> --
> 2.19.1
>
>

Re: [PATCH] vhost-user-scsi: Fix memleaks in vus_proc_req()

Posted by Alex Chen 5 years, 2 months ago

On 2020/11/24 23:37, Raphael Norwitz wrote:
> On Tue, Nov 24, 2020 at 9:50 AM Alex Chen <alex.chen@huawei.com> wrote:
>>
>> The 'elem' is allocated memory in vu_queue_pop(), and it's memory should be
>> freed in all error branchs after vu_queue_pop().
> 
> s/branchs/branches
> 
>> In addition, in order to free 'elem' memory outside of while(1) loop, move the
>> definition of 'elem' to the begin of vus_proc_req().
> 
> s/begin/beginning
> 
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> 
> Other than spelling in the commit message, looks good to me.
> 

Thanks for your review, I will fix it and send patch v2.

Thanks,
Alex

> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> 
>> ---
>>  contrib/vhost-user-scsi/vhost-user-scsi.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/contrib/vhost-user-scsi/vhost-user-scsi.c b/contrib/vhost-user-scsi/vhost-user-scsi.c
>> index 0f9ba4b2a2..4639440a70 100644
>> --- a/contrib/vhost-user-scsi/vhost-user-scsi.c
>> +++ b/contrib/vhost-user-scsi/vhost-user-scsi.c
>> @@ -232,6 +232,7 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
>>      VugDev *gdev;
>>      VusDev *vdev_scsi;
>>      VuVirtq *vq;
>> +    VuVirtqElement *elem = NULL;
>>
>>      assert(vu_dev);
>>
>> @@ -248,7 +249,6 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
>>      g_debug("Got kicked on vq[%d]@%p", idx, vq);
>>
>>      while (1) {
>> -        VuVirtqElement *elem;
>>          VirtIOSCSICmdReq *req;
>>          VirtIOSCSICmdResp *rsp;
>>
>> @@ -288,6 +288,7 @@ static void vus_proc_req(VuDev *vu_dev, int idx)
>>
>>          free(elem);
>>      }
>> +    free(elem);
>>  }
>>
>>  static void vus_queue_set_started(VuDev *vu_dev, int idx, bool started)
>> --
>> 2.19.1
>>
>>
> .
>