[libvirt] [PATCH 1/6] Avoid Coverity FORWARD_NULL prior to strtok_r calls

[John Ferlan]

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 at 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
>