Re: [libvirt] [PATCH 1/6] Avoid Coverity FORWARD_NULL prior to strtok_r calls
On 09/24/2015 04:13 AM, Ján Tomko wrote:
On Wed, Sep 23, 2015 at 07:18:28PM -0400, John Ferlan wrote:
> The 'strtok_r' function requires passing a NULL as the first parameter
> on subsequent calls in order to ensure the code picks up where it left
> off on a previous call. However, Coverity doesn't quite realize this
> and points out that if a NULL was passed in as the third argument it would
> result in a possible NULL deref because the strtok_r function will assign
> the third argument to the first in the call is NULL.
The description seems contradictory to the patch. From the asserts that
fix it, I'd assume Coverity has a problem with the first argument
possibly being NULL - usually obtained from a library function Coverity
doesn't understand.
Been a while since I wrote this one and I've forgotten the details -
what I do "see" is according to Coverity for each of the instances
below, the strtok_r ends up calling __strtok_r_1c for some reason.
Although there are other strtok_r calls which don't end up in that same
path (some in the same module. I'll dig a bit deeper on each.
John
>
> Adding an sa_assert() prior to each initial call quiets Coverity
>
Thus rendering the coverity scan useless.
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/esx/esx_vi.c | 1 +
> src/libxl/libxl_conf.c | 1 +
> src/openvz/openvz_conf.c | 2 ++
> src/xenapi/xenapi_utils.c | 1 +
> 4 files changed, 5 insertions(+)
>
> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> index af822b1..a57d753 100644
> --- a/src/esx/esx_vi.c
> +++ b/src/esx/esx_vi.c
> @@ -1173,6 +1173,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx,
const char *path)
> goto cleanup;
>
> /* Lookup Datacenter */
> + sa_assert(tmp);
This should be asserted in the caller. It seems priv->parsedUri->path
can be NULL in esxConnectToVCenter, but this function is not called in
that case.
> item = strtok_r(tmp, "/", &saveptr);
>
> if (!item) {
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 35fd495..ab588e8 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -353,6 +353,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
> /* Split capabilities string into tokens. strtok_r is OK here because
> * we "own" the buffer. Parse out the features from each token.
> */
> + sa_assert(ver_info->capabilities);
This one should be closer to the function that filled out the string.
> for (str = ver_info->capabilities, nr_guest_archs = 0;
> nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0])
> && (token = strtok_r(str, " ", &saveptr)) !=
NULL;
> diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
> index db0a9a7..003272e 100644
> --- a/src/openvz/openvz_conf.c
> +++ b/src/openvz/openvz_conf.c
> @@ -138,6 +138,7 @@ openvzParseBarrierLimit(const char* value,
> char *str;
> int ret = -1;
>
> + sa_assert(value);
Same here.
> if (VIR_STRDUP(str, value) < 0)
> goto error;
>
> @@ -965,6 +966,7 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len)
> }
> }
>
> + sa_assert(line);
This one already is right after getline.
> iden = strtok_r(line, " ", &saveptr);
> uuidbuf = strtok_r(NULL, "\n", &saveptr);
>
> diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c
> index a80e084..6b710cd 100644
> --- a/src/xenapi/xenapi_utils.c
> +++ b/src/xenapi/xenapi_utils.c
> @@ -301,6 +301,7 @@ getCpuBitMapfromString(char *mask, unsigned char *cpumap, int
maplen)
> int max_bits = maplen * 8;
> char *num = NULL, *bp = NULL;
> bzero(cpumap, maplen);
> + sa_assert(mask);
> num = strtok_r(mask, ",", &bp);
> while (num != NULL) {
> if (virStrToLong_i(num, NULL, 10, &pos) < 0)
virBitmap* functions can be used instead.
Jan