Patchwork vdi: Fix memory leak

login
register
mail settings
Submitter Kevin Wolf
Date Nov. 22, 2011, 4:16 p.m.
Message ID <1321978604-4858-1-git-send-email-kwolf@redhat.com>
Download mbox | patch
Permalink /patch/127120/
State New
Headers show

Comments

Kevin Wolf - Nov. 22, 2011, 4:16 p.m.
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(-)
Stefan Weil - Nov. 22, 2011, 6:37 p.m.
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
Kevin Wolf - Nov. 23, 2011, 8:26 a.m.
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

Patch

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);
 }