Re: [libvirt] [PATCH] daemon: Plug memory leaks

On Thu, Apr 12, 2012 at 03:24:08PM +0800, Alex Jia wrote: > Detected by valgrind. Leaks are introduced in commit 6e6e9be. > > * daemon/libvirtd-config.c (macro GET_CONF_STR): fix memory leaks. > > How to reproduce? > > % make&& make -C tests check TESTS=libvirtdconftest > % cd tests&& valgrind -v --leak-check=full ./libvirtdconftest > > actual result: > > ==11008== 185 bytes in 5 blocks are definitely lost in loss record 3 of 5 > ==11008== at 0x4A05FDE: malloc (vg_replace_malloc.c:236) > ==11008== by 0x39CF07F6E1: strdup (strdup.c:43) > ==11008== by 0x406626: daemonConfigLoadOptions (libvirtd-config.c:438) > ==11008== by 0x406800: daemonConfigLoadData (libvirtd-config.c:492) > ==11008== by 0x403CCF: testCorrupt (libvirtdconftest.c:110) > ==11008== by 0x404FAD: virtTestRun (testutils.c:145) > ==11008== by 0x403A34: mymain (libvirtdconftest.c:219) > ==11008== by 0x404687: virtTestMain (testutils.c:700) > ==11008== by 0x39CF01ECDC: (below main) (libc-start.c:226) > ==11008== > ==11008== LEAK SUMMARY: > ==11008== definitely lost: 185 bytes in 5 blocks > > Signed-off-by: Alex Jia&lt;ajia(a)redhat.com&gt; > --- > daemon/libvirtd-config.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c > index 4d041f0..0a4323a 100644 > --- a/daemon/libvirtd-config.c > +++ b/daemon/libvirtd-config.c > @@ -152,6 +152,7 @@ checkType (virConfValuePtr p, const char *filename, > virReportOOMError(); \ > goto error; \ > } \ > + VIR_FREE(data->var_name); \ > } \ > } while (0) Errr, no. Please look at the context of what's going on, rather than just blindly adding code to free a block at the point where valgrind shows the allocation. The full context to this diff #define GET_CONF_STR(conf, filename, var_name) \ do { \ virConfValuePtr p = virConfGetValue (conf, #var_name); \ if (p) { \ if (checkType (p, filename, #var_name, VIR_CONF_STRING)< 0) \ goto error; \ VIR_FREE(data->var_name); \ if (!(data->var_name = strdup (p->str))) { \ virReportOOMError(); \ goto error; \ } \ } \ } while (0) Your suggested change means we would be free'ing the config file before we have even used it. If you look at this line:

> ==11008== by 0x406626: daemonConfigLoadOptions (libvirtd-config.c:438) You'll see it matches: GET_CONF_STR (conf, filename, host_uuid); There are no leak reports for other uses of GET_CONF_STR, so this suggests that 'host_uuid' is not being freed. Check daemonConfigFree and you'll see this is indeed missing

Daniel