Re: [PATCH 08/13 v5] libxl: don't leak ptr in libxl_list_vm error case

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 03/12/13 10:21, Ian Campbell wrote:
> On Tue, 2013-12-03 at 14:29 +1300, Matthew Daley wrote:
>> While at it, tidy up the function; there's no point in allocating more
>> than the amount of domains actually returned by xc_domain_getinfolist
>> (barring the caveat described in the newly-added comment)
>>
>> Coverity-ID: 1055888
>> Signed-off-by: Matthew Daley <mattd@bugfuzz.com>
>> ---
>> v5: Use libxl__calloc instead of calloc
>>
>>  tools/libxl/libxl.c |   27 +++++++++++++++++----------
>>  1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 67a8e0e..3b73d99 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -671,20 +671,24 @@ out:
>>   * be an aggregate of multiple domains. */
>>  libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int *nb_vm_out)
>>  {
>> -    libxl_vminfo *ptr;
>> +    GC_INIT(ctx);
>> +    libxl_vminfo *ptr = NULL;
>>      int idx, i, ret;
>>      xc_domaininfo_t info[1024];
>> -    int size = 1024;
>>  
>> -    ptr = calloc(size, sizeof(libxl_vminfo));
>> -    if (!ptr)
>> -        return NULL;
>> -
>> -    ret = xc_domain_getinfolist(ctx->xch, 1, 1024, info);
>> -    if (ret<0) {
>> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "geting domain info list");
>> -        return NULL;
>> +    ret = xc_domain_getinfolist(ctx->xch, 1, ARRAY_SIZE(info), info);
>> +    if (ret < 0) {
>> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
>> +        goto out;
>>      }
>> +
>> +    /*
>> +     * Always make sure to allocate at least one element; if we don't and we
>> +     * request zero, libxl__calloc (might) think its internal call to calloc
>> +     * has failed (if it returns null), if so it would kill our process.
> Is size==0 something we could/should handle in our libxl__*alloc
> wrappers?
>
> Or maybe this is something we should handle here e.g. by returning NULL,
> except perhaps our API doesn't allow for that?

The current API means that returning NULL from here constitutes a
failure, which needs to be distinct from "I did what you asked and there
are no domains".

*nb_vm_out is a second return parameter from this function.

~Andrew

>
>> +     */
>> +    ptr = libxl__calloc(NOGC, ret ? ret : 1, sizeof(libxl_vminfo));
>> +
>>      for (idx = i = 0; i < ret; i++) {
>>          if (libxl_is_stubdom(ctx, info[i].domain, NULL))
>>              continue;
>> @@ -694,6 +698,9 @@ libxl_vminfo * libxl_list_vm(libxl_ctx *ctx, int *nb_vm_out)
>>          idx++;
>>      }
>>      *nb_vm_out = idx;
>> +
>> +out:
>> +    GC_FREE;
>>      return ptr;
>>  }
>>  
>