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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]