[libvirt] [PATCH v2 0/6] Fix some Coverity issues
This series is based off the review of patch 1 from the series: http://www.redhat.com/archives/libvir-list/2015-September/msg00841.html In review of patch 1: http://www.redhat.com/archives/libvir-list/2015-September/msg00859.html it was noted that instead of using sa_assert, the proper checks should be made. During investigation, I found that while the caller could check for a non-NULL "first" parameter that ends up being used for strtok_r, that was not "good enough" for Coverity which still needed to consider the function where the to be first param cannot be NULL. In any case, I separated out each into their own patch rather than lumping them together. Patches 1-4 should be relatively straightforward. Patch 5 is new - it's one that I had been working on and finally figured out what the issue is/was. It was a bit more complex and hidden. Patch 6 was from the original patch 1, but it's review had a comment regarding using virBitmap* instead of the open coding. This one I believe I have intoned the magic words to make it better, but since I don't use xenapi, perhaps extra care would be necessary to make sure I got it right. John Ferlan (6): openvz: Resolve Coverity FORWARD_NULL openvz: Resolve Coverity FORWARD_NULL libxl: Resolve Coverity FORWARD_NULL esx: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL xenapi: Resolve Coverity FORWARD_NULL src/esx/esx_vi.c | 5 +++++ src/libxl/libxl_conf.c | 6 ++++++ src/openvz/openvz_conf.c | 8 ++++---- src/qemu/qemu_process.c | 14 ++++++++++++-- src/xenapi/xenapi_driver.c | 12 +++++++----- src/xenapi/xenapi_utils.c | 21 --------------------- src/xenapi/xenapi_utils.h | 2 -- 7 files changed, 34 insertions(+), 34 deletions(-) -- 2.1.0
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@redhat.com> --- 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
Coverity complains that strtok_r cannot be called if the first and third parameters are NULL. On entry if the 'value' parameter is not checked for being NULL before the VIR_STRDUP which will set the target to NULL if the source (eg value) is NULL. Thus add a check for NULL value. NB: Although each of the callers to openvzParseBarrierLimit would only make the call when 'value' was non-NULL, it seems that doesn't satisfy the checker. The NULL check had to be in this funtion. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/openvz/openvz_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index fb8768a..e99b83c 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -135,10 +135,10 @@ openvzParseBarrierLimit(const char* value, { char *token; char *saveptr = NULL; - char *str; + char *str = NULL; int ret = -1; - if (VIR_STRDUP(str, value) < 0) + if (!value || VIR_STRDUP(str, value) < 0) goto error; token = strtok_r(str, ":", &saveptr); @@ -396,7 +396,7 @@ openvzReadFSConf(virDomainDefPtr def, param = "DISKSPACE"; ret = openvzReadVPSConfigParam(veid, param, &temp); if (ret > 0) { - if (openvzParseBarrierLimit(temp, &barrier, &limit)) { + if (openvzParseBarrierLimit(temp, &barrier, &limit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not read '%s' from config for container %d"), param, veid); -- 2.1.0
On Fri, Sep 25, 2015 at 12:31:41 -0400, John Ferlan wrote:
Coverity complains that strtok_r cannot be called if the first and third parameters are NULL. On entry if the 'value' parameter is not checked for being NULL before the VIR_STRDUP which will set the target to NULL if the source (eg value) is NULL. Thus add a check for NULL value.
NB: Although each of the callers to openvzParseBarrierLimit would only make the call when 'value' was non-NULL, it seems that doesn't satisfy the checker. The NULL check had to be in this funtion.
Erm, so then file a bug against coverity rather than hacking around it?
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/openvz/openvz_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index fb8768a..e99b83c 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -135,10 +135,10 @@ openvzParseBarrierLimit(const char* value, { char *token; char *saveptr = NULL; - char *str; + char *str = NULL; int ret = -1;
- if (VIR_STRDUP(str, value) < 0) + if (!value || VIR_STRDUP(str, value) < 0)
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.
goto error;
token = strtok_r(str, ":", &saveptr); @@ -396,7 +396,7 @@ openvzReadFSConf(virDomainDefPtr def, param = "DISKSPACE"; ret = openvzReadVPSConfigParam(veid, param, &temp); if (ret > 0) { - if (openvzParseBarrierLimit(temp, &barrier, &limit)) { + if (openvzParseBarrierLimit(temp, &barrier, &limit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not read '%s' from config for container %d"), param, veid);
This change should be mentioned in the commit message since it's different from what the current message says. Peter
On 10/07/2015 02:14 AM, Peter Krempa wrote:
On Fri, Sep 25, 2015 at 12:31:41 -0400, John Ferlan wrote:
Coverity complains that strtok_r cannot be called if the first and third parameters are NULL. On entry if the 'value' parameter is not checked for being NULL before the VIR_STRDUP which will set the target to NULL if the source (eg value) is NULL. Thus add a check for NULL value.
NB: Although each of the callers to openvzParseBarrierLimit would only make the call when 'value' was non-NULL, it seems that doesn't satisfy the checker. The NULL check had to be in this funtion.
Erm, so then file a bug against coverity rather than hacking around it?
Even doing that will take "time" for them to fix and even when fixed doesn't mean it works in our build environment. Besides I didn't have success in generating a "small reproducer". The last time I logged a bug against Coverity was with the VIR_FREE() ternary operation. That was "fixed"; however, if I remove the change from the code and build, it seems for 95% of the cases the claim of RESOURCE_LEAK has gone away, but a few of these type FORWARD_NULL's now appear. My guess is the ternary in strtok_r is what's causing the issue. That guess is based on experience of looking at Coverity issues from trace around the issue: (5) Event cond_true: Condition "1", taking true branch (6) Event cond_true: Condition "(size_t)(void const *)&":"[1] - (size_t)(void const *)":" == 1", taking true branch (7) Event cond_true: Condition "(char const *)":"[0] != 0", taking true branch (8) Event cond_true: Condition "(char const *)":"[1] == 0", taking true branch (9) Event var_deref_model: Passing "&saveptr" to "__strtok_r_1c", which dereferences null "saveptr". [details] and the "details" for the error that Coverity links one to, where events (5) - (8) seems to come from: 1137 #if !defined _HAVE_STRING_ARCH_strtok_r || defined _FORCE_INLINES 1138 # ifndef _HAVE_STRING_ARCH_strtok_r 1139 # define __strtok_r(s, sep, nextp) \ 1140 (__extension__ (__builtin_constant_p (sep) && __string2_1bptr_p (sep) \ 1141 && ((const char *) (sep))[0] != '\0' \ 1142 && ((const char *) (sep))[1] == '\0' \ 1143 ? __strtok_r_1c (s, ((const char *) (sep))[0], nextp) \ 1144 : __strtok_r (s, sep, nextp))) 1145 # endif 1146 1147 __STRING_INLINE char *__strtok_r_1c (char *__s, char __sep, char **__nextp); 1148 __STRING_INLINE char * 1149 __strtok_r_1c (char *__s, char __sep, char **__nextp) ... Remember that Coverity tries "every" path... I'm still not sure why some strtok_r calls are "ok" while others aren't. I believe it mostly has to do with where it's done. Perhaps virStringSplit* type calls would also solve these...
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/openvz/openvz_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index fb8768a..e99b83c 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -135,10 +135,10 @@ openvzParseBarrierLimit(const char* value, { char *token; char *saveptr = NULL; - char *str; + char *str = NULL; int ret = -1;
- if (VIR_STRDUP(str, value) < 0) + if (!value || VIR_STRDUP(str, value) < 0)
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.
If I change to : if (VIR_STRDUP(str, value) <= 0) it doesn't help - so it's something about checking !value that keeps Coverity quiet. That !value check also must be done in the same function as the strtok_r, so that lends even more credence to my belief about the ternary.
goto error;
token = strtok_r(str, ":", &saveptr); @@ -396,7 +396,7 @@ openvzReadFSConf(virDomainDefPtr def, param = "DISKSPACE"; ret = openvzReadVPSConfigParam(veid, param, &temp); if (ret > 0) { - if (openvzParseBarrierLimit(temp, &barrier, &limit)) { + if (openvzParseBarrierLimit(temp, &barrier, &limit) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not read '%s' from config for container %d"), param, veid);
This change should be mentioned in the commit message since it's different from what the current message says.
I forgot about this line... Not that it will matter as I'll just abandon this change and just filter it in my local build. John
On Wed, Oct 07, 2015 at 09:56:54 -0400, John Ferlan wrote:
On 10/07/2015 02:14 AM, Peter Krempa wrote:
On Fri, Sep 25, 2015 at 12:31:41 -0400, John Ferlan wrote:
Coverity complains that strtok_r cannot be called if the first and third parameters are NULL. On entry if the 'value' parameter is not checked for being NULL before the VIR_STRDUP which will set the target to NULL if the source (eg value) is NULL. Thus add a check for NULL value.
NB: Although each of the callers to openvzParseBarrierLimit would only make the call when 'value' was non-NULL, it seems that doesn't satisfy the checker. The NULL check had to be in this funtion.
Erm, so then file a bug against coverity rather than hacking around it?
Even doing that will take "time" for them to fix and even when fixed doesn't mean it works in our build environment.
Well, that seems to be the problem with non-free products.
Besides I didn't have success in generating a "small reproducer".
The last time I logged a bug against Coverity was with the VIR_FREE() ternary operation. That was "fixed"; however, if I remove the change from the code and build, it seems for 95% of the cases the claim of RESOURCE_LEAK has gone away, but a few of these type FORWARD_NULL's now appear. My guess is the ternary in strtok_r is what's causing the issue. That guess is based on experience of looking at Coverity issues from trace around the issue:
So a commercial product (not that free-ness of it would matter) is telling us that our code is wrong, which isn't true. If that same software would actually insist on adding a bug I don't think we should act on it's behalf. Yes .. that's a speculation, but the point is that even if you do have access to a tool like that it should help and not create just additional work. If we change our code just to shut it up it's not really adding any value by using it. Also if somebody sent a patch like this without mentioning any static analysis the first question that most reviewers would ask is why it actually is necessary and how does it help. I doubt we'd accept anything like that.
(5) Event cond_true: Condition "1", taking true branch (6) Event cond_true: Condition "(size_t)(void const *)&":"[1] - (size_t)(void const *)":" == 1", taking true branch (7) Event cond_true: Condition "(char const *)":"[0] != 0", taking true branch (8) Event cond_true: Condition "(char const *)":"[1] == 0", taking true branch (9) Event var_deref_model: Passing "&saveptr" to "__strtok_r_1c", which dereferences null "saveptr". [details]
and the "details" for the error that Coverity links one to, where events (5) - (8) seems to come from:
1137 #if !defined _HAVE_STRING_ARCH_strtok_r || defined _FORCE_INLINES 1138 # ifndef _HAVE_STRING_ARCH_strtok_r 1139 # define __strtok_r(s, sep, nextp) \ 1140 (__extension__ (__builtin_constant_p (sep) && __string2_1bptr_p (sep) \ 1141 && ((const char *) (sep))[0] != '\0' \ 1142 && ((const char *) (sep))[1] == '\0' \ 1143 ? __strtok_r_1c (s, ((const char *) (sep))[0], nextp) \ 1144 : __strtok_r (s, sep, nextp))) 1145 # endif 1146 1147 __STRING_INLINE char *__strtok_r_1c (char *__s, char __sep, char **__nextp); 1148 __STRING_INLINE char * 1149 __strtok_r_1c (char *__s, char __sep, char **__nextp) ...
Remember that Coverity tries "every" path... I'm still not sure why some strtok_r calls are "ok" while others aren't. I believe it mostly has to do with where it's done. Perhaps virStringSplit* type calls would also solve these...
Probably yes. I also would not oppose such change as it actually has some value in comparison with workarounds like this.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/openvz/openvz_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
To be clear, NACK to this and workarounds for false positives like this. Peter
Since the strtok_r call in libxlCapsInitGuests expects a non NULL first parameter when the third parameter is NULL, we need to check that the returned 'capabilities' from a libxl_get_version_info call is not NULL and error out if so since the code expects it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_conf.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 35fd495..4eed5ca 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -319,6 +319,12 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) return -1; } + if (!ver_info->capabilities) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to get capabilities from libxenlight")); + return -1; + } + err = regcomp(®ex, XEN_CAP_REGEX, REG_EXTENDED); if (err != 0) { char error[100]; -- 2.1.0
On Fri, Sep 25, 2015 at 12:31:42 -0400, John Ferlan wrote:
Since the strtok_r call in libxlCapsInitGuests expects a non NULL first parameter when the third parameter is NULL, we need to check that the returned 'capabilities' from a libxl_get_version_info call is not NULL and error out if so since the code expects it.
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.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_conf.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 35fd495..4eed5ca 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -319,6 +319,12 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) return -1; }
+ if (!ver_info->capabilities) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to get capabilities from libxenlight")); + return -1; + } +
ACK, Peter
Coverity complains that strtok_r cannot be called if the first and third parameters are NULL. On entry, the 'path' parameter is not checked for being NULL before the VIR_STRDUP which will set the target to NULL if the source (eg path) is NULL. Thus add a check for NULL path. NB: Checks were made in the caller to only call this function if that parameter was non-NULL; however, that didn't satisfy the checker. The check had to be in this function. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/esx/esx_vi.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index af822b1..1b46c10 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -1169,6 +1169,11 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) esxVI_ManagedObjectReference *root = NULL; esxVI_Folder *folder = NULL; + if (!path) { + virReportError(VIR_ERR_INVALID_ARG, "%s", _("Path is empty")); + goto cleanup; + } + if (VIR_STRDUP(tmp, path) < 0) goto cleanup; -- 2.1.0
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@redhat.com> --- 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 Fri, Sep 25, 2015 at 12:31:44 -0400, John Ferlan wrote:
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@redhat.com> --- 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,
Since we are in qemuProcessStop, which means that the whole active definition will be nuked a few lines below this code ...
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;
.. should we even bother to do this? Just remove the free and let virDomainDefFree remove it afterwards either when vm->def is being replaced by vm->newDef or when the transient vm object is being nuked too. Peter
On 10/07/2015 02:32 AM, Peter Krempa wrote:
On Fri, Sep 25, 2015 at 12:31:44 -0400, John Ferlan wrote:
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@redhat.com> --- 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,
Since we are in qemuProcessStop, which means that the whole active definition will be nuked a few lines below this code ...
Just removing the VIR_FREE(net->ifname) works - that's fine as well. Do you want to see the diff before pushing? John
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;
.. should we even bother to do this? Just remove the free and let virDomainDefFree remove it afterwards either when vm->def is being replaced by vm->newDef or when the transient vm object is being nuked too.
Peter
On Wed, Oct 07, 2015 at 10:12:05 -0400, John Ferlan wrote:
On 10/07/2015 02:32 AM, Peter Krempa wrote:
On Fri, Sep 25, 2015 at 12:31:44 -0400, John Ferlan wrote:
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@redhat.com> --- 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,
Since we are in qemuProcessStop, which means that the whole active definition will be nuked a few lines below this code ...
Just removing the VIR_FREE(net->ifname) works - that's fine as well. Do you want to see the diff before pushing?
ACK to that, but it might need a better commit message. Peter
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@redhat.com> --- 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
Subject is technically right, but entirely misleading. This patch is more a refactor of the code that fixes a coverity error rather than the other way around. On Fri, Sep 25, 2015 at 12:31:45 -0400, John Ferlan wrote:
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
So a false positive again?
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@redhat.com> --- 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(-)
This makes me like this patch though.
ACK if you fix the commit message. Peter
On 09/25/2015 12:31 PM, John Ferlan wrote:
This series is based off the review of patch 1 from the series:
http://www.redhat.com/archives/libvir-list/2015-September/msg00841.html
In review of patch 1:
http://www.redhat.com/archives/libvir-list/2015-September/msg00859.html
it was noted that instead of using sa_assert, the proper checks should be made. During investigation, I found that while the caller could check for a non-NULL "first" parameter that ends up being used for strtok_r, that was not "good enough" for Coverity which still needed to consider the function where the to be first param cannot be NULL.
In any case, I separated out each into their own patch rather than lumping them together.
Patches 1-4 should be relatively straightforward.
Patch 5 is new - it's one that I had been working on and finally figured out what the issue is/was. It was a bit more complex and hidden.
Patch 6 was from the original patch 1, but it's review had a comment regarding using virBitmap* instead of the open coding. This one I believe I have intoned the magic words to make it better, but since I don't use xenapi, perhaps extra care would be necessary to make sure I got it right.
John Ferlan (6): openvz: Resolve Coverity FORWARD_NULL openvz: Resolve Coverity FORWARD_NULL libxl: Resolve Coverity FORWARD_NULL esx: Resolve Coverity FORWARD_NULL qemu: Resolve Coverity FORWARD_NULL xenapi: Resolve Coverity FORWARD_NULL
src/esx/esx_vi.c | 5 +++++ src/libxl/libxl_conf.c | 6 ++++++ src/openvz/openvz_conf.c | 8 ++++---- src/qemu/qemu_process.c | 14 ++++++++++++-- src/xenapi/xenapi_driver.c | 12 +++++++----- src/xenapi/xenapi_utils.c | 21 --------------------- src/xenapi/xenapi_utils.h | 2 -- 7 files changed, 34 insertions(+), 34 deletions(-)
I pushed patches 3 & 5 (with adjustment noted in review to code and commit message). Patch 1, 2, and 4 I understand are NACK'd - that's fine - I get the reasoning. Not sure I 100% agree with the statement that we wouldn't accept a patch that wasn't absolutely necessary or didn't help. I find some of the refactor patches unnecessary especially when they mess up backports, but they get accepted for the 'greater good'. Patch 6 while ACK'd I'm less confident about the results. It would be nice to have someone with the xenapi environment "test" that it works. Since it too is a "workaround" of sorts similar to patches 1, 2, & 4, I'll let it sit for now. I finally had some luck generating a small program to exhibit the error - I'll be able to submit a coverity bug and see what happens. John
participants (2)
-
John Ferlan -
Peter Krempa