From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; The xenParseSxpr method sets def->nconsoles to 1 before allocating the def->consoles array. If the allocation fails due to OOM the cleanup code will thus crash accessing out of bounds. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/xenxs/xen_sxpr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 6209c68..462eac6 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -1439,9 +1439,9 @@ xenParseSxpr(const struct sexpr *root, def->parallels[def->nparallels++] = chr; } } else if (def->id != 0) { - def->nconsoles = 1; if (VIR_ALLOC_N(def->consoles, 1) < 0) goto error; + def->nconsoles = 1; /* Fake a paravirt console, since that's not in the sexpr */ if (!(def->consoles[0] = xenParseSxprChar("pty", tty))) goto error; -- 1.8.3.1

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; The methods for obtaining the Xen dom ID cannot distinguish between returning -1 due to an error and returning -1 due to the domain being shutoff. Change them to return the dom ID via an output parameter. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/xen/xen_driver.c | 3 ++- src/xen/xend_internal.c | 10 ++++++---- src/xenxs/xen_sxpr.c | 17 ++++++++++------- src/xenxs/xen_sxpr.h | 4 ++-- tests/sexpr2xmltest.c | 3 ++- 5 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 6cb4f4f..40b98ee 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1604,7 +1604,8 @@ xenUnifiedConnectDomainXMLFromNative(virConnectPtr conn, def = xenParseXM(conf, priv->xendConfigVersion, priv->caps); } else if (STREQ(format, XEN_CONFIG_FORMAT_SEXPR)) { - id = xenGetDomIdFromSxprString(config, priv->xendConfigVersion); + if (xenGetDomIdFromSxprString(config, priv->xendConfigVersion, &id) < 0) + goto cleanup; xenUnifiedLock(priv); tty = xenStoreDomainGetConsolePath(conn, id); vncport = xenStoreDomainGetVNCPort(conn, id); diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index f698c8d..dc57350 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1561,7 +1561,7 @@ xenDaemonDomainFetch(virConnectPtr conn, int domid, const char *name, { struct sexpr *root; xenUnifiedPrivatePtr priv = conn->privateData; - virDomainDefPtr def; + virDomainDefPtr def = NULL; int id; char * tty; int vncport; @@ -1573,7 +1573,8 @@ xenDaemonDomainFetch(virConnectPtr conn, int domid, const char *name, if (root == NULL) return NULL; - id = xenGetDomIdFromSxpr(root, priv->xendConfigVersion); + if (xenGetDomIdFromSxpr(root, priv->xendConfigVersion, &id) < 0) + goto cleanup; xenUnifiedLock(priv); if (sexpr_lookup(root, "domain/image/hvm")) tty = xenStoreDomainGetSerialConsolePath(conn, id); @@ -3224,7 +3225,7 @@ xenDaemonDomainBlockPeek(virConnectPtr conn, xenUnifiedPrivatePtr priv = conn->privateData; struct sexpr *root = NULL; int fd = -1, ret = -1; - virDomainDefPtr def; + virDomainDefPtr def = NULL; int id; char * tty; int vncport; @@ -3249,7 +3250,8 @@ xenDaemonDomainBlockPeek(virConnectPtr conn, return -1; } - id = xenGetDomIdFromSxpr(root, priv->xendConfigVersion); + if (xenGetDomIdFromSxpr(root, priv->xendConfigVersion, &id) < 0) + goto cleanup; xenUnifiedLock(priv); tty = xenStoreDomainGetConsolePath(conn, id); vncport = xenStoreDomainGetVNCPort(conn, id); diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 462eac6..bb8a335 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -40,30 +40,33 @@ #include "virstring.h" /* Get a domain id from a S-expression string */ -int xenGetDomIdFromSxprString(const char *sexpr, int xendConfigVersion) +int xenGetDomIdFromSxprString(const char *sexpr, int xendConfigVersion, int *id) { struct sexpr *root = string2sexpr(sexpr); + int ret; + + *id = -1; if (!root) return -1; - int id = xenGetDomIdFromSxpr(root, xendConfigVersion); + ret = xenGetDomIdFromSxpr(root, xendConfigVersion, id); sexpr_free(root); - return id; + return ret; } /* Get a domain id from a S-expression */ -int xenGetDomIdFromSxpr(const struct sexpr *root, int xendConfigVersion) +int xenGetDomIdFromSxpr(const struct sexpr *root, int xendConfigVersion, int *id) { - int id = -1; const char * tmp = sexpr_node(root, "domain/domid"); if (tmp == NULL && xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) { /* domid was mandatory */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("domain information incomplete, missing id")); + return -1; } else { - id = tmp ? sexpr_int(root, "domain/domid") : -1; + *id = tmp ? sexpr_int(root, "domain/domid") : -1; + return 0; } - return id; } /***************************************************************** diff --git a/src/xenxs/xen_sxpr.h b/src/xenxs/xen_sxpr.h index d7ce46a..f354a50 100644 --- a/src/xenxs/xen_sxpr.h +++ b/src/xenxs/xen_sxpr.h @@ -40,8 +40,8 @@ typedef enum { } xenConfigVersionEnum; /* helper functions to get the dom id from a sexpr */ -int xenGetDomIdFromSxprString(const char *sexpr, int xendConfigVersion); -int xenGetDomIdFromSxpr(const struct sexpr *root, int xendConfigVersion); +int xenGetDomIdFromSxprString(const char *sexpr, int xendConfigVersion, int *id); +int xenGetDomIdFromSxpr(const struct sexpr *root, int xendConfigVersion, int *id); virDomainDefPtr xenParseSxprString(const char *sexpr, int xendConfigVersion, char *tty, int vncport); diff --git a/tests/sexpr2xmltest.c b/tests/sexpr2xmltest.c index eafefda..b939319 100644 --- a/tests/sexpr2xmltest.c +++ b/tests/sexpr2xmltest.c @@ -50,7 +50,8 @@ testCompareFiles(const char *xml, const char *sexpr, int xendConfigVersion) if (virMutexInit(&priv.lock) < 0) goto fail; - id = xenGetDomIdFromSxprString(sexprData, xendConfigVersion); + if (xenGetDomIdFromSxprString(sexprData, xendConfigVersion, &id) < 0) + goto fail; xenUnifiedLock(&priv); tty = xenStoreDomainGetConsolePath(conn, id); vncport = xenStoreDomainGetVNCPort(conn, id); -- 1.8.3.1

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; The virDomainSnapshotDefParse method assigned to def->ndisks before allocating def->disks. Thus if an OOM occurred, the cleanup code would access out of bounds. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/conf/snapshot_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 45d6af4..207a8fe 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -303,9 +303,9 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) goto cleanup; if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS) { - def->ndisks = n; - if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) + if (n && VIR_ALLOC_N(def->disks, n) < 0) goto cleanup; + def->ndisks = n; for (i = 0; i < def->ndisks; i++) { if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0) goto cleanup; -- 1.8.3.1

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; The virStoragePoolDefParseSource method would set def->nhosts before allocating def->hosts. If the allocation failed due to OOM, the cleanup code would crash accessing out of bounds. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/conf/storage_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 002663f..975e662 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -594,11 +594,11 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, if ((n = virXPathNodeSet("./host", ctxt, &nodeset)) < 0) goto cleanup; - source->nhost = n; - if (source->nhost) { - if (VIR_ALLOC_N(source->hosts, source->nhost) < 0) + if (n) { + if (VIR_ALLOC_N(source->hosts, n) < 0) goto cleanup; + source->nhost = n; for (i = 0; i < source->nhost; i++) { name = virXMLPropString(nodeset[i], "name"); -- 1.8.3.1

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; The code parsing comments in config files called virConfAddEntry but did not check for failure. This caused the comment string to leak on OOM. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/util/virconf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virconf.c b/src/util/virconf.c index 9952f3f..e882d15 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -589,7 +589,10 @@ virConfParseComment(virConfParserCtxtPtr ctxt) while ((ctxt->cur < ctxt->end) && (!IS_EOL(CUR))) NEXT; if (VIR_STRNDUP(comm, base, ctxt->cur - base) < 0) return -1; - virConfAddEntry(ctxt->conf, NULL, NULL, comm); + if (virConfAddEntry(ctxt->conf, NULL, NULL, comm) == NULL) { + VIR_FREE(comm); + return -1; + } return 0; } -- 1.8.3.1

On 09/25/2013 08:51 AM, Daniel P. Berrange wrote: Indentation of \ is now off. ACK to 6-10 -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; If OOM or another error occurs in virJSONValueFromString the parser state object will be leaked. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/util/virjson.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virjson.c b/src/util/virjson.c index 8918bc7..2bb7324 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1014,6 +1014,7 @@ cleanup: for (i = 0; i < parser.nstate; i++) { VIR_FREE(parser.state[i].key); } + VIR_FREE(parser.state); } VIR_DEBUG("result=%p", parser.head); -- 1.8.3.1

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; For inexplicable reasons, the nwfilter XML parser is intentionally ignoring errors that arise during parsing. As well as meaning that users don't get any feedback on their XML mistakes, this will lead it to silently drop data in OOM conditions. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/conf/nwfilter_conf.c | 16 ++++++++-------- tests/nwfilterxml2xmltest.c | 14 ++++++-------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 00e74eb..3456b77 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2369,9 +2369,7 @@ virNWFilterRuleParse(xmlNodePtr node) if (virNWFilterRuleDetailsParse(cur, ret, virAttr[i].att) < 0) { - /* we ignore malformed rules - goto err_exit; - */ + goto err_exit; } break; } @@ -2573,11 +2571,13 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt) { if (VIR_ALLOC(entry) < 0) goto cleanup; - /* ignore malformed rule and include elements */ - if (xmlStrEqual(curr->name, BAD_CAST "rule")) - entry->rule = virNWFilterRuleParse(curr); - else if (xmlStrEqual(curr->name, BAD_CAST "filterref")) - entry->include = virNWFilterIncludeParse(curr); + if (xmlStrEqual(curr->name, BAD_CAST "rule")) { + if (!(entry->rule = virNWFilterRuleParse(curr))) + goto cleanup; + } else if (xmlStrEqual(curr->name, BAD_CAST "filterref")) { + if (!(entry->include = virNWFilterIncludeParse(curr))) + goto cleanup; + } if (entry->rule || entry->include) { if (VIR_REALLOC_N(ret->filterEntries, ret->nentries+1) < 0) { diff --git a/tests/nwfilterxml2xmltest.c b/tests/nwfilterxml2xmltest.c index 5476284..84e61da 100644 --- a/tests/nwfilterxml2xmltest.c +++ b/tests/nwfilterxml2xmltest.c @@ -36,15 +36,12 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml, virResetLastError(); - if (!(dev = virNWFilterDefParseString(NULL, inXmlData))) + if (!(dev = virNWFilterDefParseString(NULL, inXmlData))) { + if (expect_error) { + virResetLastError(); + goto done; + } goto fail; - - if (!!virGetLastError() != expect_error) - goto fail; - - if (expect_error) { - /* need to suppress the errors */ - virResetLastError(); } if (!(actual = virNWFilterDefFormat(dev))) @@ -55,6 +52,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml, goto fail; } + done: ret = 0; fail: -- 1.8.3.1

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Don't leak the path string in qemuMonitorCommonTestNew if an OOM occurs. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- tests/qemumonitortestutils.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 486d72f..763102c 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -793,6 +793,7 @@ qemuMonitorCommonTestNew(virDomainXMLOptionPtr xmlopt, src->type = VIR_DOMAIN_CHR_TYPE_UNIX; src->data.nix.path = (char *)path; src->data.nix.listen = false; + path = NULL; if (virNetSocketListen(test->server, 1) < 0) goto error; @@ -801,6 +802,7 @@ cleanup: return test; error: + VIR_FREE(path); VIR_FREE(tmpdir_template); qemuMonitorTestFree(test); test = NULL; -- 1.8.3.1

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; The virDomainChrSourceDef variable should be memset to 0, so that the cleanup block does not free uninitialized data on OOM. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- tests/qemumonitortestutils.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 9568476..bca3385 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -878,6 +878,8 @@ qemuMonitorTestNew(bool json, qemuMonitorTestPtr test = NULL; virDomainChrSourceDef src; + memset(&src, 0, sizeof(src)); + if (!(test = qemuMonitorCommonTestNew(xmlopt, vm, &src))) goto error; @@ -915,6 +917,8 @@ qemuMonitorTestNewAgent(virDomainXMLOptionPtr xmlopt) qemuMonitorTestPtr test = NULL; virDomainChrSourceDef src; + memset(&src, 0, sizeof(src)); + if (!(test = qemuMonitorCommonTestNew(xmlopt, NULL, &src))) goto error; -- 1.8.3.1

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; The virlockspacetest.c did not check for failure to create a lockspace, causing a crash on OOM Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- tests/virlockspacetest.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/tests/virlockspacetest.c b/tests/virlockspacetest.c index b659f61..1985a4a 100644 --- a/tests/virlockspacetest.c +++ b/tests/virlockspacetest.c @@ -44,7 +44,8 @@ static int testLockSpaceCreate(const void *args ATTRIBUTE_UNUSED) rmdir(LOCKSPACE_DIR); - lockspace = virLockSpaceNew(LOCKSPACE_DIR); + if (!(lockspace = virLockSpaceNew(LOCKSPACE_DIR))) + goto cleanup; if (!virFileIsDir(LOCKSPACE_DIR)) goto cleanup; @@ -65,7 +66,8 @@ static int testLockSpaceResourceLifecycle(const void *args ATTRIBUTE_UNUSED) rmdir(LOCKSPACE_DIR); - lockspace = virLockSpaceNew(LOCKSPACE_DIR); + if (!(lockspace = virLockSpaceNew(LOCKSPACE_DIR))) + goto cleanup; if (!virFileIsDir(LOCKSPACE_DIR)) goto cleanup; @@ -98,7 +100,8 @@ static int testLockSpaceResourceLockExcl(const void *args ATTRIBUTE_UNUSED) rmdir(LOCKSPACE_DIR); - lockspace = virLockSpaceNew(LOCKSPACE_DIR); + if (!(lockspace = virLockSpaceNew(LOCKSPACE_DIR))) + goto cleanup; if (!virFileIsDir(LOCKSPACE_DIR)) goto cleanup; @@ -143,7 +146,8 @@ static int testLockSpaceResourceLockExclAuto(const void *args ATTRIBUTE_UNUSED) rmdir(LOCKSPACE_DIR); - lockspace = virLockSpaceNew(LOCKSPACE_DIR); + if (!(lockspace = virLockSpaceNew(LOCKSPACE_DIR))) + goto cleanup; if (!virFileIsDir(LOCKSPACE_DIR)) goto cleanup; @@ -180,7 +184,8 @@ static int testLockSpaceResourceLockShr(const void *args ATTRIBUTE_UNUSED) rmdir(LOCKSPACE_DIR); - lockspace = virLockSpaceNew(LOCKSPACE_DIR); + if (!(lockspace = virLockSpaceNew(LOCKSPACE_DIR))) + goto cleanup; if (!virFileIsDir(LOCKSPACE_DIR)) goto cleanup; @@ -233,7 +238,8 @@ static int testLockSpaceResourceLockShrAuto(const void *args ATTRIBUTE_UNUSED) rmdir(LOCKSPACE_DIR); - lockspace = virLockSpaceNew(LOCKSPACE_DIR); + if (!(lockspace = virLockSpaceNew(LOCKSPACE_DIR))) + goto cleanup; if (!virFileIsDir(LOCKSPACE_DIR)) goto cleanup; @@ -292,7 +298,8 @@ static int testLockSpaceResourceLockPath(const void *args ATTRIBUTE_UNUSED) rmdir(LOCKSPACE_DIR); - lockspace = virLockSpaceNew(NULL); + if (!(lockspace = virLockSpaceNew(NULL))) + goto cleanup; if (mkdir(LOCKSPACE_DIR, 0700) < 0) goto cleanup; -- 1.8.3.1

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; The virportallocatortest did not check if the object allocation failed in all cases. This lead to a crash on OOM in the testsuite Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- tests/virportallocatortest.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c index 1a0cdfa..615fa15 100644 --- a/tests/virportallocatortest.c +++ b/tests/virportallocatortest.c @@ -67,6 +67,9 @@ static int testAllocAll(const void *args ATTRIBUTE_UNUSED) int ret = -1; unsigned short p1, p2, p3, p4, p5, p6, p7; + if (!alloc) + return -1; + if (virPortAllocatorAcquire(alloc, &p1) < 0) goto cleanup; if (p1 != 5901) { @@ -137,6 +140,9 @@ static int testAllocReuse(const void *args ATTRIBUTE_UNUSED) int ret = -1; unsigned short p1, p2, p3, p4; + if (!alloc) + return -1; + if (virPortAllocatorAcquire(alloc, &p1) < 0) goto cleanup; if (p1 != 5901) { -- 1.8.3.1

From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; If an error occurs in virnetmessagetest it was possible it would free uninitialized data. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- tests/virnetmessagetest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/virnetmessagetest.c b/tests/virnetmessagetest.c index 3c9bead..eabc609 100644 --- a/tests/virnetmessagetest.c +++ b/tests/virnetmessagetest.c @@ -327,6 +327,8 @@ static int testMessagePayloadDecode(const void *args ATTRIBUTE_UNUSED) }; int ret = -1; + memset(&err, 0, sizeof(err)); + if (!msg) return -1; @@ -334,7 +336,6 @@ static int testMessagePayloadDecode(const void *args ATTRIBUTE_UNUSED) if (VIR_ALLOC_N(msg->buffer, msg->bufferLength) < 0) goto cleanup; memcpy(msg->buffer, input_buffer, msg->bufferLength); - memset(&err, 0, sizeof(err)); if (virNetMessageDecodeLength(msg) < 0) { VIR_DEBUG("Failed to decode message header"); -- 1.8.3.1