collect_image_check() is called twice in img_check(), the filename/format will be alloced without free the original memory.
It is not a big deal since the process will exit anyway, but seems like a clean code and it will remove the warning spotted by asan.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
 qemu-img.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 2b4562b9d9..bcbca6c9a2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -638,6 +638,8 @@ static int collect_image_check(BlockDriverState *bs,
         return ret;
     }
 
+    g_free(check->filename);
+    g_free(check->format);
     check->filename                 = g_strdup(filename);
     check->format                   = g_strdup(bdrv_get_format_name(bs));
     check->check_errors             = result.check_errors;
-- 
2.18.2

On 26.02.20 04:30, Pan Nengyuan wrote:
> collect_image_check() is called twice in img_check(), the filename/format will be alloced without free the original memory.
> It is not a big deal since the process will exit anyway, but seems like a clean code and it will remove the warning spotted by asan.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
>  qemu-img.c | 2 ++
>  1 file changed, 2 insertions(+)

I think this should happen in the caller.  And there I think it would
make more sense to completely discard the old object and allocate a new one:

qapi_free_ImageCheck(check);
check = g_new0(ImageCheck, 1);

This way, we can’t forget to free any fields if new pointers were to be
added to the ImageCheck object.

Max

On 2/26/2020 6:13 PM, Max Reitz wrote:
> On 26.02.20 04:30, Pan Nengyuan wrote:
>> collect_image_check() is called twice in img_check(), the filename/format will be alloced without free the original memory.
>> It is not a big deal since the process will exit anyway, but seems like a clean code and it will remove the warning spotted by asan.
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>>  qemu-img.c | 2 ++
>>  1 file changed, 2 insertions(+)
> 
> I think this should happen in the caller.  And there I think it would
> make more sense to completely discard the old object and allocate a new one:
> 
> qapi_free_ImageCheck(check);
> check = g_new0(ImageCheck, 1);
> 
> This way, we can’t forget to free any fields if new pointers were to be
> added to the ImageCheck object.

Good idea, thanks.

> 
> Max
>