[libvirt] PATCH: Fix some more memory leaks

While testing Cole's series of patches I identified a couple more places where we leak memory. In libvirt.c, the default authentication callback uses uninitialized data, and indeed strdup()'s it and this is then never released. This simply disables that bit of code. In qparams.c when free'ing the struct the 'p' struct field was not released. I took the opportunity to switch it over to using the new style memory.h functions In remote.c there were a couple of handlers which forgot to free the virDomainPtr object when they were done. qemud/remote.c | 18 ++++++++++++++---- src/libvirt.c | 16 +++++++++------- src/qparams.c | 31 +++++++++++++------------------ 3 files changed, 36 insertions(+), 29 deletions(-) Dan. Index: qemud/remote.c =================================================================== RCS file: /data/cvs/libvirt/qemud/remote.c,v retrieving revision 1.32 diff -u -p -r1.32 remote.c --- qemud/remote.c 21 May 2008 20:53:30 -0000 1.32 +++ qemud/remote.c 21 May 2008 21:16:42 -0000 @@ -792,8 +792,11 @@ remoteDispatchDomainBlockStats (struct q } path = args->path; - if (virDomainBlockStats (dom, path, &stats, sizeof stats) == -1) + if (virDomainBlockStats (dom, path, &stats, sizeof stats) == -1) { + virDomainFree (dom); return -1; + } + virDomainFree (dom); ret->rd_req = stats.rd_req; ret->rd_bytes = stats.rd_bytes; @@ -823,8 +826,11 @@ remoteDispatchDomainInterfaceStats (stru } path = args->path; - if (virDomainInterfaceStats (dom, path, &stats, sizeof stats) == -1) + if (virDomainInterfaceStats (dom, path, &stats, sizeof stats) == -1) { + virDomainFree (dom); return -1; + } + virDomainFree (dom); ret->rx_bytes = stats.rx_bytes; ret->rx_packets = stats.rx_packets; @@ -1258,7 +1264,11 @@ remoteDispatchDomainMigratePerform (stru args->cookie.cookie_len, args->uri, args->flags, dname, args->resource); - if (r == -1) return -1; + if (r == -1) { + virDomainFree (dom); + return -1; + } + virDomainFree (dom); return 0; } @@ -1281,7 +1291,7 @@ remoteDispatchDomainMigrateFinish (struc if (ddom == NULL) return -1; make_nonnull_domain (&ret->ddom, ddom); - + virDomainFree (ddom); return 0; } Index: src/libvirt.c =================================================================== RCS file: /data/cvs/libvirt/src/libvirt.c,v retrieving revision 1.140 diff -u -p -r1.140 libvirt.c --- src/libvirt.c 21 May 2008 20:53:31 -0000 1.140 +++ src/libvirt.c 21 May 2008 21:16:45 -0000 @@ -170,13 +170,15 @@ static int virConnectAuthCallbackDefault return -1; } - if (STREQ(bufptr, "") && cred[i].defresult) - cred[i].result = strdup(cred[i].defresult); - else - cred[i].result = strdup(bufptr); - if (!cred[i].result) - return -1; - cred[i].resultlen = strlen(cred[i].result); + if (cred[i].type != VIR_CRED_EXTERNAL) { + if (STREQ(bufptr, "") && cred[i].defresult) + cred[i].result = strdup(cred[i].defresult); + else + cred[i].result = strdup(bufptr); + if (!cred[i].result) + return -1; + cred[i].resultlen = strlen(cred[i].result); + } } return 0; Index: src/qparams.c =================================================================== RCS file: /data/cvs/libvirt/src/qparams.c,v retrieving revision 1.4 diff -u -p -r1.4 qparams.c --- src/qparams.c 28 Apr 2008 15:14:59 -0000 1.4 +++ src/qparams.c 21 May 2008 21:16:45 -0000 @@ -27,7 +27,7 @@ #include <stdarg.h> #include "buf.h" - +#include "memory.h" #include "qparams.h" struct qparam_set * @@ -39,13 +39,12 @@ new_qparam_set (int init_alloc, ...) if (init_alloc <= 0) init_alloc = 1; - ps = malloc (sizeof (*ps)); - if (!ps) return NULL; + if (VIR_ALLOC(ps) < 0) + return NULL; ps->n = 0; ps->alloc = init_alloc; - ps->p = malloc (init_alloc * sizeof (ps->p[0])); - if (!ps->p) { - free (ps); + if (VIR_ALLOC_N(ps->p, ps->alloc) < 0) { + VIR_FREE (ps); return NULL; } @@ -87,13 +86,8 @@ append_qparams (struct qparam_set *ps, . static int grow_qparam_set (struct qparam_set *ps) { - struct qparam *old_p; - if (ps->n >= ps->alloc) { - old_p = ps->p; - ps->p = realloc (ps->p, 2 * ps->alloc * sizeof (ps->p[0])); - if (!ps->p) { - ps->p = old_p; + if (VIR_REALLOC_N(ps->p, ps->alloc * 2) < 0) { perror ("realloc"); return -1; } @@ -115,13 +109,13 @@ append_qparam (struct qparam_set *ps, pvalue = strdup (value); if (!pvalue) { - free (pname); + VIR_FREE (pname); return -1; } if (grow_qparam_set (ps) == -1) { - free (pname); - free (pvalue); + VIR_FREE (pname); + VIR_FREE (pvalue); return -1; } @@ -161,10 +155,11 @@ free_qparam_set (struct qparam_set *ps) int i; for (i = 0; i < ps->n; ++i) { - free (ps->p[i].name); - free (ps->p[i].value); + VIR_FREE (ps->p[i].name); + VIR_FREE (ps->p[i].value); } - free (ps); + VIR_FREE (ps->p); + VIR_FREE (ps); } struct qparam_set * -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Wed, May 21, 2008 at 10:21:09PM +0100, Daniel P. Berrange wrote:
While testing Cole's series of patches I identified a couple more places where we leak memory.
In libvirt.c, the default authentication callback uses uninitialized data, and indeed strdup()'s it and this is then never released. This simply disables that bit of code.
In qparams.c when free'ing the struct the 'p' struct field was not released. I took the opportunity to switch it over to using the new style memory.h functions
In remote.c there were a couple of handlers which forgot to free the virDomainPtr object when they were done.
Sure, +1. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

