Since 98b9acf5aa02551dd37d0209339aba2e22e4004a This ends up being a false positive for two reasons... expected to be already allocated and thus is passed by value; whereas, the call into remoteDomainGetJobStats() 'params' is passed by reference. Thus if the VIR_ALLOC is done there is no way for it to be leaked for callers that passed by value. path that handles 'nparams == 0 && params == NULL' on entry. Thus all other callers have guaranteed that 'params' is non NULL. Of course Coverity isn't wise enough to pick up on this, but nonetheless is does point out something "future callers" for which future callers need to be aware. Even though it is a false positive, it's probably a good idea to at least make some sort of check (and to "trick" Coverity into believing we know what we're doing). The easiest/cheapest way was to check the input 'limit' value. For the remoteDomainGetJobStats() it is passed as 0 indicating (perhaps) that the caller has done the limits length checking already and that its caller can handle allocating something that can be passed back to the caller. Doing this means an adjustment to remoteConnectGetAllDomainStats() in order to do the prerequisite maximum checking per set of stats returned and passing 0 to remoteDeserializeTypedParameters() in order to allocate memory into &elem->params. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/remote/remote_driver.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8221683..1594c89 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1761,6 +1761,18 @@ remoteDeserializeTypedParameters(remote_typed_param *ret_params_val, goto cleanup; } } else { + /* For callers that can return this allocated buffer back to their + * caller, pass a 0 in the 'limit' field indicating that the + * ret_params_len MAX checking has already occurred *and* that + * the caller has passed 'params' by reference; otherwise, a + * caller that receives the 'params' by value will potentially + * leak memory we're allocating here + */ + if (limit != 0) { + virReportError(VIR_ERR_RPC, "%s", + _("invalid call - params is NULL on input")); + goto cleanup; + } if (VIR_ALLOC_N(*params, ret_params_len) < 0) goto cleanup; } @@ -7771,6 +7783,12 @@ remoteConnectGetAllDomainStats(virConnectPtr conn, for (i = 0; i < ret.retStats.retStats_len; i++) { remote_domain_stats_record *rec = ret.retStats.retStats_val + i; + if (rec->params.params_len > REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX) { + virReportError(VIR_ERR_RPC, "%s", + _("returned number of parameters exceeds limit")); + goto cleanup; + } + if (VIR_ALLOC(elem) < 0) goto cleanup; @@ -7779,7 +7797,7 @@ remoteConnectGetAllDomainStats(virConnectPtr conn, if (remoteDeserializeTypedParameters(rec->params.params_val, rec->params.params_len, - REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX, + 0, &elem->params, &elem->nparams)) goto cleanup; -- 1.9.3

Coverity notes that after we VIR_ALLOC_N(params, nparams) a failed call to virDomainGetCPUStats could result in nparams being set to -1. In that case, the subsequent virTypedParamsFree in cleanup will pass -1 which isn't good. Use the returned value as the number of stats to display in the loop as it will be the value reported from the hypervisor and may be less than nparams which is OK Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tools/virsh-domain.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cc1e554..e951b1b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6734,7 +6734,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; virTypedParameterPtr params = NULL; - int pos, max_id, cpu = 0, show_count = -1, nparams = 0; + int pos, max_id, cpu = 0, show_count = -1, nparams = 0, stats_per_cpu; size_t i, j; bool show_total = false, show_per_cpu = false; unsigned int flags = 0; @@ -6853,11 +6853,12 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) goto cleanup; /* passing start_cpu == -1 gives us domain's total status */ - if ((nparams = virDomainGetCPUStats(dom, params, nparams, -1, 1, flags)) < 0) + if ((stats_per_cpu = virDomainGetCPUStats(dom, params, nparams, + -1, 1, flags)) < 0) goto failed_stats; vshPrint(ctl, _("Total:\n")); - for (i = 0; i < nparams; i++) { + for (i = 0; i < stats_per_cpu; i++) { vshPrint(ctl, "\t%-12s ", params[i].field); if ((STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_CPUTIME) || STREQ(params[i].field, VIR_DOMAIN_CPU_STATS_USERTIME) || -- 1.9.3

With eblake's help - adjust the checks for stdinfd/stdoutfd to ensure the values are within the range we expect; otherwise the dup2()'s and subsequent VIR_CLOSE() calls cause Coverity to believe there's a resource leak. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- daemon/libvirtd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 0503cd0..05ee50e 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -163,9 +163,9 @@ static int daemonForkIntoBackground(const char *argv0) VIR_FORCE_CLOSE(statuspipe[0]); - if ((stdinfd = open("/dev/null", O_RDONLY)) < 0) + if ((stdinfd = open("/dev/null", O_RDONLY)) <= STDERR_FILENO) goto cleanup; - if ((stdoutfd = open("/dev/null", O_WRONLY)) < 0) + if ((stdoutfd = open("/dev/null", O_WRONLY)) <= STDERR_FILENO) goto cleanup; if (dup2(stdinfd, STDIN_FILENO) != STDIN_FILENO) goto cleanup; @@ -173,9 +173,9 @@ static int daemonForkIntoBackground(const char *argv0) goto cleanup; if (dup2(stdoutfd, STDERR_FILENO) != STDERR_FILENO) goto cleanup; - if (stdinfd > STDERR_FILENO && VIR_CLOSE(stdinfd) < 0) + if (VIR_CLOSE(stdinfd) < 0) goto cleanup; - if (stdoutfd > STDERR_FILENO && VIR_CLOSE(stdoutfd) < 0) + if (VIR_CLOSE(stdoutfd) < 0) goto cleanup; if (setsid() < 0) -- 1.9.3

This ends up being a very bizarre false positive. With an assist from eblake, the claim is that mgetgroups() could return a -1 value, but yet still have a groups buffer allocated, yet the example shown doesn't seem to prove that. Rather than fret about it, by adding a well placed sa_assert() on the returned *list value we can "assure" ourselves that the mgetgroups() failure path won't signal this condition. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virutil.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virutil.c b/src/util/virutil.c index 8d2f62a..5197969 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1063,6 +1063,7 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list) ret = mgetgroups(user, primary, list); if (ret < 0) { + sa_assert(!*list); virReportSystemError(errno, _("cannot get group list for '%s'"), user); goto cleanup; -- 1.9.3

With the virGetGroupList() change in place - Coverity further complains that if we fail to virFork(), the groups will be leaked - which aha seems to be the case. Adjust the logic to save off the -errno, free the groups, and then return the value we saved Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virfile.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 7c506c9..b3b8be2 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2000,8 +2000,11 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, } pid = virFork(); - if (pid < 0) - return -errno; + if (pid < 0) { + ret = -errno; + VIR_FREE(groups); + return ret; + } if (pid == 0) { -- 1.9.3

Coverity complains that because of how 'offset' is initialized to 0 (zero), the resulting math and comparison on rem is pointless. For the "while (rem < 0)", the value of 'rem' must be between 0 and 86399 (SECS_PER_DAY = 86400ULL). Thus, the addition of offset (0) does nothing and the while (rem < 0) is pointless. For the "while (rem > SECS_PER_DAY)", we have the same issue. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virtime.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/virtime.c b/src/util/virtime.c index 9fefb67..99c7cf6 100644 --- a/src/util/virtime.c +++ b/src/util/virtime.c @@ -135,10 +135,12 @@ void virTimeFieldsThen(unsigned long long when, struct tm *fields) days = whenSecs / SECS_PER_DAY; rem = whenSecs % SECS_PER_DAY; rem += offset; + /* coverity[dead_error_condition] - when offset is calculated remove this */ while (rem < 0) { rem += SECS_PER_DAY; --days; } + /* coverity[dead_error_condition] - when offset is calculated remove this */ while (rem >= SECS_PER_DAY) { rem -= SECS_PER_DAY; ++days; -- 1.9.3

If we end up at the cleanup lable before we've VIR_EXPAND_N the list, then calling virQEMUCapsFreeStringList() with a NULL proplist could theoretically deref proplist if nproplist was set. Coverity doesn't Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a652f29..81ada48 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1728,7 +1728,7 @@ virQEMUCapsParseDeviceStrObjectProps(const char *str, ret = nproplist; cleanup: - if (ret < 0) + if (ret < 0 && proplist) virQEMUCapsFreeStringList(nproplist, proplist); return ret; } -- 1.9.3

Add a check of the return for virDomainHostdevInsert() like every other call. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libxl/libxl_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 17d6257..2f2c590 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2891,7 +2891,8 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) return -1; } - virDomainHostdevInsert(vmdef, hostdev); + if (virDomainHostdevInsert(vmdef, hostdev) < 0) + return -1; break; default: -- 1.9.3

Coverity complains that the condition "size + 1 == 0" cannot happen. Since 'size' is unsigned 32bit value set using virReadBufInt32BE. Thus rather than + 1, it seems the comparison should be is it at max now and if so, return the failure. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virstoragefile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 299edcd..0219ce8 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -393,7 +393,7 @@ qcowXGetBackingStore(char **res, } if (offset + size > buf_size || offset + size < offset) return BACKING_STORE_INVALID; - if (size + 1 == 0) + if (size == UINT_MAX) return BACKING_STORE_INVALID; if (VIR_ALLOC_N(*res, size + 1) < 0) return BACKING_STORE_ERROR; -- 1.9.3