[v1] vhost-user-test: fix a memory leak

[PATCH] vhost-user-test: fix a memory leak

Posted by pannengyuan@huawei.com 6 years, 2 months ago

From: Pan Nengyuan <pannengyuan@huawei.com>

Spotted by ASAN.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
 tests/vhost-user-test.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 91ea373..54be931 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
     guint64 size;
 
     if (!wait_for_fds(s)) {
+        g_free(uri);
+        test_server_free(dest);
         return;
     }
 
-- 
2.7.2.windows.1

Re: [PATCH] vhost-user-test: fix a memory leak

Posted by Laurent Vivier 6 years, 1 month ago

On 11/12/2019 01:55, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> Spotted by ASAN.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
>  tests/vhost-user-test.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 91ea373..54be931 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
>      guint64 size;
>  
>      if (!wait_for_fds(s)) {
> +        g_free(uri);
> +        test_server_free(dest);
>          return;
>      }
>  

Don't we need also a g_string_free(dest_cmdline)?

I think it is also missing at the end of the function.

Thanks,
Laurent

Re: [PATCH] vhost-user-test: fix a memory leak

Posted by Pan Nengyuan 6 years, 1 month ago


On 2019/12/11 15:48, Laurent Vivier wrote:
> On 11/12/2019 01:55, pannengyuan@huawei.com wrote:
>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>
>> Spotted by ASAN.
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>>  tests/vhost-user-test.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
>> index 91ea373..54be931 100644
>> --- a/tests/vhost-user-test.c
>> +++ b/tests/vhost-user-test.c
>> @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
>>      guint64 size;
>>  
>>      if (!wait_for_fds(s)) {
>> +        g_free(uri);
>> +        test_server_free(dest);
>>          return;
>>      }
>>  
> 
> Don't we need also a g_string_free(dest_cmdline)?
> 
> I think it is also missing at the end of the function.
> 
Yes, you are right. But I'm surprised that it hasn't been detected by asan.

I will continue to try it and send a new version.

> Thanks,
> Laurent
> 
>

Re: [PATCH] vhost-user-test: fix a memory leak

Posted by Thomas Huth 6 years, 1 month ago

 Hi!

On 11/12/2019 01.55, pannengyuan@huawei.com wrote:
[...]
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 91ea373..54be931 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
>      guint64 size;
>  
>      if (!wait_for_fds(s)) {
> +        g_free(uri);
> +        test_server_free(dest);
>          return;
>      }

Well spotted. But I'd prefer to rather move the allocation of these
resources after the if-statement instead of doing the allocation at the
declaration of the variables already. Or maybe use a "goto out" and jump
to the end of the function instead? ... whatever you prefer, but
duplicating the "free" functions sounds like a cumbersome solution to me.

 Thanks,
  Thomas

Re: [PATCH] vhost-user-test: fix a memory leak

Posted by Marc-André Lureau 6 years, 1 month ago

Hi

On Wed, Dec 11, 2019 at 11:57 AM Thomas Huth <thuth@redhat.com> wrote:
>
>  Hi!
>
> On 11/12/2019 01.55, pannengyuan@huawei.com wrote:
> [...]
> > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> > index 91ea373..54be931 100644
> > --- a/tests/vhost-user-test.c
> > +++ b/tests/vhost-user-test.c
> > @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
> >      guint64 size;
> >
> >      if (!wait_for_fds(s)) {
> > +        g_free(uri);
> > +        test_server_free(dest);
> >          return;
> >      }
>
> Well spotted. But I'd prefer to rather move the allocation of these
> resources after the if-statement instead of doing the allocation at the
> declaration of the variables already. Or maybe use a "goto out" and jump
> to the end of the function instead? ... whatever you prefer, but
> duplicating the "free" functions sounds like a cumbersome solution to me.

g_auto (preferably) should work as well.


-- 
Marc-André Lureau

Re: [PATCH] vhost-user-test: fix a memory leak

Posted by Pan Nengyuan 6 years, 1 month ago


On 2019/12/11 16:18, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Dec 11, 2019 at 11:57 AM Thomas Huth <thuth@redhat.com> wrote:
>>
>>  Hi!
>>
>> On 11/12/2019 01.55, pannengyuan@huawei.com wrote:
>> [...]
>>> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
>>> index 91ea373..54be931 100644
>>> --- a/tests/vhost-user-test.c
>>> +++ b/tests/vhost-user-test.c
>>> @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
>>>      guint64 size;
>>>
>>>      if (!wait_for_fds(s)) {
>>> +        g_free(uri);
>>> +        test_server_free(dest);
>>>          return;
>>>      }
>>
>> Well spotted. But I'd prefer to rather move the allocation of these
>> resources after the if-statement instead of doing the allocation at the
>> declaration of the variables already. Or maybe use a "goto out" and jump
>> to the end of the function instead? ... whatever you prefer, but
>> duplicating the "free" functions sounds like a cumbersome solution to me.
> 
> g_auto (preferably) should work as well.

Yes, it should work as well. But try to keep it as it is, I will use a
"goto" instead.

Thanks.

> 
>

Re: [PATCH] vhost-user-test: fix a memory leak

Posted by Pan Nengyuan 6 years, 1 month ago


On 2019/12/11 15:57, Thomas Huth wrote:
>  Hi!
> 
> On 11/12/2019 01.55, pannengyuan@huawei.com wrote:
> [...]
>> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
>> index 91ea373..54be931 100644
>> --- a/tests/vhost-user-test.c
>> +++ b/tests/vhost-user-test.c
>> @@ -717,6 +717,8 @@ static void test_migrate(void *obj, void *arg, QGuestAllocator *alloc)
>>      guint64 size;
>>  
>>      if (!wait_for_fds(s)) {
>> +        g_free(uri);
>> +        test_server_free(dest);
>>          return;
>>      }
> 
> Well spotted. But I'd prefer to rather move the allocation of these
> resources after the if-statement instead of doing the allocation at the
> declaration of the variables already. Or maybe use a "goto out" and jump
> to the end of the function instead? ... whatever you prefer, but
> duplicating the "free" functions sounds like a cumbersome solution to me.
> 

Yes, I think use a "goto out" is more better. I will change it.

Thanks.

>  Thanks,
>   Thomas
> 
>