In openvzGetVPSUUID while parsing the conf_file, if "line" is for some reason returned as NULL from a getline() call with a successful status, then the subsequent call to strtok_r would attempt to reference the 'saveptr' which was initialized to NULL leading to an issue. Altough getline() claims it would return an error, this does avoid the problem and of course keep Coverity quiet Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/openvz/openvz_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index db0a9a7..fb8768a 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -956,7 +956,7 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len) goto cleanup; while (1) { - if (getline(&line, &line_size, fp) < 0) { + if (getline(&line, &line_size, fp) < 0 || !line) { if (feof(fp)) { /* EOF, UUID was not found */ uuidstr[0] = 0; break; -- 2.1.0

On Fri, Sep 25, 2015 at 12:31:41 -0400, John Ferlan wrote: Erm, so then file a bug against coverity rather than hacking around it? VIR_STRDUP returns 0 if @value was NULL so you can as well as test for < 1. Since then the value gets initialized to NULL you don't need the first hunk. This change should be mentioned in the commit message since it's different from what the current message says. Peter

On Fri, Sep 25, 2015 at 12:31:42 -0400, John Ferlan wrote: According to the source of the function that fills ver_info http://fossies.org/dox/xen-4.5.1/libxl_8c_source.html#l05266 the "capabilities" member is filled via strdup and not checked, so this makes sense. Btw, few of the other fields have the same issue too, but none of those is used in this function. ACK, Peter

Coverity notices that net->ifname is potentially referenced after a VIR_FREE(). Looking through history, the vport check code was added by commit id 'df8100463' and later augmented by commit id 'd490f47b'. The data is allocated via virNetDevMacVLanCreateWithVPortProfile, so it is reasonable that it's free'd after the virNetDevMacVLanDeleteWithVPortProfile call. Additionally, the virNetDevTapDelete call was added by commit id '075650ff4', but it doesn't seem there's a corresponding VIR_FREE of the ifname, so include that as well while we're at it. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_process.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f2586a1..e3d1c62 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5321,6 +5321,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, def = vm->def; for (i = 0; i < def->nnets; i++) { + bool free_net_iface = false; virDomainNetDefPtr net = def->nets[i]; vport = virDomainNetGetActualVirtPortProfile(net); @@ -5332,13 +5333,16 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainNetGetActualDirectMode(net), virDomainNetGetActualVirtPortProfile(net), cfg->stateDir)); - VIR_FREE(net->ifname); + free_net_iface = true; break; case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: #ifdef VIR_NETDEV_TAP_REQUIRE_MANUAL_CLEANUP - if (!(vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)) + if (!(vport && vport->virtPortType == + VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)) { ignore_value(virNetDevTapDelete(net->ifname, net->backend.tap)); + free_net_iface = true; + } #endif break; } @@ -5355,6 +5359,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, } } + /* Corrolary to virNetDevMacVLanCreateWithVPortProfile or + * qemuNetworkIfaceConnect allocation + */ + if (free_net_iface) + VIR_FREE(net->ifname); + /* kick the device out of the hostdev list too */ virDomainNetRemoveHostdev(def, net); networkReleaseActualDevice(vm->def, net); -- 2.1.0

On 10/07/2015 02:32 AM, Peter Krempa wrote: Just removing the VIR_FREE(net->ifname) works - that's fine as well. Do you want to see the diff before pushing? John

Coverity flagged that if the input 'mask'parameter was NULL, then the strtok_r call would have a NULL first and third parameter, which would cause an issue. Although the caller doesn't call getCpuBitMapfromString with a non-NULL mask value, Coverity doesn't check that. A fix could have been to check the 'mask' in the function and keep Coverity happy; however, it was noted in a previous review that virBitmap* functions could be used instead. So this patch modifies xenapiDomainGetVcpus to use virBitmap* functions and removes getCpuBitMapfromString in favor of those. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/xenapi/xenapi_driver.c | 12 +++++++----- src/xenapi/xenapi_utils.c | 21 --------------------- src/xenapi/xenapi_utils.h | 2 -- 3 files changed, 7 insertions(+), 28 deletions(-) diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 3045c5a..4246c67 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1269,7 +1269,7 @@ xenapiDomainGetVcpus(virDomainPtr dom, virNodeInfo nodeInfo; virVcpuInfoPtr ifptr; xen_session *session = ((struct _xenapiPrivate *)(dom->conn->privateData))->session; - char *mask = NULL; + virBitmapPtr mask = NULL; if (cpumaps != NULL && maplen < 1) return -1; if (xenapiDomainGetInfo(dom, &domInfo) == 0) { @@ -1304,7 +1304,8 @@ xenapiDomainGetVcpus(virDomainPtr dom, } for (i = 0; i < vcpu_params->size; i++) { if (STREQ(vcpu_params->contents[i].key, "mask")) { - if (VIR_STRDUP(mask, vcpu_params->contents[i].val) < 0) { + if (virBitmapParse(vcpu_params->contents[i].val, 0, + &mask, maplen * CHAR_BIT) < 0) { xen_vm_set_free(vms); xen_string_string_map_free(vcpu_params); return -1; @@ -1318,10 +1319,11 @@ xenapiDomainGetVcpus(virDomainPtr dom, ifptr->state = VIR_VCPU_RUNNING; ifptr->cpuTime = 0; ifptr->cpu = 0; - if (mask != NULL) - getCpuBitMapfromString(mask, VIR_GET_CPUMAP(cpumaps, maplen, i), maplen); + if (mask && cpumaps) + virBitmapToDataBuf(mask, VIR_GET_CPUMAP(cpumaps, maplen, i), + maplen); } - VIR_FREE(mask); + virBitmapFree(mask); xen_vm_set_free(vms); return i; } diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index a80e084..5c910c8 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -293,27 +293,6 @@ mapDomainPinVcpu(unsigned char *cpumap, int maplen) return ret; } -/* obtains the CPU bitmap from the string passed */ -void -getCpuBitMapfromString(char *mask, unsigned char *cpumap, int maplen) -{ - int pos; - int max_bits = maplen * 8; - char *num = NULL, *bp = NULL; - bzero(cpumap, maplen); - num = strtok_r(mask, ",", &bp); - while (num != NULL) { - if (virStrToLong_i(num, NULL, 10, &pos) < 0) - return; - if (pos < 0 || pos > max_bits - 1) - VIR_WARN("number in str %d exceeds cpumap's max bits %d", pos, max_bits); - else - (cpumap)[pos / 8] |= (1 << (pos % 8)); - num = strtok_r(NULL, ",", &bp); - } -} - - /* mapping XenServer power state to Libvirt power state */ virDomainState mapPowerState(enum xen_vm_power_state state) diff --git a/src/xenapi/xenapi_utils.h b/src/xenapi/xenapi_utils.h index 26e1ac2..9bd49bb 100644 --- a/src/xenapi/xenapi_utils.h +++ b/src/xenapi/xenapi_utils.h @@ -59,8 +59,6 @@ xenapiNormalExitEnum2virDomainLifecycle(enum xen_on_normal_exit action); virDomainLifecycleCrashAction xenapiCrashExitEnum2virDomainLifecycle(enum xen_on_crash_behaviour action); -void getCpuBitMapfromString(char *mask, unsigned char *cpumap, int maplen); - int getStorageVolumeType(char *type); char *returnErrorFromSession(xen_session *session); -- 2.1.0