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@xxxxxxxxxx>
>> ---
>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list