conf: use virDomainDeviceDefFree free dev

[libvirt] [PATCH] conf: use virDomainDeviceDefFree free dev

Posted by Xu Yandong 4 years, 7 months ago

In function virDomainDeviceDefParse, we shoud use virDomainDeviceDefFree
free data structure avoid potential memory leak.

Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
---
 src/conf/domain_conf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 848c831330..8fb9480827 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16504,7 +16504,8 @@ virDomainDeviceDefParse(const char *xmlStr,
     return dev;
 
  error:
-    VIR_FREE(dev);
+    virDomainDeviceDefFree(dev);
+    dev = NULL;
     goto cleanup;
 }
 
-- 
2.18.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: use virDomainDeviceDefFree free dev

Posted by Daniel Henrique Barboza 4 years, 7 months ago


On 9/19/19 5:01 AM, Xu Yandong wrote:
> In function virDomainDeviceDefParse, we shoud use virDomainDeviceDefFree
> free data structure avoid potential memory leak.
>
> Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
> ---
>   src/conf/domain_conf.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 848c831330..8fb9480827 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16504,7 +16504,8 @@ virDomainDeviceDefParse(const char *xmlStr,
>       return dev;
>   
>    error:
> -    VIR_FREE(dev);
> +    virDomainDeviceDefFree(dev);
> +    dev = NULL;

You don't need the 'dev = NULL' after virDomainDeviceDefFree(dev). This
function will end up calling VIR_FREE(dev) in the end, which will make
dev = NULL.


WIthout the extra dev = NULL assignment:


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


>       goto cleanup;
>   }
>   

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: use virDomainDeviceDefFree free dev

Posted by Daniel Henrique Barboza 4 years, 7 months ago


On 9/19/19 5:01 AM, Xu Yandong wrote:
> In function virDomainDeviceDefParse, we shoud use virDomainDeviceDefFree
> free data structure avoid potential memory leak.
>
> Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
> ---
>   src/conf/domain_conf.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 848c831330..8fb9480827 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16504,7 +16504,8 @@ virDomainDeviceDefParse(const char *xmlStr,
>       return dev;
>   
>    error:
> -    VIR_FREE(dev);
> +    virDomainDeviceDefFree(dev);
> +    dev = NULL;
>       goto cleanup;


Just noticed that the current master code does not have this potential
leak anymore. It was fixed by commit 475777c9ec, introducing
VIR_AUTOFREE() for virDomainDeviceDefPtr.



Thanks,


DHB





>   }
>   

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: use virDomainDeviceDefFree free dev

Posted by Peter Krempa 4 years, 7 months ago

On Thu, Sep 19, 2019 at 10:33:53 -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 9/19/19 5:01 AM, Xu Yandong wrote:
> > In function virDomainDeviceDefParse, we shoud use virDomainDeviceDefFree
> > free data structure avoid potential memory leak.
> > 
> > Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
> > ---
> >   src/conf/domain_conf.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 848c831330..8fb9480827 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -16504,7 +16504,8 @@ virDomainDeviceDefParse(const char *xmlStr,
> >       return dev;
> >    error:
> > -    VIR_FREE(dev);
> > +    virDomainDeviceDefFree(dev);
> > +    dev = NULL;
> >       goto cleanup;
> 
> 
> Just noticed that the current master code does not have this potential
> leak anymore. It was fixed by commit 475777c9ec, introducing
> VIR_AUTOFREE() for virDomainDeviceDefPtr.

VIR_AUTOFREE calls only a VIR_FREE to release the memory associated with
'dev', so if this patch fixes a leak of one of the members of the 'dev'
a patch is still required with a different approach. E.g. VIR_AUTOPTR.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: use virDomainDeviceDefFree free dev

Posted by Ján Tomko 4 years, 2 months ago

On Thu, Sep 19, 2019 at 05:23:26PM +0200, Peter Krempa wrote:
>On Thu, Sep 19, 2019 at 10:33:53 -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 9/19/19 5:01 AM, Xu Yandong wrote:
>> > In function virDomainDeviceDefParse, we shoud use virDomainDeviceDefFree
>> > free data structure avoid potential memory leak.
>> >
>> > Signed-off-by: Xu Yandong <xuyandong2@huawei.com>
>> > ---
>> >   src/conf/domain_conf.c | 3 ++-
>> >   1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> > index 848c831330..8fb9480827 100644
>> > --- a/src/conf/domain_conf.c
>> > +++ b/src/conf/domain_conf.c
>> > @@ -16504,7 +16504,8 @@ virDomainDeviceDefParse(const char *xmlStr,
>> >       return dev;
>> >    error:
>> > -    VIR_FREE(dev);
>> > +    virDomainDeviceDefFree(dev);
>> > +    dev = NULL;
>> >       goto cleanup;
>>
>>
>> Just noticed that the current master code does not have this potential
>> leak anymore. It was fixed by commit 475777c9ec, introducing
>> VIR_AUTOFREE() for virDomainDeviceDefPtr.
>
>VIR_AUTOFREE calls only a VIR_FREE to release the memory associated with
>'dev', so if this patch fixes a leak of one of the members of the 'dev'
>a patch is still required with a different approach. E.g. VIR_AUTOPTR.

Pushed now:
commit 3f40a487a9a820004214574f82f0e492a836adf0
     conf: use correct free function for virDomainDeviceDef

Jano