Re: [libvirt] Fixes for several memory leaks

On 03/03/2011 08:28 PM, Phil Petty (phpetty) wrote: First, a big thank you for this. You found several real bugs, some easier than others to trigger. Wow, your formatting came across awkwardly. Please send plain-text emails, rather than html formatting. I had to manually re-apply the changes in this patch, because 'git am' couldn't read it. Also, your line numbers didn't match latest libvirt.git; if you diffed against the 0.8.8 release instead of libvirt.git, then it helps to say so. This hunk is okay, but not strictly necessary, since the function guarantees that cleanup will be reached and cleanup also does VIR_FREE(prop). Ah - the real leak. uuidstr was NOT being cleaned up on error. But again, this hunk is not strictly necessary. Hmm, I'm more used to unified diffs rather than context diffs (that is, 'git diff' matches 'diff -u', not 'diff -c'). This fails 'make syntax-check' - you have a useless if-before-free. But you are correct that we need to add a VIR_FREE(uuidstr). Not strictly necessary, but I left it in. Another useless-if-before-free, but a valid leak plug. Useless if-before-free, but I agree that if you have more than one name= parameter, we leaked the earlier names; likewise for the other variables parsed in this for loop. You missed that pkipath needed the same treatment. Good catch. I also cleanup up the return() syntax (return is not a function). Here, it's easier to fix the leak by doing goto cleanup rather than an early return. Ah, a leak and a formatting problem. While there, I shortened the error message (virXMLError already prints __func__, so repeating the function name ourselves is redundant). 'diff -p' is handy, especially in large files like this with lots of copy-and-paste and intermediate churn that affects line numbers, to be sure that I'm applying the patch to the right function (all the more reason why telling us where you diff'd against, if not libvirt.git, would be handy). Fortunately, I found where you were referring to, in cmdCd. Ah, you invented dir_root to know whether to free dir. However, I'm opposed to adding any more use of TRUE/FALSE aberrations than absolutely necessary in that file; I've been meaning to convert it to properly use <stdbool.h> throughout. However, your fix introduces a double-free in the case of 'cd /dir'. You really want to free dir only when virGetUserDirectory returned a malloc'd result, but not when it was supplied on the command line or when it defaulted to /; hence the name dir_root isn't quite right. Here's what I'm pushing in your name (plus a line in AUTHORS): diff --git i/src/conf/secret_conf.c w/src/conf/secret_conf.c index bbdad89..fc4ae82 100644 --- i/src/conf/secret_conf.c +++ w/src/conf/secret_conf.c @@ -1,7 +1,7 @@ /* * secret_conf.c: internal <secret> XML handling * - * Copyright (C) 2009 Red Hat, Inc. + * Copyright (C) 2009, 2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -182,6 +182,7 @@ secretXMLParseNode(xmlDocPtr xml, xmlNodePtr root) cleanup: VIR_FREE(prop); + VIR_FREE(uuidstr); virSecretDefFree(def); xmlXPathFreeContext(ctxt); return ret; diff --git i/src/nwfilter/nwfilter_gentech_driver.c w/src/nwfilter/nwfilter_gentech_driver.c index facad13..f89fb79 100644 --- i/src/nwfilter/nwfilter_gentech_driver.c +++ w/src/nwfilter/nwfilter_gentech_driver.c @@ -1,6 +1,7 @@ /* * nwfilter_gentech_driver.c: generic technology driver * + * Copyright (C) 2011 Red Hat, Inc. * Copyright (C) 2010 IBM Corp. * Copyright (C) 2010 Stefan Berger * @@ -660,8 +661,6 @@ virNWFilterInstantiate(virConnectPtr conn, } virNWFilterUnlockIface(ifname); - - VIR_FREE(ptrs); } err_exit: @@ -670,6 +669,7 @@ err_exit: virNWFilterRuleInstFree(insts[j]); VIR_FREE(insts); + VIR_FREE(ptrs); virNWFilterHashTableFree(missing_vars); diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c index 4ca52ba..58d7f96 100644 --- i/src/remote/remote_driver.c +++ w/src/remote/remote_driver.c @@ -479,22 +479,27 @@ doRemoteOpen (virConnectPtr conn, for (i = 0; i < vars->n; i++) { var = &vars->p[i]; if (STRCASEEQ (var->name, "name")) { + VIR_FREE(name); name = strdup (var->value); if (!name) goto out_of_memory; var->ignore = 1; } else if (STRCASEEQ (var->name, "command")) { + VIR_FREE(command); command = strdup (var->value); if (!command) goto out_of_memory; var->ignore = 1; } else if (STRCASEEQ (var->name, "socket")) { + VIR_FREE(sockname); sockname = strdup (var->value); if (!sockname) goto out_of_memory; var->ignore = 1; } else if (STRCASEEQ (var->name, "auth")) { + VIR_FREE(authtype); authtype = strdup (var->value); if (!authtype) goto out_of_memory; var->ignore = 1; } else if (STRCASEEQ (var->name, "netcat")) { + VIR_FREE(netcat); netcat = strdup (var->value); if (!netcat) goto out_of_memory; var->ignore = 1; @@ -511,6 +516,7 @@ doRemoteOpen (virConnectPtr conn, else priv->debugLog = stderr; } else if (STRCASEEQ(var->name, "pkipath")) { + VIR_FREE(pkipath); pkipath = strdup(var->value); if (!pkipath) goto out_of_memory; var->ignore = 1; diff --git i/src/util/conf.c w/src/util/conf.c index d9a7603..71a344f 100644 --- i/src/util/conf.c +++ w/src/util/conf.c @@ -1,7 +1,7 @@ /** * conf.c: parser for a subset of the Python encoded Xen configuration files * - * Copyright (C) 2006, 2007, 2008, 2009, 2010 Red Hat, Inc. + * Copyright (C) 2006-2011 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -648,22 +648,23 @@ virConfParseStatement(virConfParserCtxtPtr ctxt) SKIP_BLANKS_AND_EOL; if (CUR == '#') { - return(virConfParseComment(ctxt)); + return virConfParseComment(ctxt); } name = virConfParseName(ctxt); if (name == NULL) - return(-1); + return -1; SKIP_BLANKS; if (CUR != '=') { virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting an assignment")); - return(-1); + VIR_FREE(name); + return -1; } NEXT; SKIP_BLANKS; value = virConfParseValue(ctxt); if (value == NULL) { VIR_FREE(name); - return(-1); + return -1; } SKIP_BLANKS; if (CUR == '#') { @@ -675,16 +676,16 @@ virConfParseStatement(virConfParserCtxtPtr ctxt) virReportOOMError(); VIR_FREE(name); virConfFreeValue(value); - return(-1); + return -1; } } if (virConfAddEntry(ctxt->conf, name, value, comm) == NULL) { VIR_FREE(name); virConfFreeValue(value); VIR_FREE(comm); - return(-1); + return -1; } - return(0); + return 0; } /** diff --git i/src/util/storage_file.c w/src/util/storage_file.c index 62dad0d..ede79fa 100644 --- i/src/util/storage_file.c +++ w/src/util/storage_file.c @@ -1,7 +1,7 @@ /* * storage_file.c: file utility functions for FS storage backend * - * Copyright (C) 2007-2010 Red Hat, Inc. + * Copyright (C) 2007-2011 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -838,8 +838,9 @@ virStorageFileGetMetadataFromFD(const char *path, if (format < 0 || format >= VIR_STORAGE_FILE_LAST) { - virReportSystemError(EINVAL, _("unknown storage file format %d"), format); - return -1; + virReportSystemError(EINVAL, _("unknown storage file format %d"), + format); + goto cleanup; } ret = virStorageFileGetMetadataFromBuf(format, path, head, len, meta); diff --git i/src/util/xml.c w/src/util/xml.c index ff340d8..2c50e14 100644 --- i/src/util/xml.c +++ w/src/util/xml.c @@ -105,9 +105,10 @@ virXPathStringLimit(const char *xpath, if (tmp != NULL && strlen(tmp) >= maxlen) { virXMLError(VIR_ERR_INTERNAL_ERROR, - _("\'%s\' value longer than %zd bytes in virXPathStringLimit()"), + _("\'%s\' value longer than %zd bytes"), xpath, maxlen); - return NULL; + VIR_FREE(tmp); + return NULL; } return tmp; diff --git i/tools/virsh.c w/tools/virsh.c index 62fca17..2555421 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -9518,6 +9518,8 @@ cmdCd(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { const char *dir; int found; + int ret = TRUE; + bool dir_malloced = false; if (!ctl->imode) { vshError(ctl, "%s", _("cd: command valid only in interactive mode")); @@ -9528,16 +9530,19 @@ cmdCd(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) if (!found) { uid_t uid = geteuid(); dir = virGetUserDirectory(uid); + dir_malloced = !!dir; } if (!dir) dir = "/"; - if (chdir (dir) == -1) { + if (chdir(dir) == -1) { vshError(ctl, _("cd: %s: %s"), strerror(errno), dir); - return FALSE; + ret = FALSE; } - return TRUE; + if (dir_malloced) + VIR_FREE(dir); + return ret; } #endif -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org