8:21 a.m.
All the issues here are found by a later version of Coverity than
the one used by our internal testing. Two were false positives and
one just an ordering issue with the virCheckFlags in the zfs backend.
The future coverity really has a problem with strtok_r usage after a
VIR_STRDUP which only checks < 0 because it's perfectly fine for the
"source" to be NULL and a < 0 check won't fail. The reason why this is
a problem is the target is then NULL and used for the strtok_r call.
Code inspection finds the cases we have are all false positives; however,
rather than making the <= or sa_assert() type check to validate that -
I chose to use the virStringSplitCount in order to split and inspect
the input string. It was much cleaner at least on the inspection front.
The storage changes related to checking (ret == -1) resulted in a
RESOURCE_LEAK false positive. As it turns out, the only reason why
-1 was being check was to determine whether or not the VIR_APPEND_ELEMENT
insertion failed. If that succeeds, then 'vol' is cleared anyway, so
truly we only need to attempt the free if we have a new vol. The
virStorageVolDefFree will quietly return if vol == NULL.
John Ferlan (3):
openvz: Use virStringSplitCount instead of strtok_r
zfs: Resolve RESOURCE_LEAK
storage: No need to check ret after VIR_APPEND_ELEMENT
src/openvz/openvz_conf.c | 32 +++++++++++---------------------
src/storage/storage_backend_logical.c | 2 +-
src/storage/storage_backend_zfs.c | 6 ++++--
3 files changed, 16 insertions(+), 24 deletions(-)
--
2.5.0
8:21 a.m.
New subject: [libvirt] [PATCH 1/3] openvz: Use virStringSplitCount instead of strtok_r
When parsing the barrier:limit values, use virStringSplitCount in order
to split the pair and make the approriate checks to get the data.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/openvz/openvz_conf.c | 32 +++++++++++---------------------
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index e32dd6f..cf0d67c 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -133,35 +133,25 @@ openvzParseBarrierLimit(const char* value,
unsigned long long *barrier,
unsigned long long *limit)
{
- char *token;
- char *saveptr = NULL;
- char *str;
+ char **tmp = NULL;
+ size_t ntmp = 0;
int ret = -1;
- if (VIR_STRDUP(str, value) < 0)
+ if (!(tmp = virStringSplitCount(value, ":", 0, &ntmp)))
goto error;
- token = strtok_r(str, ":", &saveptr);
- if (token == NULL) {
+ if (ntmp != 2)
goto error;
- } else {
- if (barrier != NULL) {
- if (virStrToLong_ull(token, NULL, 10, barrier))
- goto error;
- }
- }
- token = strtok_r(NULL, ":", &saveptr);
- if (token == NULL) {
+
+ if (barrier && virStrToLong_ull(tmp[0], NULL, 10, barrier))
goto error;
- } else {
- if (limit != NULL) {
- if (virStrToLong_ull(token, NULL, 10, limit))
- goto error;
- }
- }
+
+ if (limit && virStrToLong_ull(tmp[1], NULL, 10, limit))
+ goto error;
+
ret = 0;
error:
- VIR_FREE(str);
+ virStringFreeListCount(tmp, ntmp);
return ret;
}
--
2.5.0
8:21 a.m.
New subject: [libvirt] [PATCH 2/3] zfs: Resolve RESOURCE_LEAK
Found by my Coverity checker - virCheckFlags call could return -1, but
not virCommandFree(destroy_cmd).
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_zfs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c
index 6bf7963..4d04c70 100644
--- a/src/storage/storage_backend_zfs.c
+++ b/src/storage/storage_backend_zfs.c
@@ -355,10 +355,12 @@ virStorageBackendZFSDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED,
unsigned int flags)
{
int ret = -1;
- virCommandPtr destroy_cmd = virCommandNewArgList(ZFS, "destroy", NULL);
+ virCommandPtr destroy_cmd = NULL;
virCheckFlags(0, -1);
+ destroy_cmd = virCommandNewArgList(ZFS, "destroy", NULL);
+
virCommandAddArgFormat(destroy_cmd, "%s/%s",
pool->def->source.name, vol->name);
--
2.5.0
8:21 a.m.
New subject: [libvirt] [PATCH 3/3] storage: No need to check ret after VIR_APPEND_ELEMENT
Generates a false positive for Coverity, but it turns out there's no need
to check ret == -1 since if VIR_APPEND_ELEMENT is successful, the local
vol pointer is cleared anyway.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_logical.c | 2 +-
src/storage/storage_backend_zfs.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index f0d6f80..167fe58 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -307,7 +307,7 @@ virStorageBackendLogicalMakeVol(char **const groups,
ret = 0;
cleanup:
- if (is_new_vol && (ret == -1))
+ if (is_new_vol)
virStorageVolDefFree(vol);
return ret;
}
diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c
index 4d04c70..2e6e407 100644
--- a/src/storage/storage_backend_zfs.c
+++ b/src/storage/storage_backend_zfs.c
@@ -161,7 +161,7 @@ virStorageBackendZFSParseVol(virStoragePoolObjPtr pool,
cleanup:
virStringFreeList(tokens);
virStringFreeList(name_tokens);
- if (is_new_vol && (ret == -1))
+ if (is_new_vol)
virStorageVolDefFree(volume);
return ret;
}
--
2.5.0
3335
days inactive
3335
days old
5 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik