[libvirt] Fixes for several memory leaks
Eric Blake
eblake at redhat.com
Fri Mar 4 16:53:28 UTC 2011
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.
> Index: libvirt/src/conf/secret_conf.c
>
> ===================================================================
>
> ***************
>
> *** 154,160 ****
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.
>
> _("invalid value of 'private'"));
>
> goto cleanup;
>
> }
>
> - VIR_FREE(prop);
>
> }
This hunk is okay, but not strictly necessary, since the function
guarantees that cleanup will be reached and cleanup also does
VIR_FREE(prop).
>
>
>
> uuidstr = virXPathString("string(./uuid)", ctxt);
>
> --- 154,159 ----
>
> ***************
>
> *** 170,176 ****
>
> "%s", _("malformed uuid element"));
>
> goto cleanup;
>
> }
>
> - VIR_FREE(uuidstr);
>
> }
Ah - the real leak. uuidstr was NOT being cleaned up on error. But
again, this hunk is not strictly necessary.
> cleanup:
>
> ! VIR_FREE(prop);
Hmm, I'm more used to unified diffs rather than context diffs (that is,
'git diff' matches 'diff -u', not 'diff -c').
> cleanup:
>
> ! if (prop) {
>
> ! VIR_FREE(prop);
>
> ! }
>
> ! if (uuidstr) {
>
> ! VIR_FREE(uuidstr);
>
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).
> Index: libvirt/src/nwfilter/nwfilter_gentech_driver.c
>
> ===================================================================
>
> ***************
>
> *** 662,673 ****
>
> }
>
>
>
> virNWFilterUnlockIface(ifname);
>
> -
>
> - VIR_FREE(ptrs);
Not strictly necessary, but I left it in.
> err_exit:
>
>
>
> + if (ptrs) {
>
> + VIR_FREE(ptrs);
Another useless-if-before-free, but a valid leak plug.
> Index: libvirt/src/remote/remote_driver.c
>
> ===================================================================
>
> ***************
>
> *** 474,495 ****
>
> --- 474,510 ----
>
> for (i = 0; i < vars->n; i++) {
>
> var = &vars->p[i];
>
> if (STRCASEEQ (var->name, "name")) {
>
> + if (name) {
>
> + VIR_FREE(name);
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.
> } else if (STRCASEEQ (var->name, "netcat")) {
>
> + if (netcat) {
>
> + VIR_FREE(netcat);
You missed that pkipath needed the same treatment.
> Index: libvirt/src/util/conf.c
>
> ===================================================================
>
> ***************
>
> *** 651,656 ****
>
> --- 651,657 ----
>
> SKIP_BLANKS;
>
> if (CUR != '=') {
>
> virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting an
> assignment"));
>
> + VIR_FREE(name);
>
> return(-1);
Good catch. I also cleanup up the return() syntax (return is not a
function).
> Index: libvirt/src/util/storage_file.c
>
> ===================================================================
>
> ***************
>
> *** 742,747 ****
>
> --- 742,748 ----
>
> if (format < 0 ||
>
> format >= VIR_STORAGE_FILE_LAST) {
>
> virReportSystemError(EINVAL, _("unknown storage file format
> %d"), format);
>
> + VIR_FREE(head);
>
> return -1;
Here, it's easier to fix the leak by doing goto cleanup rather than an
early return.
> Index: libvirt/src/util/xml.c
>
> --- 107,114 ----
>
> virXMLError(VIR_ERR_INTERNAL_ERROR,
>
> _("\'%s\' value longer than %Zd bytes in
> virXPathStringLimit()"),
>
> xpath, maxlen);
>
> ! VIR_FREE(tmp);
>
> ! return NULL;
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).
> Index: libvirt/tools/virsh.c
>
> ===================================================================
>
> ***************
>
> *** 8697,8702 ****
>
> --- 8697,8703 ----
>
> {
'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.
>
> const char *dir;
>
> int found;
>
> + int dir_root = FALSE;
>
> ! if (!dir) {
>
> dir = "/";
>
> + dir_root = TRUE;
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110304/714e46a7/attachment-0001.sig>
More information about the libvir-list
mailing list