On Wed, May 13, 2015 at 12:32:20 -0400, John Ferlan wrote:

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@redhat.com> --- 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;

This doesn't seem to be correct if either output or error is NULL and the other one is non-NULL. I'm too lazy to check if it's possible or not, but I think we should change this code in the following way and be safe: if (ret < 0) { if ((output && strstr(output, "unrecognised disk label")) || (error && strstr(error, "unrecognised disk label"))) { ret = 1; Jirka

On 05/14/2015 08:35 AM, John Ferlan wrote:

On 05/13/2015 02:43 PM, Jiri Denemark wrote:

...

...

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@redhat.com> --- 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; This doesn't seem to be correct if either output or error is NULL and

On Wed, May 13, 2015 at 12:32:20 -0400, John Ferlan wrote: the other one is non-NULL. I'm too lazy to check if it's possible or not, but I think we should change this code in the following way and be safe:

if (ret < 0) { if ((output && strstr(output, "unrecognised disk label")) || (error && strstr(error, "unrecognised disk label"))) { ret = 1;

Jirka

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...

In case there's any question - ACK to Jirka's version of the patch.

On 05/13/2015 12:32 PM, John Ferlan wrote:

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@redhat.com> --- 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);

Theoretically domlist->objs could be set to NULL by some other thread after the sa_asser and before the virObjectLock, but since these are false positives, the purpose is to shut up coverity, not to actually check for a non-NULL pointer. So ACK.

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;

On 05/13/2015 12:32 PM, John Ferlan wrote:

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.

Agreed (I hand traced all the possible combinations to be sure), and ACK.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 {

On 05/18/2015 09:21 AM, John Ferlan wrote:

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@redhat.com> --- 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;

Seemed a bit strange at first (since the n == 0 case now continues instead of returning immediately), but in the end it makes sense - if n is 0, you end up allocating keywrap, never going through the loop, then freeing keywrap and returning 0, to the result is the same as before. ACK.

if (VIR_ALLOC(def->keywrap) < 0) goto cleanup;

On 05/18/2015 09:21 AM, John Ferlan wrote:

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.

ACK.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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);