virBitmapFormat returns the string that should be freed.

All strings in three ADD_BITMAP calls in qemuDomainGetGuestVcpusParams
are contained in tmp. So memory leak is possible here without VIR_FREE.

Fixes: 3e7db8d3e8 ("Remove backslash alignment attempts")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
 src/qemu/qemu_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9e0f204e44..a70bfb6450 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18420,6 +18420,7 @@ qemuDomainGetGuestVcpusParams(virTypedParameterPtr *params,
         goto cleanup; \
     if (virTypedParamsAddString(&par, &npar, &maxpar, #name, tmp) < 0) \
         goto cleanup; \
+    VIR_FREE(tmp); \
 
     ADD_BITMAP(vcpus);
     ADD_BITMAP(online);
-- 
2.30.2

On 9/28/23 13:55, Anastasia Belova wrote:
> virBitmapFormat returns the string that should be freed.
> 
> All strings in three ADD_BITMAP calls in qemuDomainGetGuestVcpusParams
> are contained in tmp. So memory leak is possible here without VIR_FREE.
> 
> Fixes: 3e7db8d3e8 ("Remove backslash alignment attempts")

Nothing in that commit suggests the VIR_FREE() was removed. In fact,
digging in our history it was commit
0108deb944af5ca6f1da350c9d0352c8ed18738b which removed the call.

Nit pick: I dislike short hashes because they are not unique. For
instance, this short has is 10 characters, but as our history grows
it'll need to grow too, otherwise it won't be unique. What is unique
though (and both human and machine readable) is: 'git describe --tags'
and if it's older commit then 'git describe --tags --contains'.

But the purpose of 'Fixes' in our commit messages is to help downstream
maintainers. For instance, when I'd be cherry picking that defect
commit, plain search through git log shows what other commits fix it.
Another reason against short hashes.

> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> ---
>  src/qemu/qemu_driver.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9e0f204e44..a70bfb6450 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18420,6 +18420,7 @@ qemuDomainGetGuestVcpusParams(virTypedParameterPtr *params,
>          goto cleanup; \
>      if (virTypedParamsAddString(&par, &npar, &maxpar, #name, tmp) < 0) \
>          goto cleanup; \
> +    VIR_FREE(tmp); \

Nice catch!

We can use this opportunity to:

1) drop the trailing '\' as we don't really need the macro continue on
the next (empty) line,

2) drop the trailing ';' so that ...

>  
>      ADD_BITMAP(vcpus);
>      ADD_BITMAP(online);

... these calls can retain their semicolons (and in fact enforce them).

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

and pushed.

Michal

On a Friday in 2023, Michal Prívozník wrote:
>On 9/28/23 13:55, Anastasia Belova wrote:
>> virBitmapFormat returns the string that should be freed.
>>
>> All strings in three ADD_BITMAP calls in qemuDomainGetGuestVcpusParams
>> are contained in tmp. So memory leak is possible here without VIR_FREE.
>>
>> Fixes: 3e7db8d3e8 ("Remove backslash alignment attempts")
>
>Nothing in that commit suggests the VIR_FREE() was removed. In fact,
>digging in our history it was commit
>0108deb944af5ca6f1da350c9d0352c8ed18738b which removed the call.
>
>Nit pick: I dislike short hashes because they are not unique. For
>instance, this short has is 10 characters, but as our history grows
>it'll need to grow too, otherwise it won't be unique. What is unique
>though (and both human and machine readable) is: 'git describe --tags'
>and if it's older commit then 'git describe --tags --contains'.
>
>But the purpose of 'Fixes' in our commit messages is to help downstream
>maintainers. For instance, when I'd be cherry picking that defect
>commit, plain search through git log shows what other commits fix it.
>Another reason against short hashes.
>
>> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
>> ---
>>  src/qemu/qemu_driver.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 9e0f204e44..a70bfb6450 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -18420,6 +18420,7 @@ qemuDomainGetGuestVcpusParams(virTypedParameterPtr *params,
>>          goto cleanup; \
>>      if (virTypedParamsAddString(&par, &npar, &maxpar, #name, tmp) < 0) \
>>          goto cleanup; \
>> +    VIR_FREE(tmp); \
>

tmp is declared as 'g_autofree'. Generally we try to avoid mixing
autofree and manual memory freeing.

But from the functional point of view, it does not matter.

Jano

>Nice catch!
>
>We can use this opportunity to:
>
>1) drop the trailing '\' as we don't really need the macro continue on
>the next (empty) line,
>
>2) drop the trailing ';' so that ...
>
>>
>>      ADD_BITMAP(vcpus);
>>      ADD_BITMAP(online);
>
>... these calls can retain their semicolons (and in fact enforce them).
>
>Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>
>and pushed.
>
>Michal
>