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

Michal Privoznik posted 6 patches 11 months ago
[PATCH 3/6] ch: Don't leak virCHDomainObjPrivate struct members
Posted by Michal Privoznik 11 months 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 11 months 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 11 months 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>