[libvirt] [PATCH 00/26] Resolve more Coverity issues
Sorry for the large dump, but before I got too involved in other things I figured I'd go through the list of the remaining 68 Coverity issues from the new version in order to reduce the pile. Many are benign, some seemingly false positives, and I think most are error paths. The one non error path that does stick out is the qemu_driver.c changes in the qemuDomainSetBlkioParameters() routine where 'param' and 'params' were used differently between LIVE and CONFIG. In particular, in CONFIG the use of 'params->field' instead of 'param->field'. One that does bear looking at more closely and if someone has a better idea is avoiding a false positive resource_leak in remote_driver.c. I left a healthy comment in the code - you'll know when you see it. These patches get the numbers down to 19 issues. Of the remaining issues - some are related to Coverity thinking that 'mgetgroups' could return a negative value with an allocated groups structure (which I'm still scratching my head over). There is also a few calls to virJSONValueObjectGetNumberUlong() in qemu_monitor_json.c that don't check status, but I'm not sure why - just didn't have the research cycles for that. John Ferlan (26): qemu_driver: Resolve Coverity COPY_PASTE_ERROR remote_driver: Resolve Coverity RESOURCE_LEAK storage: Resolve Coverity UNUSED_VALUE vbox: Resolve Coverity UNUSED_VALUE qemu: Resolve Coverity REVERSE_INULL storage: Resolve Coverity OVERFLOW_BEFORE_WIDEN virsh: Resolve Coverity DEADCODE virfile: Resolve Coverity DEADCODE virsh: Resolve Coverity DEADCODE qemu: Resolve Coverity DEADCODE tests: Resolve Coverity DEADCODE virsh: Resolve Coverity DEADCODE qemu: Resolve Coverity FORWARD_NULL lxc: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL network: Resolve Coverity FORWARD_NULL virstring: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL network_conf: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity NEGATIVE_RETURNS nodeinfo: Resolve Coverity NEGATIVE_RETURNS virsh: Resolve Coverity NEGATIVE_RETURNS xen: Resolve Coverity NEGATIVE_RETURNS qemu: Resolve Coverity NEGATIVE_RETURNS qemu: Resolve Coverity NEGATIVE_RETURNS libxl: Resolve Coverity NULL_RETURNS src/conf/network_conf.c | 4 ++-- src/libxl/libxl_migration.c | 1 - src/lxc/lxc_driver.c | 6 ++++-- src/network/leaseshelper.c | 3 +-- src/nodeinfo.c | 2 +- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_command.c | 1 + src/qemu/qemu_driver.c | 26 +++++++++++++++----------- src/qemu/qemu_migration.c | 3 ++- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 5 +++-- src/remote/remote_driver.c | 12 ++++++++++++ src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 1 - src/util/virfile.c | 5 ++--- src/util/virstring.c | 3 +++ src/vbox/vbox_common.c | 9 +++++++-- src/xen/xend_internal.c | 3 ++- tests/virstringtest.c | 5 +++++ tools/virsh-domain.c | 22 ++++++++-------------- tools/virsh-edit.c | 9 --------- tools/virsh-interface.c | 3 --- tools/virsh-network.c | 12 +++++------- tools/virsh-nwfilter.c | 3 --- tools/virsh-pool.c | 3 --- tools/virsh-snapshot.c | 3 --- 26 files changed, 76 insertions(+), 74 deletions(-) -- 1.9.3
In qemuDomainSetBlkioParameters(), Coverity points out that the calls to qemuDomainParseBlkioDeviceStr() are slightly different and points out there may be a cut-n-paste error. In the first call (AFFECT_LIVE), the second parameter is "param->field"; however, for the second call (AFFECT_CONFIG), the second parameter is "params->field". It seems the "param->field" is correct especially since each path as a setting of "param" to "¶ms[i]". Furthermore, there were a few more instances of using "params[i]" instead of "param->" which I cleaned up. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5121f85..f52f00d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7825,7 +7825,7 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr param = ¶ms[i]; if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT)) { - if (virCgroupSetBlkioWeight(priv->cgroup, params[i].value.ui) < 0) + if (virCgroupSetBlkioWeight(priv->cgroup, param->value.ui) < 0) ret = -1; } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) || STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_READ_IOPS) || @@ -7836,7 +7836,7 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, virBlkioDevicePtr devices = NULL; size_t j; - if (qemuDomainParseBlkioDeviceStr(params[i].value.s, + if (qemuDomainParseBlkioDeviceStr(param->value.s, param->field, &devices, &ndevices) < 0) { @@ -7919,7 +7919,7 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr param = ¶ms[i]; if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT)) { - persistentDef->blkio.weight = params[i].value.ui; + persistentDef->blkio.weight = param->value.ui; } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) || STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_READ_IOPS) || STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WRITE_IOPS) || @@ -7928,8 +7928,8 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, virBlkioDevicePtr devices = NULL; size_t ndevices; - if (qemuDomainParseBlkioDeviceStr(params[i].value.s, - params->field, + if (qemuDomainParseBlkioDeviceStr(param->value.s, + param->field, &devices, &ndevices) < 0) { ret = -1; -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
In qemuDomainSetBlkioParameters(), Coverity points out that the calls to qemuDomainParseBlkioDeviceStr() are slightly different and points out there may be a cut-n-paste error.
In the first call (AFFECT_LIVE), the second parameter is "param->field"; however, for the second call (AFFECT_CONFIG), the second parameter is "params->field". It seems the "param->field" is correct especially since each path as a setting of "param" to "¶ms[i]". Furthermore, there were a few more instances of using "params[i]" instead of "param->" which I cleaned up.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5121f85..f52f00d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7825,7 +7825,7 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr param = ¶ms[i];
if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT)) { - if (virCgroupSetBlkioWeight(priv->cgroup, params[i].value.ui) < 0) + if (virCgroupSetBlkioWeight(priv->cgroup, param->value.ui) < 0) ret = -1; } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) || STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_READ_IOPS) || @@ -7836,7 +7836,7 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, virBlkioDevicePtr devices = NULL; size_t j;
- if (qemuDomainParseBlkioDeviceStr(params[i].value.s, + if (qemuDomainParseBlkioDeviceStr(param->value.s, param->field, &devices, &ndevices) < 0) { @@ -7919,7 +7919,7 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr param = ¶ms[i];
if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT)) { - persistentDef->blkio.weight = params[i].value.ui; + persistentDef->blkio.weight = param->value.ui; } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) || STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_READ_IOPS) || STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WRITE_IOPS) || @@ -7928,8 +7928,8 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, virBlkioDevicePtr devices = NULL; size_t ndevices;
- if (qemuDomainParseBlkioDeviceStr(params[i].value.s, - params->field, + if (qemuDomainParseBlkioDeviceStr(param->value.s, + param->field,
wow, this was the real bug here. The original code would use the "field" value of the first element in the params array. Very easy to overlook.
&devices, &ndevices) < 0) { ret = -1;
ACK. Peter
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. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 915e8e5..4b4644d 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; } -- 1.9.3
On 09/05/14 00:26, 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.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 915e8e5..4b4644d 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; }
This unfortunately breaks the remote driver impl of GetAllDomainStats. As it seems that the limit parameter isn't used for automatically allocated arrays and I expected that it is I'll need either fix the remote impl of the stats function or add support for checking the limit here. And I probably prefer option 2, fixing remoteDeserializeTypedParameters to use limit correctly even for auto-alloced typed parameters. Peter
On 09/11/2014 02:38 AM, Peter Krempa wrote:
On 09/05/14 00:26, 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
Careful, my virDomainBlockCopy API passes nparams==0 && params == (non-NULL 0-length array) from the RPC daemon/remote.c receiver into the libvirtd side of the API call. It tripped me up the first time I tried to assert that nparams==0 implied params==NULL, and failed the test.
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.
This unfortunately breaks the remote driver impl of GetAllDomainStats. As it seems that the limit parameter isn't used for automatically allocated arrays and I expected that it is I'll need either fix the remote impl of the stats function or add support for checking the limit here. And I probably prefer option 2, fixing remoteDeserializeTypedParameters to use limit correctly even for auto-alloced typed parameters.
I haven't looked closely at the patch, but it is a tricky area of code. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/11/2014 11:55 AM, Eric Blake wrote:
On 09/11/2014 02:38 AM, Peter Krempa wrote:
On 09/05/14 00:26, 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
Careful, my virDomainBlockCopy API passes nparams==0 && params == (non-NULL 0-length array) from the RPC daemon/remote.c receiver into the libvirtd side of the API call. It tripped me up the first time I tried to assert that nparams==0 implied params==NULL, and failed the test.
Well in general the newer Coverity has been squawking about this case - that is assumptions where nparams/params type parameters are used. In most cases, it's a "for (i = 0; i < nparams; i++)" do something with "params[i]" that get flagged on the params[i] reference because there's no nparams == 0 type check. Particular to this query though the code in question is remoteDeserializeTypedParameters(); whereas, the remoteDomainBlockCopy() uses remoteSerializeTypedParameters(). And yes, it's all very tricky. While I do believe from reading the code that this particular case is a false positive - I really was trying to find a way around making too many changes. Open to suggestions naturally! John
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.
This unfortunately breaks the remote driver impl of GetAllDomainStats. As it seems that the limit parameter isn't used for automatically allocated arrays and I expected that it is I'll need either fix the remote impl of the stats function or add support for checking the limit here. And I probably prefer option 2, fixing remoteDeserializeTypedParameters to use limit correctly even for auto-alloced typed parameters.
I haven't looked closely at the patch, but it is a tricky area of code.
On 09/11/2014 04:38 AM, Peter Krempa wrote:
On 09/05/14 00:26, 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.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 915e8e5..4b4644d 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; }
This unfortunately breaks the remote driver impl of GetAllDomainStats. As it seems that the limit parameter isn't used for automatically allocated arrays and I expected that it is I'll need either fix the remote impl of the stats function or add support for checking the limit here. And I probably prefer option 2, fixing remoteDeserializeTypedParameters to use limit correctly even for auto-alloced typed parameters.
Peter
Hmm... yeah after a bit of digging I see - the '&ret->params' is a virTypedParameterPtr which yes, will be NULL on input, <sigh>... Of course 'limit' never being checked could have led to other problems down the road, but I don't think we're there yet. The "good" news is it seems a comparison 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; } Prior to the (VIR_ALLOC(elem) < 0) check would suffice as well as passing the '0' in the 3rd (or limit) param. If we don't do the limit != 0 check, then other callers of remoteDeserializeTypedParameters() would probably need some sort of /* coverity[resource_leak] */ comment or the remoteDomainGetJobStats would have to change to not pass 0 if the decision was to adjust the logic in remoteDeserializeTypedParameters I was merely using that 0 passing as a "marker" since 'limit' wasn't checked/used in the auto allocated case. It was a whole lot easier, cheaper, simpler than the alternatives. John
Since cd4d547576a4f0371d1d4d4e0ca6db124c5ba257 Coverity notes that setting 'ret = -3' prior to the unconditional setting of 'ret = 0' will cause the value to be UNUSED. Since the comment indicates that it is expect to allow the code to continue, just remove the ret = -3 setting. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index b8f907a..0f8ceee 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -128,7 +128,6 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot probe backing volume format: %s"), target->backingStore->path); - ret = -3; } else { target->backingStore->format = rc; } -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Since cd4d547576a4f0371d1d4d4e0ca6db124c5ba257
Coverity notes that setting 'ret = -3' prior to the unconditional setting of 'ret = 0' will cause the value to be UNUSED.
Since the comment indicates that it is expect to allow the code to continue, just remove the ret = -3 setting.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 1 - 1 file changed, 1 deletion(-)
ACK, Peter
Handle a few places where Coverity complains about the value being unused. For two of them (Close cases) - the comments above the close indicate there is no harm to ignore the error - so added an ignore_value. For the other condition, added an rc check like other callers. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vbox/vbox_common.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index b9858ee..10a3205 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4340,6 +4340,11 @@ static int vboxCloseDisksRecursively(virDomainPtr dom, char *location) PRUnichar *childLocationUtf = NULL; char *childLocation = NULL; rc = gVBoxAPI.UIMedium.GetLocation(childMedium, &childLocationUtf); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get childMedium location")); + goto cleanup; + } VBOX_UTF16_TO_UTF8(childLocationUtf, &childLocation); VBOX_UTF16_FREE(childLocationUtf); if (vboxCloseDisksRecursively(dom, childLocation) < 0) { @@ -4762,7 +4767,7 @@ vboxSnapshotRedefine(virDomainPtr dom, * succeed, unless there is still another machine which uses the * medium. No harm done if we ignore the error. */ - rc = gVBoxAPI.UIMedium.Close(medium); + ignore_value(gVBoxAPI.UIMedium.Close(medium)); } VBOX_UTF8_FREE(locationUtf8); } @@ -6982,7 +6987,7 @@ vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot) * reference in a sane order, which means that closing will normally * succeed, unless there is still another machine which uses the * medium. No harm done if we ignore the error. */ - rc = gVBoxAPI.UIMedium.Close(medium); + ignore_value(gVBoxAPI.UIMedium.Close(medium)); } VBOX_UTF16_FREE(locationUtf16); VBOX_UTF8_FREE(locationUtf8); -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Handle a few places where Coverity complains about the value being unused. For two of them (Close cases) - the comments above the close indicate there is no harm to ignore the error - so added an ignore_value. For the other condition, added an rc check like other callers.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vbox/vbox_common.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
ACK
Coverity complains that checking for !domlist after setting doms = domlist and making a deref of doms just above It seems the call in question was intended to me made in the case that 'doms' was passed in and not when the virDomainObjListExport() call allocated domlist and already called virConnectGetAllDomainStatsCheckACL(). Thus rather than check for !domlist - check that "doms != domlist" in order to avoid the Coverity message. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f52f00d..362d1ab 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17363,7 +17363,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, if (!(dom = qemuDomObjFromDomain(doms[i]))) continue; - if (!domlist && + if (doms != domlist && !virConnectGetAllDomainStatsCheckACL(conn, dom->def)) continue; -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Coverity complains that checking for !domlist after setting doms = domlist and making a deref of doms just above
It seems the call in question was intended to me made in the case that 'doms' was passed in and not when the virDomainObjListExport() call allocated domlist and already called virConnectGetAllDomainStatsCheckACL().
Thus rather than check for !domlist - check that "doms != domlist" in order to avoid the Coverity message.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
False positive, but fair enough. ACK. Peter
Coverity complains that when multiplying to 32 bit values that eventually will be stored in a 64 bit value that it's possible the math could overflow unless one of the values being multiplied is type cast to the proper size. 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 cb6a8d5..abab1e1 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -560,7 +560,7 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr pool, unsigned long long extraBytes = 0; unsigned long long alignedAllocation = allocation; virStoragePoolSourceDevicePtr dev = &pool->def->source.devices[0]; - unsigned long long cylinderSize = dev->geometry.heads * + unsigned long long cylinderSize = (unsigned long long)dev->geometry.heads * dev->geometry.sectors * SECTOR_SIZE; VIR_DEBUG("find free area: allocation %llu, cyl size %llu", allocation, -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Coverity complains that when multiplying to 32 bit values that eventually will be stored in a 64 bit value that it's possible the math could overflow unless one of the values being multiplied is type cast to the proper size.
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 cb6a8d5..abab1e1 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -560,7 +560,7 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr pool, unsigned long long extraBytes = 0; unsigned long long alignedAllocation = allocation; virStoragePoolSourceDevicePtr dev = &pool->def->source.devices[0]; - unsigned long long cylinderSize = dev->geometry.heads * + unsigned long long cylinderSize = (unsigned long long)dev->geometry.heads * dev->geometry.sectors * SECTOR_SIZE;
VIR_DEBUG("find free area: allocation %llu, cyl size %llu", allocation,
ACK
Since 0766783abbe8bbc9ea686c2c3149f4c0ac139e19 Coverity complains that the EDIT_FREE definition results in DEADCODE. As it turns out with the change to use the EDIT_FREE macro the call to vir*Free() wouldn't be necessary nor would it happen... Prior code to above commitid would : vir*Ptr foo = NULL; ... foo = vir*GetXMLDesc() ... vir*Free(foo); foo = vir*DefineXML() ... And thus the free was needed. With the change to use EDIT_FREE the same code changed to: vir*Ptr foo = NULL; vir*Ptr foo_edited = NULL; ... foo = vir*GetXMLDesc() ... if (foo_edited) vir*Free(foo_edited); foo_edited = vir*DefineXML() ... However, foo_edited could never be set in the code path - even with all the goto's since the only way for it to be set is if vir*DefineXML() succeeds in which case the code to allow a retry (and thus all the goto's) never leaves foo_edited set All error paths lead to "cleanup:" which causes both foo and foo_edited to call the respective vir*Free() routines if set. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 5 ----- tools/virsh-edit.c | 9 --------- tools/virsh-interface.c | 3 --- tools/virsh-network.c | 3 --- tools/virsh-nwfilter.c | 3 --- tools/virsh-pool.c | 3 --- tools/virsh-snapshot.c | 3 --- 7 files changed, 29 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 68d49d6..1cdb596 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4032,7 +4032,6 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (virDomainSaveImageDefineXML(ctl->conn, file, doc_edited, define_flags) == 0) -#define EDIT_FREE /* */ #include "virsh-edit.c" vshPrint(ctl, _("State file %s edited.\n"), file); @@ -7162,7 +7161,6 @@ cmdMetadata(vshControl *ctl, const vshCmd *cmd) #define EDIT_DEFINE \ (virDomainSetMetadata(dom, VIR_DOMAIN_METADATA_ELEMENT, doc_edited, \ key, uri, flags) == 0) -#define EDIT_FREE /* nothing */ #include "virsh-edit.c" vshPrint("%s\n", _("Metadata modified")); @@ -10656,9 +10654,6 @@ cmdEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (dom_edited = virDomainDefineXML(ctl->conn, doc_edited)) -#define EDIT_FREE \ - if (dom_edited) \ - virDomainFree(dom_edited); #include "virsh-edit.c" vshPrint(ctl, _("Domain %s XML configuration edited.\n"), diff --git a/tools/virsh-edit.c b/tools/virsh-edit.c index 5d35c55..10298f6 100644 --- a/tools/virsh-edit.c +++ b/tools/virsh-edit.c @@ -40,9 +40,6 @@ * For example: * #define EDIT_DEFINE (dom_edited = virDomainDefineXML(ctl->conn, doc_edited)) * - * EDIT_FREE - statement which vir*Free()-s object defined by EDIT_DEFINE, e.g: - * #define EDIT_FREE if (dom_edited) virDomainFree(dom_edited); - * * Michal Privoznik <mprivozn@redhat.com> */ @@ -58,10 +55,6 @@ # error Missing EDIT_DEFINE definition #endif -#ifndef EDIT_FREE -# error Missing EDIT_FREE definition -#endif - do { char *tmp = NULL; char *doc = NULL; @@ -116,7 +109,6 @@ do { } /* Everything checks out, so redefine the object. */ - EDIT_FREE; if (!msg && !(EDIT_DEFINE)) { msg = _("Failed."); } @@ -162,4 +154,3 @@ do { #undef EDIT_GET_XML #undef EDIT_NOT_CHANGED #undef EDIT_DEFINE -#undef EDIT_FREE diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index d4ec854..6cacaf1 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -128,9 +128,6 @@ cmdInterfaceEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (iface_edited = virInterfaceDefineXML(ctl->conn, doc_edited, 0)) -#define EDIT_FREE \ - if (iface_edited) \ - virInterfaceFree(iface_edited); #include "virsh-edit.c" vshPrint(ctl, _("Interface %s XML configuration edited.\n"), diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 9497472..5fe4b32 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1111,9 +1111,6 @@ cmdNetworkEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (network_edited = virNetworkDefineXML(ctl->conn, doc_edited)) -#define EDIT_FREE \ - if (network_edited) \ - virNetworkFree(network_edited); #include "virsh-edit.c" vshPrint(ctl, _("Network %s XML configuration edited.\n"), diff --git a/tools/virsh-nwfilter.c b/tools/virsh-nwfilter.c index 6e6e21b..ca54c9d 100644 --- a/tools/virsh-nwfilter.c +++ b/tools/virsh-nwfilter.c @@ -428,9 +428,6 @@ cmdNWFilterEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (nwfilter_edited = virNWFilterDefineXML(ctl->conn, doc_edited)) -#define EDIT_FREE \ - if (nwfilter_edited) \ - virNWFilterFree(nwfilter); #include "virsh-edit.c" vshPrint(ctl, _("Network filter %s XML configuration edited.\n"), diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 80313b1..0b8dba5 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -1767,9 +1767,6 @@ cmdPoolEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (pool_edited = virStoragePoolDefineXML(ctl->conn, doc_edited, 0)) -#define EDIT_FREE \ - if (pool_edited) \ - virStoragePoolFree(pool_edited); #include "virsh-edit.c" vshPrint(ctl, _("Pool %s XML configuration edited.\n"), diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 3ecb3de..885ad22 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -589,9 +589,6 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) (strstr(doc, "<state>disk-snapshot</state>") ? \ define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY : 0), \ edited = virDomainSnapshotCreateXML(dom, doc_edited, define_flags) -#define EDIT_FREE \ - if (edited) \ - virDomainSnapshotFree(edited); #include "virsh-edit.c" edited_name = virDomainSnapshotGetName(edited); -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Since 0766783abbe8bbc9ea686c2c3149f4c0ac139e19
Coverity complains that the EDIT_FREE definition results in DEADCODE.
As it turns out with the change to use the EDIT_FREE macro the call to vir*Free() wouldn't be necessary nor would it happen...
Prior code to above commitid would :
vir*Ptr foo = NULL; ... foo = vir*GetXMLDesc() ... vir*Free(foo); foo = vir*DefineXML() ...
And thus the free was needed. With the change to use EDIT_FREE the same code changed to:
vir*Ptr foo = NULL; vir*Ptr foo_edited = NULL; ... foo = vir*GetXMLDesc() ... if (foo_edited) vir*Free(foo_edited); foo_edited = vir*DefineXML() ...
However, foo_edited could never be set in the code path - even with all the goto's since the only way for it to be set is if vir*DefineXML() succeeds in which case the code to allow a retry (and thus all the goto's) never leaves foo_edited set
All error paths lead to "cleanup:" which causes both foo and foo_edited to call the respective vir*Free() routines if set.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 5 ----- tools/virsh-edit.c | 9 --------- tools/virsh-interface.c | 3 --- tools/virsh-network.c | 3 --- tools/virsh-nwfilter.c | 3 --- tools/virsh-pool.c | 3 --- tools/virsh-snapshot.c | 3 --- 7 files changed, 29 deletions(-)
Yep, the free()s in the cleanup section of each command are redundant. ACK
Adjust the parentheses in/for the waitpid loops; otherwise, Coverity points out: (1) Event assignment: Assigning: "waitret" = "waitpid(pid, &status, 0) == -1" (2) Event between: At condition "waitret == -1", the value of "waitret" must be between 0 and 1. (3) Event dead_error_condition: The condition "waitret == -1" cannot be true. (4) Event dead_error_begin: Execution cannot reach this statement: "ret = -*__errno_location();". Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index cfb6cc1..b602144 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2072,8 +2072,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, } /* wait for child to complete, and retrieve its exit code */ - while ((waitret = waitpid(pid, &status, 0) == -1) - && (errno == EINTR)); + while ((waitret = waitpid(pid, &status, 0)) == -1 && (errno == EINTR)); if (waitret == -1) { ret = -errno; virReportSystemError(errno, @@ -2290,7 +2289,7 @@ virDirCreate(const char *path, if (pid) { /* parent */ /* wait for child to complete, and retrieve its exit code */ VIR_FREE(groups); - while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); + while ((waitret = waitpid(pid, &status, 0)) == -1 && (errno == EINTR)); if (waitret == -1) { ret = -errno; virReportSystemError(errno, -- 1.9.3
Well, the bug here is a bit more serious than mere DEADCODE ... On 09/05/14 00:26, John Ferlan wrote:
Adjust the parentheses in/for the waitpid loops; otherwise, Coverity points out:
(1) Event assignment: Assigning: "waitret" = "waitpid(pid, &status, 0) == -1" (2) Event between: At condition "waitret == -1", the value of "waitret" must be between 0 and 1. (3) Event dead_error_condition: The condition "waitret == -1" cannot be true. (4) Event dead_error_begin: Execution cannot reach this statement: "ret = -*__errno_location();".
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index cfb6cc1..b602144 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2072,8 +2072,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, }
/* wait for child to complete, and retrieve its exit code */ - while ((waitret = waitpid(pid, &status, 0) == -1) - && (errno == EINTR));
This existing code worked by chance as the return value of waitpid was compared to -1 and then assigned to waitret. ..also that part was used in the comparison. If the return value was -1 then it would correctly loop.
+ while ((waitret = waitpid(pid, &status, 0)) == -1 && (errno == EINTR));
the errno == EINTR doesn't need parentheses
if (waitret == -1) {
But this condition couldn't be reached.
ret = -errno; virReportSystemError(errno, @@ -2290,7 +2289,7 @@ virDirCreate(const char *path, if (pid) { /* parent */ /* wait for child to complete, and retrieve its exit code */ VIR_FREE(groups); - while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); + while ((waitret = waitpid(pid, &status, 0)) == -1 && (errno == EINTR));
Same issue here.
if (waitret == -1) { ret = -errno; virReportSystemError(errno,
ACK
Coverity points out that by using EMPTYSTR(type) we are guarding against the possibility that it could be NULL; however, based on how 'type' was initialized to NULL, then either "ipv4", "ipv6", or "" - there is no way it could be NULL. Since "-" is supposed to mean something empty in a field - remove the initialization to NULL and use it as the ending else rather than using "". Also changed the name from 'type' to 'typestr'. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-network.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 5fe4b32..f505c14 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1360,7 +1360,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) "---------------------------------------------------------"); for (i = 0; i < nleases; i++) { - const char *type = NULL; + const char *typestr; char *cidr_format = NULL; virNetworkDHCPLeasePtr lease = leases[i]; time_t expirytime_tmp = lease->expirytime; @@ -1369,14 +1369,15 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) ts = *localtime_r(&expirytime_tmp, &ts); strftime(expirytime, sizeof(expirytime), "%Y-%m-%d %H:%M:%S", &ts); - type = (lease->type == VIR_IP_ADDR_TYPE_IPV4) ? "ipv4" : - (lease->type == VIR_IP_ADDR_TYPE_IPV6) ? "ipv6" : ""; + typestr = (lease->type == VIR_IP_ADDR_TYPE_IPV4) ? "ipv4" : + (lease->type == VIR_IP_ADDR_TYPE_IPV6) ? "ipv6" : NULL; ignore_value(virAsprintf(&cidr_format, "%s/%d", lease->ipaddr, lease->prefix)); vshPrintExtra(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n", - expirytime, EMPTYSTR(lease->mac), EMPTYSTR(type), cidr_format, + expirytime, EMPTYSTR(lease->mac), + EMPTYSTR(typestr), cidr_format, EMPTYSTR(lease->hostname), EMPTYSTR(lease->clientid)); } -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Coverity points out that by using EMPTYSTR(type) we are guarding against the possibility that it could be NULL; however, based on how 'type' was initialized to NULL, then either "ipv4", "ipv6", or "" - there is no way it could be NULL. Since "-" is supposed to mean something empty in a field - remove the initialization to NULL and use it as the ending else rather than using "".
Also changed the name from 'type' to 'typestr'.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-network.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 5fe4b32..f505c14 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1360,7 +1360,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) "---------------------------------------------------------");
for (i = 0; i < nleases; i++) { - const char *type = NULL; + const char *typestr; char *cidr_format = NULL; virNetworkDHCPLeasePtr lease = leases[i]; time_t expirytime_tmp = lease->expirytime; @@ -1369,14 +1369,15 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) ts = *localtime_r(&expirytime_tmp, &ts); strftime(expirytime, sizeof(expirytime), "%Y-%m-%d %H:%M:%S", &ts);
- type = (lease->type == VIR_IP_ADDR_TYPE_IPV4) ? "ipv4" : - (lease->type == VIR_IP_ADDR_TYPE_IPV6) ? "ipv6" : ""; + typestr = (lease->type == VIR_IP_ADDR_TYPE_IPV4) ? "ipv4" : + (lease->type == VIR_IP_ADDR_TYPE_IPV6) ? "ipv6" : NULL;
Yuck, nested ternaries. Would you mind refactoring it to if/else or switch() while touching this?
ignore_value(virAsprintf(&cidr_format, "%s/%d", lease->ipaddr, lease->prefix));
vshPrintExtra(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n", - expirytime, EMPTYSTR(lease->mac), EMPTYSTR(type), cidr_format, + expirytime, EMPTYSTR(lease->mac), + EMPTYSTR(typestr), cidr_format, EMPTYSTR(lease->hostname), EMPTYSTR(lease->clientid)); }
ACK
Add another 'dead_code_begin' - victims of our own coding practices Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1ca98fb..6bba4a4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6297,6 +6297,7 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver, virBufferAddLit(&buf, ",kvm=off"); break; + /* coverity[dead_error_begin] */ case VIR_DOMAIN_KVM_LAST: break; } -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Add another 'dead_code_begin' - victims of our own coding practices
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 1 + 1 file changed, 1 insertion(+)
ACK
Coverity complains that the various checks for autoincrement and changed variables are DEADCODE - seems to me to be a false positive - so mark it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/virstringtest.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 7c25b22..10fad2c 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -162,10 +162,12 @@ testStrdup(const void *data ATTRIBUTE_UNUSED) virFilePrintf(stderr, "unexpected strdup result %d, expected 1\n", value); goto cleanup; } + /* coverity[dead_error_begin] */ if (i != 1) { virFilePrintf(stderr, "unexpected side effects i=%zu, expected 1\n", i); goto cleanup; } + /* coverity[dead_error_begin] */ if (j != 1) { virFilePrintf(stderr, "unexpected side effects j=%zu, expected 1\n", j); goto cleanup; @@ -182,14 +184,17 @@ testStrdup(const void *data ATTRIBUTE_UNUSED) virFilePrintf(stderr, "unexpected strdup result %d, expected 0\n", value); goto cleanup; } + /* coverity[dead_error_begin] */ if (i != 2) { virFilePrintf(stderr, "unexpected side effects i=%zu, expected 2\n", i); goto cleanup; } + /* coverity[dead_error_begin] */ if (j != 2) { virFilePrintf(stderr, "unexpected side effects j=%zu, expected 2\n", j); goto cleanup; } + /* coverity[dead_error_begin] */ if (k != 1) { virFilePrintf(stderr, "unexpected side effects k=%zu, expected 1\n", k); goto cleanup; -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Coverity complains that the various checks for autoincrement and changed variables are DEADCODE - seems to me to be a false positive - so mark it.
They are not false positive. Currently the code doesn't allow that to happen. The tests are designed to catch if somebody broke it though they need to stay.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/virstringtest.c | 5 +++++ 1 file changed, 5 insertions(+)
ACK
Coverity points out that if 'dom' isn't returned from virDomainQemuAttach, then the code already jumps to cleanup, so there was no need for the subsequent if (dom != NULL) check. I moved the error message about failure into the goto cleanup on failure and then removed the if (dom != NULL) Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1cdb596..f732a6e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8260,18 +8260,16 @@ cmdQemuAttach(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (!(dom = virDomainQemuAttach(ctl->conn, pid_value, flags))) - goto cleanup; - - if (dom != NULL) { - vshPrint(ctl, _("Domain %s attached to pid %u\n"), - virDomainGetName(dom), pid_value); - virDomainFree(dom); - ret = true; - } else { + if (!(dom = virDomainQemuAttach(ctl->conn, pid_value, flags))) { vshError(ctl, _("Failed to attach to pid %u"), pid_value); + goto cleanup; } + vshPrint(ctl, _("Domain %s attached to pid %u\n"), + virDomainGetName(dom), pid_value); + virDomainFree(dom); + ret = true; + cleanup: return ret; } -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Coverity points out that if 'dom' isn't returned from virDomainQemuAttach, then the code already jumps to cleanup, so there was no need for the subsequent if (dom != NULL) check.
I moved the error message about failure into the goto cleanup on failure and then removed the if (dom != NULL)
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
ACK
If the virJSONValueNewObject() fails, then rather than going to error and getting a Coverity false positive since it doesn't seem to understand the relationship between nkeywords, keywords, and values and seems to believe calling qemuFreeKeywords will cause a NULL deref - just return NULL Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 62e7d5d..b0bc5fc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -617,7 +617,7 @@ qemuMonitorJSONKeywordStringToJSON(const char *str, const char *firstkeyword) size_t i; if (!(ret = virJSONValueNewObject())) - goto error; + return NULL; if (qemuParseKeywords(str, &keywords, &values, &nkeywords, 1) < 0) goto error; -- 1.9.3
On 2014/9/5 6:26, John Ferlan wrote:
If the virJSONValueNewObject() fails, then rather than going to error and getting a Coverity false positive since it doesn't seem to understand the relationship between nkeywords, keywords, and values and seems to believe calling qemuFreeKeywords will cause a NULL deref - just return NULL
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 62e7d5d..b0bc5fc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -617,7 +617,7 @@ qemuMonitorJSONKeywordStringToJSON(const char *str, const char *firstkeyword) size_t i;
if (!(ret = virJSONValueNewObject())) - goto error; + return NULL;
Maybe it's not enough.
if (qemuParseKeywords(str, &keywords, &values, &nkeywords, 1) < 0) goto error;
Here, qemuParseKeywords() may fail and 'values' is still NULL.
On 09/09/2014 03:36 AM, Wang Rui wrote:
On 2014/9/5 6:26, John Ferlan wrote:
If the virJSONValueNewObject() fails, then rather than going to error and getting a Coverity false positive since it doesn't seem to understand the relationship between nkeywords, keywords, and values and seems to believe calling qemuFreeKeywords will cause a NULL deref - just return NULL
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 62e7d5d..b0bc5fc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -617,7 +617,7 @@ qemuMonitorJSONKeywordStringToJSON(const char *str, const char *firstkeyword) size_t i;
if (!(ret = virJSONValueNewObject())) - goto error; + return NULL;
Maybe it's not enough.
if (qemuParseKeywords(str, &keywords, &values, &nkeywords, 1) < 0) goto error;
Here, qemuParseKeywords() may fail and 'values' is still NULL.
Yes, but for some reason once qemuParseKeywords() is called Coverity has "resolved" that things will be OK. This one is a false positive and perhaps a bug in the Coverity parser. The return NULL is the way to avoid the condition. Filing a Coverity bug is an option, but changing the logic is just quicker. I think it gets flagged because Coverity perhaps prescans the code and "flags" all callers to qemuFreeKeywords as potentially having a FORWARD_NULL. It doesn't seem to pick up the fact that nkeywords is initialized to 0 (along with keywords and values being set to NULL). It's perhaps focusing on the array addresses rather than the logic of using nkeywords to access those arrays. John
On 09/05/14 00:26, John Ferlan wrote:
If the virJSONValueNewObject() fails, then rather than going to error and getting a Coverity false positive since it doesn't seem to understand the relationship between nkeywords, keywords, and values and seems to believe calling qemuFreeKeywords will cause a NULL deref - just return NULL
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
If we jump to cleanup before allocating 'result', then the call to virBlkioDeviceArrayClear() could dereference result Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f93360f..e5b6662 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2222,8 +2222,10 @@ lxcDomainParseBlkioDeviceStr(char *blkioDeviceStr, const char *type, _("unable to parse blkio device '%s' '%s'"), type, blkioDeviceStr); cleanup: - virBlkioDeviceArrayClear(result, ndevices); - VIR_FREE(result); + if (result) { + virBlkioDeviceArrayClear(result, ndevices); + VIR_FREE(result); + } return -1; } -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
If we jump to cleanup before allocating 'result', then the call to virBlkioDeviceArrayClear() could dereference result
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
ACK
If we jump to cleanup before allocating the 'result', then the call to virBlkioDeviceArrayClear will deref result causing a problem. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 362d1ab..373b4d7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7682,8 +7682,10 @@ qemuDomainParseBlkioDeviceStr(char *blkioDeviceStr, const char *type, _("unable to parse blkio device '%s' '%s'"), type, blkioDeviceStr); cleanup: - virBlkioDeviceArrayClear(result, ndevices); - VIR_FREE(result); + if (result) { + virBlkioDeviceArrayClear(result, ndevices); + VIR_FREE(result); + } return -1; } -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
If we jump to cleanup before allocating the 'result', then the call to virBlkioDeviceArrayClear will deref result causing a problem.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
ACK, Peter
If the VIR_STRDUP(exptime,...) fails, then we will jump to cleanup, no need to check if exptime is set which causes Coverity to issue a complaint in the virStrToLong_ll call because there wasn't a check for a NULL value while there was one for the reference right after the VIR_STRDUP(). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/network/leaseshelper.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index c8543a2..5b3c9c3 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -180,8 +180,7 @@ main(int argc, char **argv) goto cleanup; /* Removed extraneous trailing space in DNSMASQ_LEASE_EXPIRES (dnsmasq < 2.52) */ - if (exptime && - exptime[strlen(exptime) - 1] == ' ') + if (exptime[strlen(exptime) - 1] == ' ') exptime[strlen(exptime) - 1] = '\0'; /* Check if it is an IPv6 lease */ -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
If the VIR_STRDUP(exptime,...) fails, then we will jump to cleanup, no need to check if exptime is set which causes Coverity to issue a complaint in the virStrToLong_ll call because there wasn't a check for a NULL value while there was one for the reference right after the VIR_STRDUP().
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/network/leaseshelper.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
ACK
Perhaps a false positive, but since Coverity doesn't understand the relationship between the 'count' and the 'strings', rather than leave the chance the on input 'strings' is NULL and causes a deref - just check for it and return Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstring.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virstring.c b/src/util/virstring.c index b14f785..935f0c6 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -198,6 +198,9 @@ virStringFreeListCount(char **strings, { size_t i; + if (!strings) + return; + for (i = 0; i < count; i++) VIR_FREE(strings[i]); -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Perhaps a false positive, but since Coverity doesn't understand the relationship between the 'count' and the 'strings', rather than leave the chance the on input 'strings' is NULL and causes a deref - just check for it and return
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstring.c | 3 +++ 1 file changed, 3 insertions(+)
Yep, false positive. All callers shall pass 0 as count if strings is NULL. ACK, doesn't hurt. Peter
If the qemuMigrationEatCookie() fails to set mig, we jump to cleanup: which will call qemuMigrationCancelDriveMirror() without first checking if mig == NULL Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_migration.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9cfb77e..d6b0f29 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3428,7 +3428,8 @@ qemuMigrationRun(virQEMUDriverPtr driver, orig_err = virSaveLastError(); /* cancel any outstanding NBD jobs */ - qemuMigrationCancelDriveMirror(mig, driver, vm); + if (mig) + qemuMigrationCancelDriveMirror(mig, driver, vm); if (spec->fwdType != MIGRATION_FWD_DIRECT) { if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0) -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
If the qemuMigrationEatCookie() fails to set mig, we jump to cleanup: which will call qemuMigrationCancelDriveMirror() without first checking if mig == NULL
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_migration.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK, genuine bug
The code compares def->forwarders when deciding to return 0 at a couple of points, then uses "def->nfwds" as a way to index into the def->forwarders array. That reference results in Coverity complaining that def->forwarders being NULL was checked as part of an arithmetic OR operation where failure could be any one 5 conditions, but that is not checked when entering the loop to dereference the array. Changing the comparisons to use nfwds will clear the warnings Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/network_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 9571ee1..f013d6b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2360,7 +2360,7 @@ virNetworkDNSDefFormat(virBufferPtr buf, { size_t i, j; - if (!(def->forwardPlainNames || def->forwarders || def->nhosts || + if (!(def->forwardPlainNames || def->nfwds || def->nhosts || def->nsrvs || def->ntxts)) return 0; @@ -2376,7 +2376,7 @@ virNetworkDNSDefFormat(virBufferPtr buf, return -1; } virBufferAsprintf(buf, " forwardPlainNames='%s'", fwd); - if (!(def->forwarders || def->nhosts || def->nsrvs || def->ntxts)) { + if (!(def->nfwds || def->nhosts || def->nsrvs || def->ntxts)) { virBufferAddLit(buf, "/>\n"); return 0; } -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
The code compares def->forwarders when deciding to return 0 at a couple of points, then uses "def->nfwds" as a way to index into the def->forwarders array. That reference results in Coverity complaining that def->forwarders being NULL was checked as part of an arithmetic OR operation where failure could be any one 5 conditions, but that is not checked when entering the loop to dereference the array. Changing the comparisons to use nfwds will clear the warnings
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/network_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK
In qemuProcessInitPCIAddresses() if qemuMonitorGetAllPCIAddresses() returns a negative (or zero) value, then no need to call the qemuProcessDetectPCIAddresses(). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 38ed3fe..e48a6a1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2721,7 +2721,7 @@ qemuProcessInitPCIAddresses(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; int naddrs; - int ret; + int ret = -1; qemuMonitorPCIAddress *addrs = NULL; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) @@ -2730,7 +2730,8 @@ qemuProcessInitPCIAddresses(virQEMUDriverPtr driver, &addrs); qemuDomainObjExitMonitor(driver, vm); - ret = qemuProcessDetectPCIAddresses(vm, addrs, naddrs); + if (naddrs > 0) + ret = qemuProcessDetectPCIAddresses(vm, addrs, naddrs); VIR_FREE(addrs); -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
In qemuProcessInitPCIAddresses() if qemuMonitorGetAllPCIAddresses() returns a negative (or zero) value, then no need to call the qemuProcessDetectPCIAddresses().
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
ACK
If the virNumaGetNodeCPUs() call fails with -1, then jumping to cleanup with 'cpus == NULL' and calling virCapabilitiesClearHostNUMACellCPUTopology will cause issues. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nodeinfo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 92a3718..2008ade 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1933,7 +1933,7 @@ nodeCapsInitNUMA(virCapsPtr caps) ret = 0; cleanup: - if (topology_failed || ret < 0) + if ((topology_failed || ret < 0) && cpus) virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus); virBitmapFree(cpumap); -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
If the virNumaGetNodeCPUs() call fails with -1, then jumping to cleanup with 'cpus == NULL' and calling virCapabilitiesClearHostNUMACellCPUTopology will cause issues.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nodeinfo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 92a3718..2008ade 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1933,7 +1933,7 @@ nodeCapsInitNUMA(virCapsPtr caps) ret = 0;
cleanup: - if (topology_failed || ret < 0) + if ((topology_failed || ret < 0) && cpus) virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus);
False positive. This function checks for NULL.
virBitmapFree(cpumap);
Also the cleanup section frees "cpus" twice. That might be worth fixing too. ACK Peter
Coverity notes that if virDomainGetCPUStats returns a negative value into 'nparams' then when we end up at cleanup, the call to virTypedParams will have issues Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f732a6e..6fe637b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6650,6 +6650,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) return ret; failed_stats: + nparams = 0; vshError(ctl, _("Failed to retrieve CPU statistics for domain '%s'"), virDomainGetName(dom)); goto cleanup; -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Coverity notes that if virDomainGetCPUStats returns a negative value into 'nparams' then when we end up at cleanup, the call to virTypedParams will have issues
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f732a6e..6fe637b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6650,6 +6650,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) return ret;
failed_stats: + nparams = 0; vshError(ctl, _("Failed to retrieve CPU statistics for domain '%s'"), virDomainGetName(dom)); goto cleanup;
This would cause a memleak if the second call to virDomainGetCPUStats fails as it will jump to the same label, while "params" was already allocated. You need to clear nparams explicitly on the first call that determines the count of the returned stats field. Peter
Coverity notes that if the call to virBitmapParse() returns a negative value, then when we jump to the error label, the call to virCapabilitiesClearHostNUMACellCPUTopology() will have issues with the negative nb_cpus Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xen/xend_internal.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index d9e76fc..f1322c4 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1112,7 +1112,8 @@ sexpr_to_xend_topology(const struct sexpr *root, virCapsPtr caps) parse_error: virReportError(VIR_ERR_XEN_CALL, "%s", _("topology syntax error")); error: - virCapabilitiesClearHostNUMACellCPUTopology(cpuInfo, nb_cpus); + if (nb_cpus > 0) + virCapabilitiesClearHostNUMACellCPUTopology(cpuInfo, nb_cpus); VIR_FREE(cpuInfo); return -1; } -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Coverity notes that if the call to virBitmapParse() returns a negative value, then when we jump to the error label, the call to virCapabilitiesClearHostNUMACellCPUTopology() will have issues with the negative nb_cpus
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xen/xend_internal.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK
Coverity notes that if qemuMonitorGetMachines() returns a negative nmachines value, then the code at the cleanup label will have issues. 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 854a9b8..a652f29 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2285,7 +2285,7 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps, size_t defIdx = 0; if ((nmachines = qemuMonitorGetMachines(mon, &machines)) < 0) - goto cleanup; + return -1; if (VIR_ALLOC_N(qemuCaps->machineTypes, nmachines) < 0) goto cleanup; -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Coverity notes that if qemuMonitorGetMachines() returns a negative nmachines value, then the code at the cleanup label will have issues.
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 854a9b8..a652f29 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2285,7 +2285,7 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps, size_t defIdx = 0;
if ((nmachines = qemuMonitorGetMachines(mon, &machines)) < 0) - goto cleanup; + return -1;
Also nmachines doesn't need to be initialized.
if (VIR_ALLOC_N(qemuCaps->machineTypes, nmachines) < 0) goto cleanup;
ACK
Coverity notes that if the virConnectListAllDomains returns a negative value then the loop at the cleanup label that ends on numDomains will have issues. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 373b4d7..5ceed92 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1004,9 +1004,11 @@ qemuStateStop(void) ret = -1; cleanup: - for (i = 0; i < numDomains; i++) - virDomainFree(domains[i]); - VIR_FREE(domains); + if (domains) { + for (i = 0; i < numDomains; i++) + virDomainFree(domains[i]); + VIR_FREE(domains); + } VIR_FREE(flags); virObjectUnref(conn); virObjectUnref(cfg); -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
Coverity notes that if the virConnectListAllDomains returns a negative value then the loop at the cleanup label that ends on numDomains will have issues.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
ACK
With all the changes in my previous foray into this code, I forgot to remove the libxlDomainEventQueue(driver, event); call inside the dom == NULL condition. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index b5d8edf..0b562f7 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -527,7 +527,6 @@ libxlDomainMigrationFinish(virConnectPtr dconn, libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - libxlDomainEventQueue(driver, event); } cleanup: -- 1.9.3
On 09/05/14 00:26, John Ferlan wrote:
With all the changes in my previous foray into this code, I forgot to remove the libxlDomainEventQueue(driver, event); call inside the dom == NULL condition.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 1 - 1 file changed, 1 deletion(-)
ACK
Would it be easier if I made batches of 5 patches? or perhaps just batches of patches of the same error? Just want to get this off the pile Tks, John On 09/04/2014 06:26 PM, John Ferlan wrote:
Sorry for the large dump, but before I got too involved in other things I figured I'd go through the list of the remaining 68 Coverity issues from the new version in order to reduce the pile. Many are benign, some seemingly false positives, and I think most are error paths. The one non error path that does stick out is the qemu_driver.c changes in the qemuDomainSetBlkioParameters() routine where 'param' and 'params' were used differently between LIVE and CONFIG. In particular, in CONFIG the use of 'params->field' instead of 'param->field'.
One that does bear looking at more closely and if someone has a better idea is avoiding a false positive resource_leak in remote_driver.c. I left a healthy comment in the code - you'll know when you see it.
These patches get the numbers down to 19 issues. Of the remaining issues - some are related to Coverity thinking that 'mgetgroups' could return a negative value with an allocated groups structure (which I'm still scratching my head over). There is also a few calls to virJSONValueObjectGetNumberUlong() in qemu_monitor_json.c that don't check status, but I'm not sure why - just didn't have the research cycles for that.
John Ferlan (26): qemu_driver: Resolve Coverity COPY_PASTE_ERROR remote_driver: Resolve Coverity RESOURCE_LEAK storage: Resolve Coverity UNUSED_VALUE vbox: Resolve Coverity UNUSED_VALUE qemu: Resolve Coverity REVERSE_INULL storage: Resolve Coverity OVERFLOW_BEFORE_WIDEN virsh: Resolve Coverity DEADCODE virfile: Resolve Coverity DEADCODE virsh: Resolve Coverity DEADCODE qemu: Resolve Coverity DEADCODE tests: Resolve Coverity DEADCODE virsh: Resolve Coverity DEADCODE qemu: Resolve Coverity FORWARD_NULL lxc: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL network: Resolve Coverity FORWARD_NULL virstring: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL network_conf: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity NEGATIVE_RETURNS nodeinfo: Resolve Coverity NEGATIVE_RETURNS virsh: Resolve Coverity NEGATIVE_RETURNS xen: Resolve Coverity NEGATIVE_RETURNS qemu: Resolve Coverity NEGATIVE_RETURNS qemu: Resolve Coverity NEGATIVE_RETURNS libxl: Resolve Coverity NULL_RETURNS
src/conf/network_conf.c | 4 ++-- src/libxl/libxl_migration.c | 1 - src/lxc/lxc_driver.c | 6 ++++-- src/network/leaseshelper.c | 3 +-- src/nodeinfo.c | 2 +- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_command.c | 1 + src/qemu/qemu_driver.c | 26 +++++++++++++++----------- src/qemu/qemu_migration.c | 3 ++- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 5 +++-- src/remote/remote_driver.c | 12 ++++++++++++ src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 1 - src/util/virfile.c | 5 ++--- src/util/virstring.c | 3 +++ src/vbox/vbox_common.c | 9 +++++++-- src/xen/xend_internal.c | 3 ++- tests/virstringtest.c | 5 +++++ tools/virsh-domain.c | 22 ++++++++-------------- tools/virsh-edit.c | 9 --------- tools/virsh-interface.c | 3 --- tools/virsh-network.c | 12 +++++------- tools/virsh-nwfilter.c | 3 --- tools/virsh-pool.c | 3 --- tools/virsh-snapshot.c | 3 --- 26 files changed, 76 insertions(+), 74 deletions(-)
On 09/10/14 23:56, John Ferlan wrote:
Would it be easier if I made batches of 5 patches? or perhaps just batches of patches of the same error?
It might help to overcome the psychological barrier of taking responsibility to review everything in the given series :) Anyhow, I'll have a look at the complete plile instead of having yoy to re-send.
Just want to get this off the pile
Tks,
John
Peter
On 09/04/2014 06:26 PM, John Ferlan wrote:
Sorry for the large dump, but before I got too involved in other things I figured I'd go through the list of the remaining 68 Coverity issues from the new version in order to reduce the pile. Many are benign, some seemingly false positives, and I think most are error paths. The one non error path that does stick out is the qemu_driver.c changes in the qemuDomainSetBlkioParameters() routine where 'param' and 'params' were used differently between LIVE and CONFIG. In particular, in CONFIG the use of 'params->field' instead of 'param->field'.
One that does bear looking at more closely and if someone has a better idea is avoiding a false positive resource_leak in remote_driver.c. I left a healthy comment in the code - you'll know when you see it.
These patches get the numbers down to 19 issues. Of the remaining issues - some are related to Coverity thinking that 'mgetgroups' could return a negative value with an allocated groups structure (which I'm still scratching my head over). There is also a few calls to virJSONValueObjectGetNumberUlong() in qemu_monitor_json.c that don't check status, but I'm not sure why - just didn't have the research cycles for that.
John Ferlan (26): qemu_driver: Resolve Coverity COPY_PASTE_ERROR remote_driver: Resolve Coverity RESOURCE_LEAK storage: Resolve Coverity UNUSED_VALUE vbox: Resolve Coverity UNUSED_VALUE qemu: Resolve Coverity REVERSE_INULL storage: Resolve Coverity OVERFLOW_BEFORE_WIDEN virsh: Resolve Coverity DEADCODE virfile: Resolve Coverity DEADCODE virsh: Resolve Coverity DEADCODE qemu: Resolve Coverity DEADCODE tests: Resolve Coverity DEADCODE virsh: Resolve Coverity DEADCODE qemu: Resolve Coverity FORWARD_NULL lxc: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL network: Resolve Coverity FORWARD_NULL virstring: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL network_conf: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity NEGATIVE_RETURNS nodeinfo: Resolve Coverity NEGATIVE_RETURNS virsh: Resolve Coverity NEGATIVE_RETURNS xen: Resolve Coverity NEGATIVE_RETURNS qemu: Resolve Coverity NEGATIVE_RETURNS qemu: Resolve Coverity NEGATIVE_RETURNS libxl: Resolve Coverity NULL_RETURNS
src/conf/network_conf.c | 4 ++-- src/libxl/libxl_migration.c | 1 - src/lxc/lxc_driver.c | 6 ++++-- src/network/leaseshelper.c | 3 +-- src/nodeinfo.c | 2 +- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_command.c | 1 + src/qemu/qemu_driver.c | 26 +++++++++++++++----------- src/qemu/qemu_migration.c | 3 ++- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 5 +++-- src/remote/remote_driver.c | 12 ++++++++++++ src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 1 - src/util/virfile.c | 5 ++--- src/util/virstring.c | 3 +++ src/vbox/vbox_common.c | 9 +++++++-- src/xen/xend_internal.c | 3 ++- tests/virstringtest.c | 5 +++++ tools/virsh-domain.c | 22 ++++++++-------------- tools/virsh-edit.c | 9 --------- tools/virsh-interface.c | 3 --- tools/virsh-network.c | 12 +++++------- tools/virsh-nwfilter.c | 3 --- tools/virsh-pool.c | 3 --- tools/virsh-snapshot.c | 3 --- 26 files changed, 76 insertions(+), 74 deletions(-)
Thanks for taking the time to review... I have pushed those that were ACK'd: 1, 3-21, 23-26 Modifying 8 to remove the parentheses around the (errno == EINTR) Modifying 9 to change the ternary into an if/else, restoring the typestr = NULL; initializer. Also changed the commit message to match the change made... Modifying 21 to remove the "if (cpus < 0) VIR_FREE(cpus)" since the code above had already handled clearing cpus Leaving 2, 22 To revisit in a v2 John
participants (4)
-
Eric Blake -
John Ferlan -
Peter Krempa -
Wang Rui