[libvirt] [PATCH 0/5] avoid memory leaks
Detected by Coverity. * daemon/remote.c: Calling allocation function 'get_nonnull_domain' to allocate memory to 'dom', but haven't released it in 'cleanup' section. * src/qemu/qemu_command.c: Taking 'if (!port)' true branch then 'def = NULL', the codes jump into 'error' section, the function 'virDomainDefFree(def)' willn't release it. * src/remote/remote_driver.c: Calling allocation function 'get_nonnull_domain' to allocate memory to 'dom', but haven't released it in 'no_memory' section. * src/util/pci.c: Calling allocation function 'pciGetVirtualFunctions' on 'virt_fns', but haven't released it in 'out' section. * src/vmx/vmx.c: Calling allocation function 'virConfReadMem' to allocate memory to 'conf' and then taking 'if (VIR_ALLOC(def) < 0)' true branch, the codes haven't released it before 'return NULL'. Signed-off-by: Alex Jia <ajia@redhat.com> --- daemon/remote.c | 2 ++ src/qemu/qemu_command.c | 1 - src/remote/remote_driver.c | 2 ++ src/util/pci.c | 1 + src/vmx/vmx.c | 1 + 5 files changed, 6 insertions(+), 1 deletions(-)
* daemon/remote.c: fix memory leak. Signed-off-by: Alex Jia <ajia@redhat.com> --- daemon/remote.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 38bbb10..45244f8 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -997,6 +997,8 @@ cleanup: VIR_FREE(ret->params.params_val); } } + if (dom) + virDomainFree(dom); VIR_FREE(params); return rv; } -- 1.7.1
On 09/18/2011 10:36 AM, ajia@redhat.com wrote:
* daemon/remote.c: fix memory leak.
Your commit message didn't mention which function you fixed,...
Signed-off-by: Alex Jia<ajia@redhat.com> --- daemon/remote.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 38bbb10..45244f8 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -997,6 +997,8 @@ cleanup:
and git didn't list it either, so I had to look. I tweaked the commit message to mention remoteDispatchDomainBlockStatsFlags, and that the leak was introduced in commit efa7fc9.
VIR_FREE(ret->params.params_val); } } + if (dom) + virDomainFree(dom); VIR_FREE(params); return rv;
ACK and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 09/20/2011 03:03 AM, Eric Blake wrote:
On 09/18/2011 10:36 AM, ajia@redhat.com wrote:
* daemon/remote.c: fix memory leak.
Your commit message didn't mention which function you fixed,...
Oh, I think 'git' will do this , and sorry about any inconvenient for you. Thanks, Alex
Signed-off-by: Alex Jia<ajia@redhat.com> --- daemon/remote.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 38bbb10..45244f8 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -997,6 +997,8 @@ cleanup:
and git didn't list it either, so I had to look. I tweaked the commit message to mention remoteDispatchDomainBlockStatsFlags, and that the leak was introduced in commit efa7fc9.
VIR_FREE(ret->params.params_val); } } + if (dom) + virDomainFree(dom); VIR_FREE(params); return rv;
ACK and pushed.
* src/qemu/qemu_command.c: fix memory leak. Signed-off-by: Alex Jia <ajia@redhat.com> --- src/qemu/qemu_command.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e8b1157..45d55fc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6540,7 +6540,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, host = disk->src; port = strchr(host, ':'); if (!port) { - def = NULL; qemuReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse nbd filename '%s'"), disk->src); goto error; -- 1.7.1
On 09/18/2011 10:36 AM, ajia@redhat.com wrote:
* src/qemu/qemu_command.c: fix memory leak.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/qemu/qemu_command.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e8b1157..45d55fc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6540,7 +6540,6 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, host = disk->src; port = strchr(host, ':'); if (!port) { - def = NULL;
Took me a few minutes of research to find when this stray line was introduced - we've had a leak on error since commit 036ad50 (0.8.7). ACK and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/remote/remote_driver.c: fix memory leak. Signed-off-by: Alex Jia <ajia@redhat.com> --- src/remote/remote_driver.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9d34b7e..9f30432 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3294,6 +3294,8 @@ no_memory: VIR_FREE(subject->identities); VIR_FREE(subject); } + if (dom) + virDomainFree(dom); return; } -- 1.7.1
On 09/18/2011 10:36 AM, ajia@redhat.com wrote:
* src/remote/remote_driver.c: fix memory leak.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/remote/remote_driver.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9d34b7e..9f30432 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3294,6 +3294,8 @@ no_memory: VIR_FREE(subject->identities); VIR_FREE(subject); } + if (dom) + virDomainFree(dom);
ACK that this is a leak, in remoteDomainBuildEventGraphics and introduced by commit 987e31e (v0.8.0). However, if we get here, we are guaranteed dom is non-NULL, so I removed the redundant if() before pushing. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/util/pci.c: fix memory leak. Signed-off-by: Alex Jia <ajia@redhat.com> --- src/util/pci.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/util/pci.c b/src/util/pci.c index 9873f33..8d8e157 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -1965,6 +1965,7 @@ out: for (i = 0; i < num_virt_fns; i++) VIR_FREE(virt_fns[i]); + VIR_FREE(virt_fns); VIR_FREE(vf_bdf); return ret; -- 1.7.1
On 09/18/2011 10:36 AM, ajia@redhat.com wrote:
* src/util/pci.c: fix memory leak.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/util/pci.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/util/pci.c b/src/util/pci.c index 9873f33..8d8e157 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -1965,6 +1965,7 @@ out: for (i = 0; i< num_virt_fns; i++) VIR_FREE(virt_fns[i]);
+ VIR_FREE(virt_fns);
ACK and pushed; leak introduced in pciGetVirtualFunctionIndex, introduced by commit 17d64ca (unreleased). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/vmx/vmx.c: fix memory leak. Signed-off-by: Alex Jia <ajia@redhat.com> --- src/vmx/vmx.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index dff3599..1ab15b7 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1253,6 +1253,7 @@ virVMXParseConfig(virVMXContext *ctx, virCapsPtr caps, const char *vmx) /* Allocate domain def */ if (VIR_ALLOC(def) < 0) { virReportOOMError(); + virConfFree(conf); return NULL; } -- 1.7.1
2011/9/18 <ajia@redhat.com>:
* src/vmx/vmx.c: fix memory leak.
Signed-off-by: Alex Jia <ajia@redhat.com> --- src/vmx/vmx.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index dff3599..1ab15b7 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1253,6 +1253,7 @@ virVMXParseConfig(virVMXContext *ctx, virCapsPtr caps, const char *vmx) /* Allocate domain def */ if (VIR_ALLOC(def) < 0) { virReportOOMError(); + virConfFree(conf); return NULL; }
Your patch is correct. But I'd suggest to replace the 'return NULL' with a 'goto cleanup' here instead of adding a virConfFree, as this is more in line with the surrounding code. Either way ACK. -- Matthias Bolte http://photron.blogspot.com
On 09/19/2011 12:47 AM, Matthias Bolte wrote:
2011/9/18<ajia@redhat.com>:
* src/vmx/vmx.c: fix memory leak.
Signed-off-by: Alex Jia<ajia@redhat.com> --- src/vmx/vmx.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index dff3599..1ab15b7 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1253,6 +1253,7 @@ virVMXParseConfig(virVMXContext *ctx, virCapsPtr caps, const char *vmx) /* Allocate domain def */ if (VIR_ALLOC(def)< 0) { virReportOOMError(); + virConfFree(conf); return NULL; } Your patch is correct.
But I'd suggest to replace the 'return NULL' with a 'goto cleanup' here instead of adding a virConfFree, as this is more in line with the surrounding code.
Either way ACK.
Yeah, 'def' has a initial value 'NULL', so 'goto cleanup' is perfected instead of adding a virConfFree. Matthias, thanks for your good suggestion, I will commit v2 patch for this. Regards, Alex
participants (4)
-
ajia＠redhat.com -
Alex Jia
-
Eric Blake -
Matthias Bolte