[Qemu-devel] [PATCH] vdi: Fix memory leak

[Qemu-devel] [PATCH] vdi: Fix memory leak

[Kevin Wolf]

The block map is allocated in vdi_open, but was never freed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
Applies on top if the migration blocker series.

 block/vdi.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 7dda522..02da6b4 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -949,6 +949,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
 static void vdi_close(BlockDriverState *bs)
 {
     BDRVVdiState *s = bs->opaque;
+
+    g_free(s->bmap);
+
     migrate_del_blocker(s->migration_blocker);
     error_free(s->migration_blocker);
 }
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH] vdi: Fix memory leak

[Stefan Weil]

Am 22.11.2011 17:16, schrieb Kevin Wolf:
> The block map is allocated in vdi_open, but was never freed.
>
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> ---
> Applies on top if the migration blocker series.
>
>   block/vdi.c |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/block/vdi.c b/block/vdi.c
> index 7dda522..02da6b4 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -949,6 +949,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
>   static void vdi_close(BlockDriverState *bs)
>   {
>       BDRVVdiState *s = bs->opaque;
> +
> +    g_free(s->bmap);
> +
>       migrate_del_blocker(s->migration_blocker);
>       error_free(s->migration_blocker);
>   }

If vdi_close is called after a jump to label fail_free_bmap,
g_free(s->bmap) will be called twice.

Setting s->bmap = NULL after g_free in fail_free_bmap
would be safer.

Otherwise your patch is fine. If you send an update, you
can add

Reviewed-by: Stefan Weil <sw@weilnetz.de>

Regards,
Stefan Weil

Re: [Qemu-devel] [PATCH] vdi: Fix memory leak

[Kevin Wolf]

Am 22.11.2011 19:37, schrieb Stefan Weil:
> Am 22.11.2011 17:16, schrieb Kevin Wolf:
>> The block map is allocated in vdi_open, but was never freed.
>>
>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>> ---
>> Applies on top if the migration blocker series.
>>
>>   block/vdi.c |    3 +++
>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/vdi.c b/block/vdi.c
>> index 7dda522..02da6b4 100644
>> --- a/block/vdi.c
>> +++ b/block/vdi.c
>> @@ -949,6 +949,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
>>   static void vdi_close(BlockDriverState *bs)
>>   {
>>       BDRVVdiState *s = bs->opaque;
>> +
>> +    g_free(s->bmap);
>> +
>>       migrate_del_blocker(s->migration_blocker);
>>       error_free(s->migration_blocker);
>>   }
> 
> If vdi_close is called after a jump to label fail_free_bmap,
> g_free(s->bmap) will be called twice.

bdrv_close() may only ever be called on a successfully opened image.

In fact, block.c already ensures this. The BDRVVdiState would already be
freed in the failing bdrv_open_common() and bs->drv would be set to
NULL, so that an invalid bdrv_close() would end up doing nothing.

> Setting s->bmap = NULL after g_free in fail_free_bmap
> would be safer.
> 
> Otherwise your patch is fine. If you send an update, you
> can add
> 
> Reviewed-by: Stefan Weil <sw@weilnetz.de>

Thanks.

Kevin