12:31 a.m.
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
12:31 a.m.
New subject: [libvirt] [PATCH v2 1/6] openvz: Resolve Coverity FORWARD_NULL
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>
---
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
12:31 a.m.
New subject: [libvirt] [PATCH v2 2/6] openvz: Resolve Coverity FORWARD_NULL
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(a)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
2:14 p.m.
New subject: [libvirt] [PATCH v2 2/6] openvz: Resolve Coverity FORWARD_NULL
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(a)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
9:56 p.m.
New subject: [libvirt] [PATCH v2 2/6] openvz: Resolve Coverity FORWARD_NULL
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(a)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
11:51 p.m.
New subject: [libvirt] [PATCH v2 2/6] openvz: Resolve Coverity FORWARD_NULL
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(a)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
12:31 a.m.
New subject: [libvirt] [PATCH v2 3/6] libxl: Resolve Coverity FORWARD_NULL
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(a)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
2:19 p.m.
New subject: [libvirt] [PATCH v2 3/6] libxl: Resolve Coverity FORWARD_NULL
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(a)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
12:31 a.m.
New subject: [libvirt] [PATCH v2 4/6] esx: Resolve Coverity FORWARD_NULL
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(a)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
12:31 a.m.
New subject: [libvirt] [PATCH v2 5/6] qemu: Resolve Coverity FORWARD_NULL
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>
---
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
2:32 p.m.
New subject: [libvirt] [PATCH v2 5/6] qemu: Resolve Coverity FORWARD_NULL
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(a)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
10:12 p.m.
New subject: [libvirt] [PATCH v2 5/6] qemu: Resolve Coverity FORWARD_NULL
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(a)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
11:32 p.m.
New subject: [libvirt] [PATCH v2 5/6] qemu: Resolve Coverity FORWARD_NULL
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(a)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
12:31 a.m.
New subject: [libvirt] [PATCH v2 6/6] xenapi: Resolve Coverity FORWARD_NULL
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>
---
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
2:40 p.m.
New subject: [libvirt] [PATCH v2 6/6] xenapi: Resolve Coverity FORWARD_NULL
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:
ACK if you fix the 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
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(a)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.
7:44 a.m.
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
3315
days inactive
3327
days old
15 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa