[PATCH 3/6] ch: Don't leak virCHDomainObjPrivate struct members

Michal Privoznik posted 6 patches 9 months, 1 week ago
[PATCH 3/6] ch: Don't leak virCHDomainObjPrivate struct members
Posted by Michal Privoznik 9 months, 1 week ago
There are some members of the virCHDomainObjPrivate struct that
are allocated at various stages of domain lifecycle but then are
never freed:

1) cgroup - allocated in virDomainCgroupSetupCgroup()
2) autoCpuset - this one is actually never allocated (and thus is
                always NULL, but soon it may be used. Just free
                it for now, which is a NOP anyways.
3) autoNodeset - same story as 2).

There are two more members, which shouldn't be freed:

1) driver - this is just a raw pointer to the CH driver (see
   virCHDomainObjPrivateAlloc()).

2) monitor - this member is cleared in virCHProcessStop(), way
             before control even gets to
             virCHDomainObjPrivateFree().

452 (400 direct, 52 indirect) bytes in 1 blocks are definitely lost in loss record 1,944 of 1,998
   at 0x484CEF3: calloc (vg_replace_malloc.c:1675)
   by 0x4F0E7A9: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8000.5)
   by 0x49479CE: virCgroupNewFromParent (vircgroup.c:893)
   by 0x49481BA: virCgroupNewDomainPartition (vircgroup.c:1068)
   by 0x494915E: virCgroupNewMachineManual (vircgroup.c:1378)
   by 0x49492FE: virCgroupNewMachine (vircgroup.c:1432)
   by 0x4B5E3DE: virDomainCgroupInitCgroup (domain_cgroup.c:377)
   by 0x4B5E9CD: virDomainCgroupSetupCgroup (domain_cgroup.c:524)
   by 0xB3AC693: virCHProcessStart (ch_process.c:951)
   by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246)
   by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188)
   by 0x168F91: remoteDispatchDomainCreateXML (remote_daemon_dispatch_stubs.h:5186)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/ch/ch_domain.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
index 4f5966adce..a08b18c5b9 100644
--- a/src/ch/ch_domain.c
+++ b/src/ch/ch_domain.c
@@ -65,6 +65,9 @@ virCHDomainObjPrivateFree(void *data)
 
     virChrdevFree(priv->chrdevs);
     g_free(priv->machineName);
+    virBitmapFree(priv->autoCpuset);
+    virBitmapFree(priv->autoNodeset);
+    virCgroupFree(priv->cgroup);
     g_free(priv);
 }
 
-- 
2.48.1
Re: [PATCH 3/6] ch: Don't leak virCHDomainObjPrivate struct members
Posted by Peter Krempa 9 months, 1 week ago
On Thu, Mar 13, 2025 at 14:44:35 +0100, Michal Privoznik wrote:
> There are some members of the virCHDomainObjPrivate struct that
> are allocated at various stages of domain lifecycle but then are
> never freed:
> 
> 1) cgroup - allocated in virDomainCgroupSetupCgroup()
> 2) autoCpuset - this one is actually never allocated (and thus is
>                 always NULL, but soon it may be used. Just free
>                 it for now, which is a NOP anyways.
> 3) autoNodeset - same story as 2).

So wait; was it copied from qemu without being used?

Will it actually be used soon? If no; I'd prefer to just delete the
members instead.

> 
> There are two more members, which shouldn't be freed:
> 
> 1) driver - this is just a raw pointer to the CH driver (see
>    virCHDomainObjPrivateAlloc()).
> 
> 2) monitor - this member is cleared in virCHProcessStop(), way
>              before control even gets to
>              virCHDomainObjPrivateFree().
> 
> 452 (400 direct, 52 indirect) bytes in 1 blocks are definitely lost in loss record 1,944 of 1,998
>    at 0x484CEF3: calloc (vg_replace_malloc.c:1675)
>    by 0x4F0E7A9: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.8000.5)
>    by 0x49479CE: virCgroupNewFromParent (vircgroup.c:893)
>    by 0x49481BA: virCgroupNewDomainPartition (vircgroup.c:1068)
>    by 0x494915E: virCgroupNewMachineManual (vircgroup.c:1378)
>    by 0x49492FE: virCgroupNewMachine (vircgroup.c:1432)
>    by 0x4B5E3DE: virDomainCgroupInitCgroup (domain_cgroup.c:377)
>    by 0x4B5E9CD: virDomainCgroupSetupCgroup (domain_cgroup.c:524)
>    by 0xB3AC693: virCHProcessStart (ch_process.c:951)
>    by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246)
>    by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188)
>    by 0x168F91: remoteDispatchDomainCreateXML (remote_daemon_dispatch_stubs.h:5186)
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/ch/ch_domain.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
> index 4f5966adce..a08b18c5b9 100644
> --- a/src/ch/ch_domain.c
> +++ b/src/ch/ch_domain.c
> @@ -65,6 +65,9 @@ virCHDomainObjPrivateFree(void *data)
>  
>      virChrdevFree(priv->chrdevs);
>      g_free(priv->machineName);
> +    virBitmapFree(priv->autoCpuset);
> +    virBitmapFree(priv->autoNodeset);
> +    virCgroupFree(priv->cgroup);
>      g_free(priv);
>  }

At least for the cgroup bit:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Re: [PATCH 3/6] ch: Don't leak virCHDomainObjPrivate struct members
Posted by Peter Krempa 9 months, 1 week ago
On Thu, Mar 13, 2025 at 15:26:52 +0100, Peter Krempa wrote:
> On Thu, Mar 13, 2025 at 14:44:35 +0100, Michal Privoznik wrote:
> > There are some members of the virCHDomainObjPrivate struct that
> > are allocated at various stages of domain lifecycle but then are
> > never freed:
> > 
> > 1) cgroup - allocated in virDomainCgroupSetupCgroup()
> > 2) autoCpuset - this one is actually never allocated (and thus is
> >                 always NULL, but soon it may be used. Just free
> >                 it for now, which is a NOP anyways.
> > 3) autoNodeset - same story as 2).
> 
> So wait; was it copied from qemu without being used?
> 
> Will it actually be used soon? If no; I'd prefer to just delete the
> members instead.

Ah I see that it's actually at least attempted to be read. So ignore
this and

Reviewed-by: Peter Krempa <pkrempa@redhat.com>