[libvirt] [PATCH 0/9] More Coverity fixes
There are two repeats from the last series (1 & 2). For patch 1, I went with my suggestion - I'm open to others For patch 2, Coverity was complaining more about the way nparams would be overwritten - fix that by adding a new variable New patches 3 & 4 -> eblake helped out with these - especially the mgetgroups oddity 5 -> Fallout from fixing 4 6 -> virTimeFieldsThen() and the "offset = 0". I'd be OK with deleting the code, but it just feels like someone had it on a todo list to come back to some day 7 & 8 -> Fairly straightforward 9 -> This was an interesting case - it seems from what was being done that I have the right "answer". I did go all the way back to the initial submission of the code and it did the same thing, except it was using an unsigned long instead of int and well thus wouldn't ever hit the condition since we're grabbing the big endian int value This gets me down to 5 issues John Ferlan (9): remote_driver: Resolve Coverity RESOURCE_LEAK virsh: Resolve Coverity NEGATIVE_RETURNS daemon: Resolve Coverity RESOURCE_LEAK virutil: Resolve Coverity RESOURCE_LEAK virfile: Resolve Coverity RESOURCE_LEAK virtime: Resolve Coverity DEADCODE qemu: Resolve Coverity FORWARD_NULL libxl: Resolve Coverity CHECKED_RETURN virstoragefile: Resolve Coverity DEADCODE daemon/libvirtd.c | 8 ++++---- src/libxl/libxl_driver.c | 3 ++- src/qemu/qemu_capabilities.c | 2 +- src/remote/remote_driver.c | 20 +++++++++++++++++++- src/util/virfile.c | 7 +++++-- src/util/virstoragefile.c | 2 +- src/util/virtime.c | 2 ++ src/util/virutil.c | 1 + tools/virsh-domain.c | 7 ++++--- 9 files changed, 39 insertions(+), 13 deletions(-) -- 1.9.3
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@redhat.com> --- 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
On 09/12/14 02:05, John Ferlan wrote:
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@redhat.com> --- 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; + }
I still dislike this part. I'd rather see limit implemented also for this branch and not used to cover up a false positive of coverity. If they decide to fix it later on we will have a piece of code for no reason or possibly worse if they will come up with a different error.
if (VIR_ALLOC_N(*params, ret_params_len) < 0) goto cleanup; }
Peter
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@redhat.com> --- 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
On 09/11/2014 06:05 PM, John Ferlan wrote:
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@redhat.com> --- tools/virsh-domain.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com> --- 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
On 09/11/2014 06:05 PM, John Ferlan wrote:
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@redhat.com> --- daemon/libvirtd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
ACK. This means that if anyone tries to do something crazy like 'libvirtd 0<&-' with stdin closed, and the OS doesn't auto-reopen an fd, then spawning into a daemon will now fail where it previously worked. But this is a _good_ thing, as it is much easier to reason about programs that prohibit being started without something open at all three standard descriptors, and since POSIX states that it is undefined behavior if you exec a process without open fds. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com> --- 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
[adding gnulib, in case anyone else runs into the same false positive] On 09/11/2014 06:06 PM, John Ferlan wrote:
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@redhat.com> --- src/util/virutil.c | 1 + 1 file changed, 1 insertion(+)
ACK. More details that we worked out on IRC: the mgetgroups code gets a list that may contain duplicates, so it makes a final pass to reduce two types of duplicates - any later member with the first, or any two adjacent members. if (1 < ng) { gid_t first = *g; gid_t *next; gid_t *groups_end = g + ng; for (next = g + 1; next < groups_end; next++) { if (*next == first || *next == *g) ng--; else *++g = *next; } } Coverity was assuming that ng-- could be executed enough times to make it go negative, but looking closer at the loop bounds, the loop cannot run more than the original ng - 1 times, so ng >= 1. I'm not sure if upstream gnulib can or should do anything to silence Coverity from triggering the false positive. But the added assert in the caller definitely lets Coverity fill in the gaps for what we know to be true.
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;
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com> --- 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
On 09/11/2014 06:06 PM, John Ferlan wrote:
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@redhat.com> --- src/util/virfile.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
ACK
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) {
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com> --- 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
On 09/12/14 02:06, John Ferlan wrote:
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@redhat.com> ---
From your cover letter: 6 -> virTimeFieldsThen() and the "offset = 0". I'd be OK with deleting the code, but it just feels like someone had it on a todo list to come back to some day According to the comment in the code: void virTimeFieldsThen(unsigned long long when, struct tm *fields) { /* This code is taken from GLibC under terms of LGPLv2+ */ long int days, rem, y; it was borrowed from glibc. AFAIK we decided to always use GMT in our time stamps and everywhere so it'll be probably better just to delete the code and note that a part is missing. I doubt that anyone will be adding it later.
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;
Anyways, if somebody speaks against deleting the part, then ACK to this approach. Peter
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@redhat.com> --- 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
On 09/11/2014 06:06 PM, John Ferlan wrote:
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
Incomplete sentence.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
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; }
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add a check of the return for virDomainHostdevInsert() like every other call. Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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
John Ferlan wrote:
Add a check of the return for virDomainHostdevInsert() like every other call.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK. Regards, Jim
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:
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@redhat.com> --- 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
On 09/11/2014 06:06 PM, John Ferlan wrote:
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@redhat.com> --- 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)
Is this dead code? After all, we just checked that offset+size is not larger than buf_size (and buf_size is smaller than UINT_MAX); and also that offset+size didn't overflow.
return BACKING_STORE_INVALID; if (VIR_ALLOC_N(*res, size + 1) < 0) return BACKING_STORE_ERROR;
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/11/2014 06:05 PM, John Ferlan wrote:
There are two repeats from the last series (1 & 2).
For patch 1, I went with my suggestion - I'm open to others For patch 2, Coverity was complaining more about the way nparams would be overwritten - fix that by adding a new variable
New patches 3 & 4 -> eblake helped out with these - especially the mgetgroups oddity 5 -> Fallout from fixing 4 6 -> virTimeFieldsThen() and the "offset = 0". I'd be OK with deleting the code, but it just feels like someone had it on a todo list to come back to some day 7 & 8 -> Fairly straightforward 9 -> This was an interesting case - it seems from what was being done that I have the right "answer". I did go all the way back to the initial submission of the code and it did the same thing, except it was using an unsigned long instead of int and well thus wouldn't ever hit the condition since we're grabbing the big endian int value
Too late for me to give a competent review on 1 or 6, I may try again in the morning when I'm not as tired. 3 and 4 are indeed tricky, but I already helped you on it earlier in the day. I've got a question on 9, but again, sleep may help me reason about it better. And with that, I'm off to some much-needed sleep :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/11/2014 08:05 PM, John Ferlan wrote:
There are two repeats from the last series (1 & 2).
For patch 1, I went with my suggestion - I'm open to others For patch 2, Coverity was complaining more about the way nparams would be overwritten - fix that by adding a new variable
New patches 3 & 4 -> eblake helped out with these - especially the mgetgroups oddity 5 -> Fallout from fixing 4 6 -> virTimeFieldsThen() and the "offset = 0". I'd be OK with deleting the code, but it just feels like someone had it on a todo list to come back to some day 7 & 8 -> Fairly straightforward 9 -> This was an interesting case - it seems from what was being done that I have the right "answer". I did go all the way back to the initial submission of the code and it did the same thing, except it was using an unsigned long instead of int and well thus wouldn't ever hit the condition since we're grabbing the big endian int value
This gets me down to 5 issues
John Ferlan (9): remote_driver: Resolve Coverity RESOURCE_LEAK virsh: Resolve Coverity NEGATIVE_RETURNS daemon: Resolve Coverity RESOURCE_LEAK virutil: Resolve Coverity RESOURCE_LEAK virfile: Resolve Coverity RESOURCE_LEAK virtime: Resolve Coverity DEADCODE qemu: Resolve Coverity FORWARD_NULL libxl: Resolve Coverity CHECKED_RETURN virstoragefile: Resolve Coverity DEADCODE
daemon/libvirtd.c | 8 ++++---- src/libxl/libxl_driver.c | 3 ++- src/qemu/qemu_capabilities.c | 2 +- src/remote/remote_driver.c | 20 +++++++++++++++++++- src/util/virfile.c | 7 +++++-- src/util/virstoragefile.c | 2 +- src/util/virtime.c | 2 ++ src/util/virutil.c | 1 + tools/virsh-domain.c | 7 ++++--- 9 files changed, 39 insertions(+), 13 deletions(-)
I pushed 2-5,7,8 leaving 1, 6, & 9 for further examination/changes. I went and looked deeper for 6 - the code was added by commit id '3ec12898' when the logging API's needed to generate timestamps. The commit message has this: virTimeFieldsNowRaw replacements for gmtime(), convert a timestamp virTimeFieldsThenRaw into a broken out set of fields. No localtime() replacement is provided, because converting to local time is not practical with only async signal safe APIs. So yeah - I think it's safe to remove the "deadcode", but will give it a bit of (ahem) time for other viewpoints Tks, John
participants (4)
-
Eric Blake -
Jim Fehlig -
John Ferlan -
Peter Krempa