[PATCH 0/6] Couple of memleak fixes
*** BLURB HERE *** Michal Prívozník (6): network: Free inhibitor in networkStateCleanup() ch: Free retval of curl_slist_append() ch: Don't leak virCHDomainObjPrivate struct members ch: Free @iothreads array in virCHProcessSetupIOThreads() ch: Unref @cfg in virCHProcessStop() ch: Rework virCHProcessConnectMonitor() src/ch/ch_domain.c | 3 +++ src/ch/ch_monitor.c | 3 +++ src/ch/ch_process.c | 22 +++++++++++++--------- src/network/bridge_driver.c | 2 ++ 4 files changed, 21 insertions(+), 9 deletions(-) -- 2.48.1
The shutdown inhibitor is created in networkStateInitialize() but corresponding call to virInhibitorFree() is missing in networkStateCleanup() leading to a memleak: 116 (72 direct, 44 indirect) bytes in 1 blocks are definitely lost in loss record 1,769 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 0x4993B9B: virInhibitorNew (virinhibitor.c:152) by 0x5279394: networkStateInitialize (bridge_driver.c:654) by 0x4CC74DC: virStateInitialize (libvirt.c:665) by 0x15B719: daemonRunStateInit (remote_daemon.c:613) by 0x49F2B44: virThreadHelper (virthread.c:256) by 0x5356662: start_thread (in /usr/lib64/libc.so.6) by 0x53D7DA3: clone (in /usr/lib64/libc.so.6) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 80d2c3a1d5..2cad1c8cbe 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -802,6 +802,8 @@ networkStateCleanup(void) network_driver->lockFD); } + virInhibitorFree(network_driver->inhibitor); + virObjectUnref(network_driver->config); virObjectUnref(network_driver->dnsmasqCaps); -- 2.48.1
On Thu, Mar 13, 2025 at 14:44:33 +0100, Michal Privoznik wrote:
The shutdown inhibitor is created in networkStateInitialize() but corresponding call to virInhibitorFree() is missing in networkStateCleanup() leading to a memleak:
good that it's a "singleton"
116 (72 direct, 44 indirect) bytes in 1 blocks are definitely lost in loss record 1,769 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 0x4993B9B: virInhibitorNew (virinhibitor.c:152) by 0x5279394: networkStateInitialize (bridge_driver.c:654) by 0x4CC74DC: virStateInitialize (libvirt.c:665) by 0x15B719: daemonRunStateInit (remote_daemon.c:613) by 0x49F2B44: virThreadHelper (virthread.c:256) by 0x5356662: start_thread (in /usr/lib64/libc.so.6) by 0x53D7DA3: clone (in /usr/lib64/libc.so.6)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
There are two places where curl_slist_append() is called but corresponding call to curl_slist_free_all() is missing: virCHMonitorPutNoContent() and virCHMonitorGet() which leads to memleaks: 41 (16 direct, 25 indirect) bytes in 1 blocks are definitely lost in loss record 992 of 1,998 at 0x4845888: malloc (vg_replace_malloc.c:446) by 0x5B2F8FE: curl_slist_append (in /usr/lib64/libcurl.so.4.8.0) by 0xB3A7B41: virCHMonitorPutNoContent (ch_monitor.c:824) by 0xB3A89FF: virCHMonitorBootVM (ch_monitor.c:1030) by 0xB3AC6F1: virCHProcessStart (ch_process.c:967) by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246) by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188) by 0x168F91: remoteDispatchDomainCreateXML (remote_daemon_dispatch_stubs.h:5186) by 0x168F18: remoteDispatchDomainCreateXMLHelper (remote_daemon_dispatch_stubs.h:5167) by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423) by 0x4B1FB99: virNetServerProgramDispatch (virnetserverprogram.c:299) by 0x4B28B5E: virNetServerProcessMsg (virnetserver.c:135) 88 (16 direct, 72 indirect) bytes in 1 blocks are definitely lost in loss record 1,501 of 1,998 at 0x4845888: malloc (vg_replace_malloc.c:446) by 0x5B2F8FE: curl_slist_append (in /usr/lib64/libcurl.so.4.8.0) by 0xB3A7E41: virCHMonitorGet (ch_monitor.c:864) by 0xB3A92E2: virCHMonitorGetInfo (ch_monitor.c:1157) by 0xB3A9CEA: virCHProcessUpdateInfo (ch_process.c:142) by 0xB3AAD36: virCHProcessSetup (ch_process.c:492) by 0xB3AC75A: virCHProcessStart (ch_process.c:973) by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246) by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188) by 0x168F91: remoteDispatchDomainCreateXML (remote_daemon_dispatch_stubs.h:5186) by 0x168F18: remoteDispatchDomainCreateXMLHelper (remote_daemon_dispatch_stubs.h:5167) by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_monitor.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index e0d6b490de..0ba927a194 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -839,6 +839,8 @@ virCHMonitorPutNoContent(virCHMonitor *mon, const char *endpoint, if (responseCode == 200 || responseCode == 204) ret = 0; + curl_slist_free_all(headers); + return ret; } @@ -884,6 +886,7 @@ virCHMonitorGet(virCHMonitor *mon, const char *endpoint, virJSONValue **response cleanup: g_free(data.content); + curl_slist_free_all(headers); /* reset the libcurl handle to avoid leaking a stack pointer to data */ curl_easy_reset(mon->handle); -- 2.48.1
On Thu, Mar 13, 2025 at 14:44:34 +0100, Michal Privoznik wrote:
There are two places where curl_slist_append() is called but corresponding call to curl_slist_free_all() is missing: virCHMonitorPutNoContent() and virCHMonitorGet() which leads to memleaks:
41 (16 direct, 25 indirect) bytes in 1 blocks are definitely lost in loss record 992 of 1,998 at 0x4845888: malloc (vg_replace_malloc.c:446) by 0x5B2F8FE: curl_slist_append (in /usr/lib64/libcurl.so.4.8.0) by 0xB3A7B41: virCHMonitorPutNoContent (ch_monitor.c:824) by 0xB3A89FF: virCHMonitorBootVM (ch_monitor.c:1030) by 0xB3AC6F1: virCHProcessStart (ch_process.c:967) by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246) by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188) by 0x168F91: remoteDispatchDomainCreateXML (remote_daemon_dispatch_stubs.h:5186) by 0x168F18: remoteDispatchDomainCreateXMLHelper (remote_daemon_dispatch_stubs.h:5167) by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423) by 0x4B1FB99: virNetServerProgramDispatch (virnetserverprogram.c:299) by 0x4B28B5E: virNetServerProcessMsg (virnetserver.c:135)
88 (16 direct, 72 indirect) bytes in 1 blocks are definitely lost in loss record 1,501 of 1,998 at 0x4845888: malloc (vg_replace_malloc.c:446) by 0x5B2F8FE: curl_slist_append (in /usr/lib64/libcurl.so.4.8.0) by 0xB3A7E41: virCHMonitorGet (ch_monitor.c:864) by 0xB3A92E2: virCHMonitorGetInfo (ch_monitor.c:1157) by 0xB3A9CEA: virCHProcessUpdateInfo (ch_process.c:142) by 0xB3AAD36: virCHProcessSetup (ch_process.c:492) by 0xB3AC75A: virCHProcessStart (ch_process.c:973) by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246) by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188) by 0x168F91: remoteDispatchDomainCreateXML (remote_daemon_dispatch_stubs.h:5186) by 0x168F18: remoteDispatchDomainCreateXMLHelper (remote_daemon_dispatch_stubs.h:5167) by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_monitor.c | 3 +++ 1 file changed, 3 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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
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>
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>
When the CH driver starts a domain virCHProcessSetupIOThreads() is called eventually which in turn calls virCHMonitorGetIOThreads(). The latter returns an array of iothreads which is never freed leading to a memleak: 130 (104 direct, 26 indirect) bytes in 1 blocks are definitely lost in loss record 1,804 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 0xB3A9359: virCHMonitorGetIOThreads (ch_monitor.c:1183) by 0xB3AA5BB: virCHProcessSetupIOThreads (ch_process.c:348) by 0xB3AAC59: virCHProcessSetup (ch_process.c:480) by 0xB3AC75A: virCHProcessStart (ch_process.c:973) by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246) by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188) by 0x168F91: remoteDispatchDomainCreateXML (remote_daemon_dispatch_stubs.h:5186) by 0x168F18: remoteDispatchDomainCreateXMLHelper (remote_daemon_dispatch_stubs.h:5167) by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423) by 0x4B1FB99: virNetServerProgramDispatch (virnetserverprogram.c:299) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_process.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 9a85f7869e..cbf98faaf0 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -344,6 +344,7 @@ virCHProcessSetupIOThreads(virDomainObj *vm) virDomainIOThreadInfo **iothreads = NULL; size_t i; int niothreads; + int ret = -1; if ((niothreads = virCHMonitorGetIOThreads(priv->monitor, &iothreads)) < 0) return -1; @@ -351,9 +352,16 @@ virCHProcessSetupIOThreads(virDomainObj *vm) for (i = 0; i < niothreads; i++) { VIR_DEBUG("IOThread index = %zu , tid = %d", i, iothreads[i]->iothread_id); if (virCHProcessSetupIOThread(vm, iothreads[i]) < 0) - return -1; + goto cleanup; } - return 0; + + ret = 0; + cleanup: + for (i = 0; i < niothreads; i++) { + virDomainIOThreadInfoFree(iothreads[i]); + } + g_free(iothreads); + return ret; } static int -- 2.48.1
On Thu, Mar 13, 2025 at 14:44:36 +0100, Michal Privoznik wrote:
When the CH driver starts a domain virCHProcessSetupIOThreads() is called eventually which in turn calls virCHMonitorGetIOThreads(). The latter returns an array of iothreads which is never freed leading to a memleak:
130 (104 direct, 26 indirect) bytes in 1 blocks are definitely lost in loss record 1,804 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 0xB3A9359: virCHMonitorGetIOThreads (ch_monitor.c:1183) by 0xB3AA5BB: virCHProcessSetupIOThreads (ch_process.c:348) by 0xB3AAC59: virCHProcessSetup (ch_process.c:480) by 0xB3AC75A: virCHProcessStart (ch_process.c:973) by 0xB39B7D4: chDomainCreateXML (ch_driver.c:246) by 0x4CC9D32: virDomainCreateXML (libvirt-domain.c:188) by 0x168F91: remoteDispatchDomainCreateXML (remote_daemon_dispatch_stubs.h:5186) by 0x168F18: remoteDispatchDomainCreateXMLHelper (remote_daemon_dispatch_stubs.h:5167) by 0x4B20066: virNetServerProgramDispatchCall (virnetserverprogram.c:423) by 0x4B1FB99: virNetServerProgramDispatch (virnetserverprogram.c:299)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_process.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
At the beginning of virCHProcessStop() the ref to driver config is obtained (via virCHDriverGetConfig()), but corresponding unref call is lacking. Use g_autoptr() to make sure the config is unrefed always. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index cbf98faaf0..79f5990cc1 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -997,11 +997,11 @@ virCHProcessStop(virCHDriver *driver, virDomainObj *vm, virDomainShutoffReason reason) { + g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); int ret; int retries = 0; unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI; virCHDomainObjPrivate *priv = vm->privateData; - virCHDriverConfig *cfg = virCHDriverGetConfig(driver); virDomainDef *def = vm->def; size_t i; -- 2.48.1
On Thu, Mar 13, 2025 at 14:44:37 +0100, Michal Privoznik wrote:
At the beginning of virCHProcessStop() the ref to driver config is obtained (via virCHDriverGetConfig()), but corresponding unref call is lacking. Use g_autoptr() to make sure the config is unrefed always.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_process.c | 2 +-
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Firstly, let's switch from explicit virCHDriverGetConfig() + virObjectUnref() combo to g_autoptr(virCHDriverConfig). This leaves us with the @monitor variable which is initialized to NULL only to be then set to the retval of virCHMonitorNew() and returned instantly. Well, the variable is now useless and can be dropped. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_process.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 79f5990cc1..ee86430e08 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -53,13 +53,9 @@ virCHProcessConnectMonitor(virCHDriver *driver, virDomainObj *vm, int logfile) { - virCHMonitor *monitor = NULL; - virCHDriverConfig *cfg = virCHDriverGetConfig(driver); + g_autoptr(virCHDriverConfig) cfg = virCHDriverGetConfig(driver); - monitor = virCHMonitorNew(vm, cfg, logfile); - - virObjectUnref(cfg); - return monitor; + return virCHMonitorNew(vm, cfg, logfile); } static void -- 2.48.1
On Thu, Mar 13, 2025 at 14:44:38 +0100, Michal Privoznik wrote:
Firstly, let's switch from explicit virCHDriverGetConfig() + virObjectUnref() combo to g_autoptr(virCHDriverConfig). This leaves us with the @monitor variable which is initialized to NULL only to be then set to the retval of virCHMonitorNew() and returned instantly. Well, the variable is now useless and can be dropped.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/ch/ch_process.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik -
Peter Krempa