"Daniel P. Berrange" <berrange@redhat.com> wrote:
While testing Cole's series of patches I identified a couple more places where we leak memory.
In libvirt.c, the default authentication callback uses uninitialized data, and indeed strdup()'s it and this is then never released. This simply disables that bit of code.
In qparams.c when free'ing the struct the 'p' struct field was not released. I took the opportunity to switch it over to using the new style memory.h functions
In remote.c there were a couple of handlers which forgot to free the virDomainPtr object when they were done.
qemud/remote.c | 18 ++++++++++++++---- src/libvirt.c | 16 +++++++++------- src/qparams.c | 31 +++++++++++++------------------ 3 files changed, 36 insertions(+), 29 deletions(-)
Looks good. I suppose the tests that exposed the leaks aren't yet run as part of "make check". It'd be nice...
Index: qemud/remote.c =================================================================== RCS file: /data/cvs/libvirt/qemud/remote.c,v retrieving revision 1.32 diff -u -p -r1.32 remote.c --- qemud/remote.c 21 May 2008 20:53:30 -0000 1.32 +++ qemud/remote.c 21 May 2008 21:16:42 -0000 @@ -792,8 +792,11 @@ remoteDispatchDomainBlockStats (struct q } path = args->path;
- if (virDomainBlockStats (dom, path, &stats, sizeof stats) == -1) + if (virDomainBlockStats (dom, path, &stats, sizeof stats) == -1) { + virDomainFree (dom); return -1; + } + virDomainFree (dom);
Recently, I've come to prefer using a single free call, snuggled right up against the last use (as long as you don't mind the additional variable). E.g., fail = (virDomainBlockStats (dom, path, &stats, sizeof stats) == -1); virDomainFree (dom); if (fail) return -1;
ret->rd_req = stats.rd_req; ret->rd_bytes = stats.rd_bytes; @@ -823,8 +826,11 @@ remoteDispatchDomainInterfaceStats (stru } path = args->path;
- if (virDomainInterfaceStats (dom, path, &stats, sizeof stats) == -1) + if (virDomainInterfaceStats (dom, path, &stats, sizeof stats) == -1) { + virDomainFree (dom); return -1; + } + virDomainFree (dom);
ret->rx_bytes = stats.rx_bytes; ret->rx_packets = stats.rx_packets; @@ -1258,7 +1264,11 @@ remoteDispatchDomainMigratePerform (stru args->cookie.cookie_len, args->uri, args->flags, dname, args->resource); - if (r == -1) return -1; + if (r == -1) { + virDomainFree (dom); + return -1; + } + virDomainFree (dom);
Here, you can call virDomainFree just once, before the "if", regardless.

On Thu, May 22, 2008 at 11:13:31AM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
While testing Cole's series of patches I identified a couple more places where we leak memory.
In libvirt.c, the default authentication callback uses uninitialized data, and indeed strdup()'s it and this is then never released. This simply disables that bit of code.
In qparams.c when free'ing the struct the 'p' struct field was not released. I took the opportunity to switch it over to using the new style memory.h functions
In remote.c there were a couple of handlers which forgot to free the virDomainPtr object when they were done.
qemud/remote.c | 18 ++++++++++++++---- src/libvirt.c | 16 +++++++++------- src/qparams.c | 31 +++++++++++++------------------ 3 files changed, 36 insertions(+), 29 deletions(-)
Looks good. I suppose the tests that exposed the leaks aren't yet run as part of "make check". It'd be nice...
I've got another patching in works for that, to fully validate OOM handling, and test qparams completely. Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, May 22, 2008 at 11:13:31AM +0200, Jim Meyering wrote:
- if (r == -1) return -1; + if (r == -1) { + virDomainFree (dom); + return -1; + } + virDomainFree (dom);
Here, you can call virDomainFree just once, before the "if", regardless.
I've changed this one, since it was already using a return status variable, but left the other as is. Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel P. Berrange
-
Daniel Veillard
-
Jim Meyering