Coverity points out it's possible for one of the virCommand{Output|Error}* API's to have not allocated 'output' and/or 'error' in which case the strstr comparison will cause a NULL deref Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_disk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 6394dac..5c2c49a 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -412,7 +412,7 @@ virStorageBackendDiskFindLabel(const char* device) /* if parted succeeds we have a valid partition table */ ret = virCommandRun(cmd, NULL); - if (ret < 0) { + if (ret < 0 && output && error) { if (strstr(output, "unrecognised disk label") || strstr(error, "unrecognised disk label")) { ret = 1; -- 2.1.0

On 05/13/2015 02:43 PM, Jiri Denemark wrote: Sure - seems reasonable, although I suspect if allocation of memory for the output buffer fails, then allocation of memory for the error buffer will fail too, but just in case it succeeds... John

Even though it's been pointed out they are false positives: http://www.redhat.com/archives/libvir-list/2015-May/msg00301.html and http://www.redhat.com/archives/libvir-list/2015-May/msg00302.html these still show up as Coverity issues. In order to silence Coverity add an 'sa_assert' prior to check failure. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index add857c..5b69b5a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23078,6 +23078,7 @@ virDomainObjListCollect(virDomainObjListPtr domlist, { struct virDomainListData data = { NULL, 0 }; + sa_assert(domlist->objs); virObjectLock(domlist); if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) { virObjectUnlock(domlist); @@ -23141,6 +23142,7 @@ virDomainObjListConvert(virDomainObjListPtr domlist, } virObjectUnlock(domlist); + sa_assert(*vms); virDomainObjListFilter(vms, nvms, conn, filter, flags); return 0; -- 2.1.0

To silence Coverity just add a 'p &&' in front of the check in networkFindUnusedBridgeName after the strchr() call. Even though we know it's not possible to have strchr return NULL since the only way into the function is if there is a '%' in def->bridge or it's NULL. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/network/bridge_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 4b53475..f438c0b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2780,7 +2780,7 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets, if (def->bridge && (p = strchr(def->bridge, '%')) == strrchr(def->bridge, '%') && - p[1] == 'd') + p && p[1] == 'd') templ = def->bridge; do { -- 2.1.0

Commit id '73eda710' added virDomainKeyWrapDefParseXML which uses virXPathNodeSet, but does not handle a -1 return thus causing a possible loop condition exit problem later when the return value is used. Change the logic to return the value from virXPathNodeSet if <= 0 Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e2b1194..a97e640 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -940,8 +940,8 @@ virDomainKeyWrapDefParseXML(virDomainDefPtr def, xmlXPathContextPtr ctxt) xmlNodePtr *nodes = NULL; int n; - if (!(n = virXPathNodeSet("./keywrap/cipher", ctxt, &nodes))) - return 0; + if ((n = virXPathNodeSet("./keywrap/cipher", ctxt, &nodes)) < 0) + return n; if (VIR_ALLOC(def->keywrap) < 0) goto cleanup; -- 2.1.0

Recent changes to the -M/--machine processing code in qemuParseCommandLine caused Coverity to determine there was a possible resource leak with how the 'list' is managed. Rather than try to add virStringFreeList calls everywhere - just promote list to the top of the variables and free it within the error processing code. Also required a couple of other tweaks in order to avoid double free's. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3ccd35d..411b7a4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -12382,6 +12382,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, size_t i; bool nographics = false; bool fullscreen = false; + char **list = NULL; char *path; size_t nnics = 0; const char **nics = NULL; @@ -12849,7 +12850,6 @@ qemuParseCommandLine(virCapsPtr qemuCaps, VIR_FREE(def->name); } else if (STREQ(arg, "-M") || STREQ(arg, "-machine")) { - char **list; char *param; size_t j = 0; @@ -12864,10 +12864,8 @@ qemuParseCommandLine(virCapsPtr qemuCaps, if (STRPREFIX(param, "type=")) param += strlen("type="); if (!strchr(param, '=')) { - if (VIR_STRDUP(def->os.machine, param) < 0) { - virStringFreeList(list); + if (VIR_STRDUP(def->os.machine, param) < 0) goto error; - } j++; } @@ -12912,6 +12910,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, } } virStringFreeList(list); + list = NULL; } else if (STREQ(arg, "-serial")) { WANT_VALUE(); if (STRNEQ(val, "none")) { @@ -13385,6 +13384,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, virDomainDiskDefFree(disk); qemuDomainCmdlineDefFree(cmd); virDomainDefFree(def); + virStringFreeList(list); VIR_FREE(nics); if (monConfig) { virDomainChrSourceDefFree(*monConfig); -- 2.1.0