The @cpus variable is an array of structs in which each item
contains a virBitmap member. As such it is not enough to just
VIR_FREE() the array - each bitmap has to be freed too.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/capabilities.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 084e09286d..4d509a6d28 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -1648,6 +1648,7 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps)
 
  cleanup:
     virBitmapFree(cpumap);
+    virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus);
     VIR_FREE(cpus);
     VIR_FREE(siblings);
     VIR_FREE(pageinfo);
-- 
2.26.3

On 5/5/21 4:02 AM, Michal Privoznik wrote:
> The @cpus variable is an array of structs in which each item
> contains a virBitmap member. As such it is not enough to just
> VIR_FREE() the array - each bitmap has to be freed too.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>   src/conf/capabilities.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 084e09286d..4d509a6d28 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -1648,6 +1648,7 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA *caps)
>   
>    cleanup:
>       virBitmapFree(cpumap);
> +    virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus);

There's some coding axiom about for every bug you fix you introduce 
another ;-)...

Anyway Coverity notes you can get to cleanup from within the for loop 
when ncpus < 0 and that will not be very good for this call. Yes, -1 can 
no longer be returned, but -2 can be and we could fall to cleanup (at 
least theoretically).

Another tweak could be to only check -2 and continue in the if statement 
since -1 is no longer possible.

John

>       VIR_FREE(cpus);
>       VIR_FREE(siblings);
>       VIR_FREE(pageinfo);
> 

On 5/11/21 12:37 PM, John Ferlan wrote:
> 
> 
> On 5/5/21 4:02 AM, Michal Privoznik wrote:
>> The @cpus variable is an array of structs in which each item
>> contains a virBitmap member. As such it is not enough to just
>> VIR_FREE() the array - each bitmap has to be freed too.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   src/conf/capabilities.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>> index 084e09286d..4d509a6d28 100644
>> --- a/src/conf/capabilities.c
>> +++ b/src/conf/capabilities.c
>> @@ -1648,6 +1648,7 @@ virCapabilitiesHostNUMAInitReal(virCapsHostNUMA
>> *caps)
>>      cleanup:
>>       virBitmapFree(cpumap);
>> +    virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus);
> 
> There's some coding axiom about for every bug you fix you introduce
> another ;-)...
> 
> Anyway Coverity notes you can get to cleanup from within the for loop
> when ncpus < 0 and that will not be very good for this call. Yes, -1 can
> no longer be returned, but -2 can be and we could fall to cleanup (at
> least theoretically).
> 
> Another tweak could be to only check -2 and continue in the if statement
> since -1 is no longer possible.

Unless I'm missing something that's just theoretical issue. Because
virCapabilitiesClearHostNUMACellCPUTopology() does check for cpus ==
NULL and only if it is not NULL then it looks at ncpus. And I don't see
how cpus could be !NULL and ncpus < 0 at the same time. But let me see
if there's something I can do.

Michal