[libvirt] [PATCH 0/3] More Coverity changes
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
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@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
On 25.02.2016 15:21, John Ferlan wrote:
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@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))
if (barrier && virStrToLong_ull() < 0)
goto error; - } else { - if (limit != NULL) { - if (virStrToLong_ull(token, NULL, 10, limit)) - goto error; - } - } + + if (limit && virStrToLong_ull(tmp[1], NULL, 10, limit))
same here.
+ goto error; + ret = 0; error: - VIR_FREE(str); + virStringFreeListCount(tmp, ntmp); return ret; }
ACK Michal
Found by my Coverity checker - virCheckFlags call could return -1, but not virCommandFree(destroy_cmd). Signed-off-by: John Ferlan <jferlan@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
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@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
On 25.02.2016 15:21, John Ferlan wrote:
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(-)
ACK series and safe for the freeze. But see my comment for 1/3 before pushing. Michal
participants (2)
-
John Ferlan -
Michal Privoznik