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

[Laine Stump]

On 01/09/2013 01:50 PM, John Ferlan wrote:
> 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.

I don't think that is possible. In order for virBitmapIsSet() to return
true for a particular bit, that bit must be set, and in order for that
bit to be set, it must have been set in a previous iteration of this
same loop (remember that the bitmap is initialized to all empty at the
top of the function), which means that ret++ must have been executed. So
ret-- can't happen without a previous corresponding ret++, therefore the
value of ret can't be < 0.

If it was possible to have a return < 0 on success, that would be a bug
in the function that would need to be fixed.