[libvirt] [PATCH 00/23] Fix yet more OOM errors
From: "Daniel P. Berrange" <berrange@redhat.com> A nice mix of leaks and crashes and ignoring errors. Daniel P. Berrange (23): Fix crash on OOM in xenParseSxpr Fix handling of OOM when getting Xen dom ID Don't clobber return value in virInterfaceDefParseProtoIPv6 Fix crash on OOM in virDomainSnapshotDefParse Don't clobber 'ret' in LXC XML test case Fix double free of hostdev on OOM in xenParseSxprPCI Fix crash on OOM parsing storage pool XML Add missing check for OOM with virVMXEscapeHexPipe Fix leak of comment string if virConfAddEntry fails on OOM Don't print all test suite errors to stderr in vmx2xmltest Fix leak of iterators in virDBusMessageIterEncode Fix double-free in virJSONParserHandleStartMap on OOM Fix leak of parser state in virJSONValueFromString Fix leak in virLockSpaceResourceFree Don't ignore errors parsing nwfilter rules Fix leak on OOM in qemuMonitorCommonTestNew Avoid double free in qemuMonitorCommonTestInit on OOM Avoid uninitialized data in qemuMonitorTestNew Avoid crash on OOM in virbuftest Avoid crash on OOM in virlockspacetest Avoid crash on OOM in virportallocatortest Avoid crash on OOM in virnetmessagetest Avoid use of uninitialized data in virnetmessagetest src/conf/interface_conf.c | 16 ++++++-------- src/conf/nwfilter_conf.c | 16 +++++++------- src/conf/snapshot_conf.c | 4 ++-- src/conf/storage_conf.c | 6 +++--- src/util/virconf.c | 5 ++++- src/util/virdbus.c | 24 ++++++++++++++++++--- src/util/virjson.c | 2 +- src/util/virlockspace.c | 1 + src/vmx/vmx.c | 3 ++- src/xen/xen_driver.c | 3 ++- src/xen/xend_internal.c | 10 +++++---- src/xenxs/xen_sxpr.c | 23 ++++++++++---------- src/xenxs/xen_sxpr.h | 4 ++-- tests/lxcxml2xmltest.c | 13 +++++++---- tests/nwfilterxml2xmltest.c | 14 ++++++------ tests/qemumonitortestutils.c | 7 +++++- tests/sexpr2xmltest.c | 3 ++- tests/virbuftest.c | 14 ++++++++++++ tests/virlockspacetest.c | 21 ++++++++++++------ tests/virnetmessagetest.c | 9 +++++++- tests/virportallocatortest.c | 6 ++++++ tests/vmx2xmltest.c | 51 +++++++++++++++++--------------------------- 22 files changed, 154 insertions(+), 101 deletions(-) -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> 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@redhat.com> --- 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
On 09/25/2013 10:50 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
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@redhat.com> --- 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;
ACK
From: "Daniel P. Berrange" <berrange@redhat.com> 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@redhat.com> --- 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@redhat.com> Several places in virInterfaceDefParseProtoIPv6 clobber the default 'ret' return value. So when jumping to cleanup on error, 'ret' may mistakenly be set to 0 instead of -1. This caused failure to report OOM errors, meaning data was silently lost during parsing. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/conf/interface_conf.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 19d0327..79aab6e 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -309,9 +309,8 @@ virInterfaceDefParseProtoIPv4(virInterfaceProtocolDefPtr def, dhcp = virXPathNode("./dhcp", ctxt); if (dhcp != NULL) { - ret = virInterfaceDefParseDhcp(def, dhcp, ctxt); - if (ret != 0) - return ret; + if (virInterfaceDefParseDhcp(def, dhcp, ctxt) < 0) + return -1; } nIpNodes = virXPathNodeSet("./ip", ctxt, &ipNodes); @@ -332,8 +331,7 @@ virInterfaceDefParseProtoIPv4(virInterfaceProtocolDefPtr def, goto error; ctxt->node = ipNodes[i]; - ret = virInterfaceDefParseIp(ip, ctxt); - if (ret != 0) { + if (virInterfaceDefParseIp(ip, ctxt) < 0) { virInterfaceIpDefFree(ip); goto error; } @@ -365,9 +363,8 @@ virInterfaceDefParseProtoIPv6(virInterfaceProtocolDefPtr def, dhcp = virXPathNode("./dhcp", ctxt); if (dhcp != NULL) { - ret = virInterfaceDefParseDhcp(def, dhcp, ctxt); - if (ret != 0) - return ret; + if (virInterfaceDefParseDhcp(def, dhcp, ctxt) < 0) + return -1; } nIpNodes = virXPathNodeSet("./ip", ctxt, &ipNodes); @@ -388,8 +385,7 @@ virInterfaceDefParseProtoIPv6(virInterfaceProtocolDefPtr def, goto error; ctxt->node = ipNodes[i]; - ret = virInterfaceDefParseIp(ip, ctxt); - if (ret != 0) { + if (virInterfaceDefParseIp(ip, ctxt) < 0) { virInterfaceIpDefFree(ip); goto error; } -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> 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@redhat.com> --- 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@redhat.com> The testCompareXMLToXMLHelper method clobbered the 'ret' variable in several places leading to a failure to report OOM errors from the test suite. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/lxcxml2xmltest.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/lxcxml2xmltest.c b/tests/lxcxml2xmltest.c index ca05d29..aeb3940 100644 --- a/tests/lxcxml2xmltest.c +++ b/tests/lxcxml2xmltest.c @@ -79,18 +79,23 @@ testCompareXMLToXMLHelper(const void *data) goto cleanup; if (info->different) { - ret = testCompareXMLToXMLFiles(xml_in, xml_out, false); + if (testCompareXMLToXMLFiles(xml_in, xml_out, false) < 0) + goto cleanup; } else { - ret = testCompareXMLToXMLFiles(xml_in, xml_in, false); + if (testCompareXMLToXMLFiles(xml_in, xml_in, false) < 0) + goto cleanup; } if (!info->inactive_only) { if (info->different) { - ret = testCompareXMLToXMLFiles(xml_in, xml_out, true); + if (testCompareXMLToXMLFiles(xml_in, xml_out, true) < 0) + goto cleanup; } else { - ret = testCompareXMLToXMLFiles(xml_in, xml_in, true); + if (testCompareXMLToXMLFiles(xml_in, xml_in, true) < 0) + goto cleanup; } } + ret = 0; cleanup: VIR_FREE(xml_in); VIR_FREE(xml_out); -- 1.8.3.1
On 09/25/2013 08:51 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The testCompareXMLToXMLHelper method clobbered the 'ret' variable in several places leading to a failure to report OOM errors from the test suite.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/lxcxml2xmltest.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
ACK 1-5. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> If xenParseSxprPCI failed to expand the def->hostdevs array due to OOM, it would free the hostdev instance twice. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/xenxs/xen_sxpr.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index bb8a335..3cbe958 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -1055,10 +1055,8 @@ xenParseSxprPCI(virDomainDefPtr def, dev->source.subsys.u.pci.addr.slot = slotID; dev->source.subsys.u.pci.addr.function = funcID; - if (VIR_REALLOC_N(def->hostdevs, def->nhostdevs+1) < 0) { - virDomainHostdevDefFree(dev); + if (VIR_REALLOC_N(def->hostdevs, def->nhostdevs+1) < 0) goto error; - } def->hostdevs[def->nhostdevs++] = dev; } -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> 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@redhat.com> --- 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@redhat.com> The virVMXFormatConfig called virVMXEscapeHexPipe but forgot to check for OOM. This caused data to silently be lost. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/vmx/vmx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 5c2c794..38b7cc0 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -3096,7 +3096,8 @@ virVMXFormatConfig(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virDomainDe /* def:description -> vmx:annotation */ if (def->description != NULL) { - annotation = virVMXEscapeHexPipe(def->description); + if (!(annotation = virVMXEscapeHexPipe(def->description))) + goto cleanup; virBufferAsprintf(&buffer, "annotation = \"%s\"\n", annotation); } -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> 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@redhat.com> --- 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
From: "Daniel P. Berrange" <berrange@redhat.com> The vmx2xmltest test would print all errors to stderr, which is not helpful when running OOM tests, and differs from the behaviour of other tests. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/vmx2xmltest.c | 51 +++++++++++++++++++-------------------------------- 1 file changed, 19 insertions(+), 32 deletions(-) diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index 00bf21d..86272ce 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -73,51 +73,38 @@ testCapsInit(void) static int testCompareFiles(const char *vmx, const char *xml) { - int result = -1; + int ret = -1; char *vmxData = NULL; char *xmlData = NULL; char *formatted = NULL; virDomainDefPtr def = NULL; - virErrorPtr err = NULL; - if (virtTestLoadFile(vmx, &vmxData) < 0) { - goto failure; - } - - if (virtTestLoadFile(xml, &xmlData) < 0) { - goto failure; - } - - def = virVMXParseConfig(&ctx, xmlopt, vmxData); + if (virtTestLoadFile(vmx, &vmxData) < 0) + goto cleanup; - if (def == NULL) { - err = virGetLastError(); - fprintf(stderr, "ERROR: %s\n", err != NULL ? err->message : "<unknown>"); - goto failure; - } + if (virtTestLoadFile(xml, &xmlData) < 0) + goto cleanup; - formatted = virDomainDefFormat(def, VIR_DOMAIN_XML_SECURE); + if (!(def = virVMXParseConfig(&ctx, xmlopt, vmxData))) + goto cleanup; - if (formatted == NULL) { - err = virGetLastError(); - fprintf(stderr, "ERROR: %s\n", err != NULL ? err->message : "<unknown>"); - goto failure; - } + if (!(formatted = virDomainDefFormat(def, VIR_DOMAIN_XML_SECURE))) + goto cleanup; if (STRNEQ(xmlData, formatted)) { virtTestDifference(stderr, xmlData, formatted); - goto failure; + goto cleanup; } - result = 0; + ret = 0; - failure: + cleanup: VIR_FREE(vmxData); VIR_FREE(xmlData); VIR_FREE(formatted); virDomainDefFree(def); - return result; + return ret; } struct testInfo { @@ -128,7 +115,7 @@ struct testInfo { static int testCompareHelper(const void *data) { - int result = -1; + int ret = -1; const struct testInfo *info = data; char *vmx = NULL; char *xml = NULL; @@ -140,13 +127,13 @@ testCompareHelper(const void *data) goto cleanup; } - result = testCompareFiles(vmx, xml); + ret = testCompareFiles(vmx, xml); cleanup: VIR_FREE(vmx); VIR_FREE(xml); - return result; + return ret; } static char * @@ -195,7 +182,7 @@ testParseVMXFileName(const char *fileName, void *opaque ATTRIBUTE_UNUSED) static int mymain(void) { - int result = 0; + int ret = 0; # define DO_TEST(_in, _out) \ do { \ @@ -203,7 +190,7 @@ mymain(void) virResetLastError(); \ if (virtTestRun("VMware VMX-2-XML "_in" -> "_out, 1, \ testCompareHelper, &info) < 0) { \ - result = -1; \ + ret = -1; \ } \ } while (0) @@ -299,7 +286,7 @@ mymain(void) virObjectUnref(caps); virObjectUnref(xmlopt); - return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE; + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } VIRT_TEST_MAIN(mymain) -- 1.8.3.1
On 09/25/2013 08:51 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The vmx2xmltest test would print all errors to stderr, which is not helpful when running OOM tests, and differs from the behaviour of other tests.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/vmx2xmltest.c | 51 +++++++++++++++++++-------------------------------- 1 file changed, 19 insertions(+), 32 deletions(-)
@@ -203,7 +190,7 @@ mymain(void) virResetLastError(); \ if (virtTestRun("VMware VMX-2-XML "_in" -> "_out, 1, \ testCompareHelper, &info) < 0) { \ - result = -1; \ + ret = -1; \ } \
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@redhat.com> If virDBusMessageIterEncode hits an OOM condition it often leaks the memory associated with the dbus iterator object Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virdbus.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index a2c4b4e..60ff574 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -601,8 +601,10 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, goto cleanup; if (virDBusTypeStackPush(&stack, &nstack, iter, types, - nstruct, narray) < 0) + nstruct, narray) < 0) { + VIR_FREE(newiter); goto cleanup; + } VIR_FREE(contsig); iter = newiter; newiter = NULL; @@ -625,8 +627,10 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, goto cleanup; if (virDBusTypeStackPush(&stack, &nstack, iter, types, - nstruct, narray) < 0) + nstruct, narray) < 0) { + VIR_FREE(newiter); goto cleanup; + } iter = newiter; newiter = NULL; types = vsig; @@ -657,8 +661,10 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, if (virDBusTypeStackPush(&stack, &nstack, iter, types, - nstruct, narray) < 0) + nstruct, narray) < 0) { + VIR_FREE(newiter); goto cleanup; + } VIR_FREE(contsig); iter = newiter; newiter = NULL; @@ -678,6 +684,18 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, ret = 0; cleanup: + while (nstack > 0) { + DBusMessageIter *thisiter = iter; + VIR_DEBUG("Popping iter=%p", iter); + if (virDBusTypeStackPop(&stack, &nstack, &iter, + &types, &nstruct, &narray) < 0) + goto cleanup; + VIR_DEBUG("Popped iter=%p", iter); + + if (thisiter != rootiter) + VIR_FREE(thisiter); + } + virDBusTypeStackFree(&stack, &nstack); VIR_FREE(contsig); VIR_FREE(newiter); -- 1.8.3.1
On 09/25/2013 08:51 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If virDBusMessageIterEncode hits an OOM condition it often leaks the memory associated with the dbus iterator object
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
cleanup: + while (nstack > 0) { + DBusMessageIter *thisiter = iter; + VIR_DEBUG("Popping iter=%p", iter); + if (virDBusTypeStackPop(&stack, &nstack, &iter, + &types, &nstruct, &narray) < 0) + goto cleanup;
I'm worried that this will infloop. What guarantee do we have that nstack reduces when virDBusTypeStackPop fails? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Sep 25, 2013 at 10:45:00AM -0600, Eric Blake wrote:
On 09/25/2013 08:51 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If virDBusMessageIterEncode hits an OOM condition it often leaks the memory associated with the dbus iterator object
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
cleanup: + while (nstack > 0) { + DBusMessageIter *thisiter = iter; + VIR_DEBUG("Popping iter=%p", iter); + if (virDBusTypeStackPop(&stack, &nstack, &iter, + &types, &nstruct, &narray) < 0) + goto cleanup;
I'm worried that this will infloop. What guarantee do we have that nstack reduces when virDBusTypeStackPop fails?
It can only return -1, if nstack == 0, so I think we're fine there static int virDBusTypeStackPop(virDBusTypeStack **stack, size_t *nstack, DBusMessageIter **iter, const char **types, size_t *nstruct, size_t *narray) { if (*nstack == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("DBus type stack is empty")); return -1; } *iter = (*stack)[(*nstack) - 1].iter; *types = (*stack)[(*nstack) - 1].types; *nstruct = (*stack)[(*nstack) - 1].nstruct; *narray = (*stack)[(*nstack) - 1].narray; VIR_DEBUG("Popped '%s'", *types); VIR_SHRINK_N(*stack, *nstack, 1); return 0; } Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/25/2013 10:54 AM, Daniel P. Berrange wrote:
On Wed, Sep 25, 2013 at 10:45:00AM -0600, Eric Blake wrote:
On 09/25/2013 08:51 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If virDBusMessageIterEncode hits an OOM condition it often leaks the memory associated with the dbus iterator object
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
cleanup: + while (nstack > 0) { + DBusMessageIter *thisiter = iter; + VIR_DEBUG("Popping iter=%p", iter); + if (virDBusTypeStackPop(&stack, &nstack, &iter, + &types, &nstruct, &narray) < 0) + goto cleanup;
I'm worried that this will infloop. What guarantee do we have that nstack reduces when virDBusTypeStackPop fails?
It can only return -1, if nstack == 0, so I think we're fine there
But you already checked that nstack was > 0. So write it: ignore_value(virDBusTypeStackPop(...)); rather than the confusing dead-code backwards goto. ACK with that change. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> If OOM occurs in virJSONParserHandleStartMap it will free a variable that is owned by another object. This leads to a later double-free. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virjson.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/util/virjson.c b/src/util/virjson.c index e93def7..8918bc7 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -862,7 +862,6 @@ static int virJSONParserHandleStartMap(void *ctx) if (VIR_REALLOC_N(parser->state, parser->nstate + 1) < 0) { - virJSONValueFree(value); return 0; } -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> If OOM or another error occurs in virJSONValueFromString the parser state object will be leaked. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- 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@redhat.com> Normally a lockspace resource is not freed while there are active owners. During initial resource creation though, an OOM error will trigger this scenario. virLockSpaceResourceFree was not freeing the 'owners' field in this case. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virlockspace.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index afb1abb..cab7775 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -102,6 +102,7 @@ static void virLockSpaceResourceFree(virLockSpaceResourcePtr res) } } + VIR_FREE(res->owners); VIR_FORCE_CLOSE(res->fd); VIR_FREE(res->path); VIR_FREE(res->name); -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> 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@redhat.com> --- 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
On 09/25/2013 08:51 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
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@redhat.com> --- src/conf/nwfilter_conf.c | 16 ++++++++-------- tests/nwfilterxml2xmltest.c | 14 ++++++-------- 2 files changed, 14 insertions(+), 16 deletions(-)
ACK 12-15. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Don't leak the path string in qemuMonitorCommonTestNew if an OOM occurs. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- 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@redhat.com> The qemuMonitorCommonTestInit method did not allocate the test object, so it should not free it upon failure. Doing so causes a double free with the caller. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/qemumonitortestutils.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 763102c..9568476 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -849,7 +849,6 @@ qemuMonitorCommonTestInit(qemuMonitorTestPtr test) return 0; error: - qemuMonitorTestFree(test); return -1; } -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> 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@redhat.com> --- 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@redhat.com> The virbuftest code did not check virBufferError before accessing the buffer contents, resulting in a crash on OOM conditions. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/virbuftest.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/virbuftest.c b/tests/virbuftest.c index febe6e4..a6dcae6 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -108,6 +108,10 @@ static int testBufAutoIndent(const void *data ATTRIBUTE_UNUSED) } virBufferAdjustIndent(buf, 2); virBufferAddLit(buf, "1"); + if (virBufferError(buf)) { + TEST_ERROR("Buffer had error"); + return -1; + } if (STRNEQ(virBufferCurrentContent(buf), " 1")) { TEST_ERROR("Wrong content"); ret = -1; @@ -134,6 +138,11 @@ static int testBufAutoIndent(const void *data ATTRIBUTE_UNUSED) virBufferEscapeShell(buf, " 11"); virBufferAddChar(buf, '\n'); + if (virBufferError(buf)) { + TEST_ERROR("Buffer had error"); + return -1; + } + result = virBufferContentAndReset(buf); if (!result || STRNEQ(result, expected)) { virtTestDifference(stderr, expected, result); @@ -166,6 +175,11 @@ static int testBufTrim(const void *data ATTRIBUTE_UNUSED) virBufferTrim(buf, "b,,", 1); virBufferTrim(buf, ",", -1); + if (virBufferError(buf)) { + TEST_ERROR("Buffer had error"); + return -1; + } + result = virBufferContentAndReset(buf); if (!result || STRNEQ(result, expected)) { virtTestDifference(stderr, expected, result); -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> The virlockspacetest.c did not check for failure to create a lockspace, causing a crash on OOM Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- 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
On 09/25/2013 08:51 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The virlockspacetest.c did not check for failure to create a lockspace, causing a crash on OOM
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/virlockspacetest.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
ACK 16-20 -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> 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@redhat.com> --- 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@redhat.com> The virnetmessagetest code did not check for failure to allocate the message object. This lead to a crash on OOM in the test suite. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/virnetmessagetest.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/virnetmessagetest.c b/tests/virnetmessagetest.c index 1aa4c25..3c9bead 100644 --- a/tests/virnetmessagetest.c +++ b/tests/virnetmessagetest.c @@ -327,6 +327,9 @@ static int testMessagePayloadDecode(const void *args ATTRIBUTE_UNUSED) }; int ret = -1; + if (!msg) + return -1; + msg->bufferLength = 4; if (VIR_ALLOC_N(msg->buffer, msg->bufferLength) < 0) goto cleanup; @@ -476,6 +479,9 @@ static int testMessagePayloadStreamEncode(const void *args ATTRIBUTE_UNUSED) }; int ret = -1; + if (!msg) + return -1; + msg->header.prog = 0x11223344; msg->header.vers = 0x01; msg->header.proc = 0x666; -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> If an error occurs in virnetmessagetest it was possible it would free uninitialized data. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- 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
On 09/25/2013 08:51 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
If an error occurs in virnetmessagetest it was possible it would free uninitialized data.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/virnetmessagetest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK 21-23 Of the series, 11 was the only patch I question. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump