[libvirt] [PATCH 1/2] virTypedParamsSerialize: minor fixes

[John Ferlan]

On 04/25/2018 11:55 AM, Marc Hartmayer wrote:
> 1. Don't allocate if there is nothing that needs to be
>    allocated. Especially as the result of calling calloc(0, ...) is
>    implementation-defined.

Following VIR_ALLOC_N one finds :

VIR_ALLOC_N(params_val, nparams)

which equates to

# define VIR_ALLOC_N(ptr, count) \
         virAllocN(&(ptr), sizeof(*(ptr)), (count), true, \
                   VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__)

or

virAllocN(&params_val, sizeof(params_val), nparams, true, ...)

and eventually/essentially

*params_val = calloc(sizeof(params_val), nparams)

If the returned value is NULL then we error w/ OOM (4th param=true).

So, unless @params_val had no elements, it won't be calloc(0,...) and
thus the question becomes is there a more specific path you are
referencing here?

FWIW: My f26 man page for calloc says:

"The calloc() function allocates memory for an array of nmemb elements
of size bytes each and returns a pointer to the allocated memory. The
memory is set to zero.  If nmemb or size is 0, then calloc() returns
either NULL, or a unique pointer value that can later be successfully
passed to free()"

We have so many places in the code that use VIR_ALLOC_N and do not check
if the sizeof() or count is 0 - which makes me wonder even more about
the specific case in which you are referencing. I looked through the
various remote_protocol.x structures that would use this and it seems
they all use remote_typed_param for the structure being returned, so
it's not clear how any of them could have a sizeof() returning 0.

> 2. Update the length @remote_params_len only if the related
>    @remote_params_val has also been set.

This one changes the error case as the returned @remote_params_len
changes from being set to @params_len on input to be undefined.


John

> 
> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
> ---
>  src/util/virtypedparam.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c
> index 2452628cdbcd..10fd28baa65c 100644
> --- a/src/util/virtypedparam.c
> +++ b/src/util/virtypedparam.c
> @@ -1500,10 +1500,10 @@ virTypedParamsSerialize(virTypedParameterPtr params,
>      size_t i;
>      size_t j;
>      int rv = -1;
> -    virTypedParameterRemotePtr params_val;
> +    virTypedParameterRemotePtr params_val = NULL;
> +    int params_len = nparams;
>  
> -    *remote_params_len = nparams;
> -    if (VIR_ALLOC_N(params_val, nparams) < 0)
> +    if (nparams && VIR_ALLOC_N(params_val, nparams) < 0)
>          goto cleanup;
>  
>      for (i = 0, j = 0; i < nparams; ++i) {
> @@ -1515,7 +1515,7 @@ virTypedParamsSerialize(virTypedParameterPtr params,
>          if (!param->type ||
>              (!(flags & VIR_TYPED_PARAM_STRING_OKAY) &&
>               param->type == VIR_TYPED_PARAM_STRING)) {
> -            --*remote_params_len;
> +            --params_len;
>              continue;
>          }
>  
> @@ -1556,6 +1556,7 @@ virTypedParamsSerialize(virTypedParameterPtr params,
>      }
>  
>      *remote_params_val = params_val;
> +    *remote_params_len = params_len;
>      params_val = NULL;
>      rv = 0;
>  
>