[libvirt] [PATCH 11/14] xen: Resource resource leak with 'cpuset'

[John Ferlan]

On 01/09/2013 11:55 AM, Laine Stump wrote:
> (you duplicated "resource" in the subject line)
> 

Missed that one... Will fix.

> On 01/09/2013 09:54 AM, John Ferlan wrote:
>> Make cpuset local to the while loop and free it once done with it each
>> time through the loop.
>> ---
>>  src/xen/xend_internal.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
>> index 84a25e8..c6b800b 100644
>> --- a/src/xen/xend_internal.c
>> +++ b/src/xen/xend_internal.c
>> @@ -1113,7 +1113,6 @@ sexpr_to_xend_topology(const struct sexpr *root,
>>  {
>>      const char *nodeToCpu;
>>      const char *cur;
>> -    virBitmapPtr cpuset = NULL;
>>      int *cpuNums = NULL;
>>      int cell, cpu, nb_cpus;
>>      int n = 0;
>> @@ -1131,6 +1130,7 @@ sexpr_to_xend_topology(const struct sexpr *root,
>>  
>>      cur = nodeToCpu;
>>      while (*cur != 0) {
>> +        virBitmapPtr cpuset = NULL;
>>          /*
>>           * Find the next NUMA cell described in the xend output
>>           */
>> @@ -1152,8 +1152,10 @@ sexpr_to_xend_topology(const struct sexpr *root,
>>                  goto memory_error;
>>          } else {
>>              nb_cpus = virBitmapParse(cur, 'n', &cpuset, numCpus);
>> -            if (nb_cpus < 0)
>> +            if (nb_cpus < 0) {
>> +                virBitmapFree(cpuset);
> 
> This virBitmapFree() isn't necessary - virBitmapParse is guaranteed to
> have nothing allocated (and will set cpuset = NULL) if it fails.
> 

According to Coverity's analysis this may not be true since it's
"possible" to hit the "ret--" line (more than once) in virBitmapParse()
while hitting either "ret++" line less times returning a negative value
on the "success" path. The example Coverity had shows 6 passes through
the loop, 4 negatives, 1 positive, and 1 nothing.

Whether realistically this could be true, I am not sure.

How Coverity determined what the value of 'cpuSet' is a mystery as the
output I have doesn't show what's being used for parsing, just that we
go through the loop 6 times. Perhaps something like "^1,^2,^3,4,^5,^6"
where 1,2,3,4,5,6 pass the virBitmapIsSet() call changing the 'ret'
value to -3.


>>                  goto error;
>> +            }
>>          }
>>  
>>          for (n = 0, cpu = 0; cpu < numCpus; cpu++) {
>> @@ -1163,28 +1165,26 @@ sexpr_to_xend_topology(const struct sexpr *root,
>>              if (used)
>>                  cpuNums[n++] = cpu;
>>          }
>> +        virBitmapFree(cpuset);
>>  
>>          if (virCapabilitiesAddHostNUMACell(caps,
>>                                             cell,
>>                                             nb_cpus,
>>                                             cpuNums) < 0)
>>              goto memory_error;
>> +
>>      }
>>      VIR_FREE(cpuNums);
>> -    virBitmapFree(cpuset);
>>      return 0;
>>  
>>    parse_error:
>>      virReportError(VIR_ERR_XEN_CALL, "%s", _("topology syntax error"));
>>    error:
>>      VIR_FREE(cpuNums);
>> -    virBitmapFree(cpuset);
>> -
>>      return -1;
>>  
>>    memory_error:
>>      VIR_FREE(cpuNums);
>> -    virBitmapFree(cpuset);
>>      virReportOOMError();
>>      return -1;
>>  }
> 
> ACK with the above nits fixed.
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>