1. Patchew
  2. QEMU
  3. View series

[PATCH] block/backup: fix memory leak in bdrv_backup_top_append()

Eiichi Tsukata posted 1 patch 4 years, 4 months ago
Test asan failed
Test checkpatch failed
Test FreeBSD failed
Test docker-mingw@fedora failed
Test docker-clang@ubuntu failed
Test docker-quick@centos7 failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191223090632.30653-1-devel@etsukata.com
Maintainers: Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>
block/backup-top.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] block/backup: fix memory leak in bdrv_backup_top_append()
Posted by Eiichi Tsukata 4 years, 4 months ago 
bdrv_open_driver() allocates bs->opaque according to drv->instance_size.
There is no need to allocate it and overwrite opaque in
bdrv_backup_top_append().

Reproducer:

  $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 valgrind -q --leak-check=full tests/test-replication -p /replication/secondary/start
  ==29792== 24 bytes in 1 blocks are definitely lost in loss record 52 of 226
  ==29792==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
  ==29792==    by 0x4B07CE0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
  ==29792==    by 0x12BAB9: bdrv_open_driver (block.c:1289)
  ==29792==    by 0x12BEA9: bdrv_new_open_driver (block.c:1359)
  ==29792==    by 0x1D15CB: bdrv_backup_top_append (backup-top.c:190)
  ==29792==    by 0x1CC11A: backup_job_create (backup.c:439)
  ==29792==    by 0x1CD542: replication_start (replication.c:544)
  ==29792==    by 0x1401B9: replication_start_all (replication.c:52)
  ==29792==    by 0x128B50: test_secondary_start (test-replication.c:427)
  ...

Fixes: 7df7868b9640 ("block: introduce backup-top filter driver")
Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
---
 block/backup-top.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/backup-top.c b/block/backup-top.c
index 7cdb1f8eba..617217374d 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -196,7 +196,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
     }
 
     top->total_sectors = source->total_sectors;
-    top->opaque = state = g_new0(BDRVBackupTopState, 1);
+    state = top->opaque;
 
     bdrv_ref(target);
     state->target = bdrv_attach_child(top, target, "target", &child_file, errp);
-- 
2.21.0
Re: [PATCH] block/backup: fix memory leak in bdrv_backup_top_append()
Posted by Vladimir Sementsov-Ogievskiy 4 years, 4 months ago 
23.12.2019 12:06, Eiichi Tsukata wrote:
> bdrv_open_driver() allocates bs->opaque according to drv->instance_size.
> There is no need to allocate it and overwrite opaque in
> bdrv_backup_top_append().
> 
> Reproducer:
> 
>    $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 valgrind -q --leak-check=full tests/test-replication -p /replication/secondary/start
>    ==29792== 24 bytes in 1 blocks are definitely lost in loss record 52 of 226
>    ==29792==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
>    ==29792==    by 0x4B07CE0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
>    ==29792==    by 0x12BAB9: bdrv_open_driver (block.c:1289)
>    ==29792==    by 0x12BEA9: bdrv_new_open_driver (block.c:1359)
>    ==29792==    by 0x1D15CB: bdrv_backup_top_append (backup-top.c:190)
>    ==29792==    by 0x1CC11A: backup_job_create (backup.c:439)
>    ==29792==    by 0x1CD542: replication_start (replication.c:544)
>    ==29792==    by 0x1401B9: replication_start_all (replication.c:52)
>    ==29792==    by 0x128B50: test_secondary_start (test-replication.c:427)
>    ...
> 
> Fixes: 7df7868b9640 ("block: introduce backup-top filter driver")
> Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
> ---
>   block/backup-top.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/backup-top.c b/block/backup-top.c
> index 7cdb1f8eba..617217374d 100644
> --- a/block/backup-top.c
> +++ b/block/backup-top.c
> @@ -196,7 +196,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>       }
>   
>       top->total_sectors = source->total_sectors;
> -    top->opaque = state = g_new0(BDRVBackupTopState, 1);
> +    state = top->opaque;
>   
>       bdrv_ref(target);
>       state->target = bdrv_attach_child(top, target, "target", &child_file, errp);
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Hmm, it was not my idea, I just copied it from mirror.. And there should be the same leak. and
may be in other places:

# git grep 'opaque =.*g_new'
block/backup-top.c:    top->opaque = state = g_new0(BDRVBackupTopState, 1);
block/file-posix.c:    state->opaque = g_new0(BDRVRawReopenState, 1);
block/gluster.c:    state->opaque = g_new0(BDRVGlusterReopenState, 1);
block/iscsi.c:    bs->opaque = g_new0(struct IscsiLun, 1);
block/mirror.c:    bs_opaque = g_new0(MirrorBDSOpaque, 1);
block/raw-format.c:    reopen_state->opaque = g_new0(BDRVRawState, 1);
block/sheepdog.c:    re_s = state->opaque = g_new0(BDRVSheepdogReopenState, 1);



-- 
Best regards,
Vladimir
Re: [PATCH] block/backup: fix memory leak in bdrv_backup_top_append()
Posted by Eiichi Tsukata 4 years, 4 months ago 

On 2019/12/23 21:40, Vladimir Sementsov-Ogievskiy wrote:
> 23.12.2019 12:06, Eiichi Tsukata wrote:
>> bdrv_open_driver() allocates bs->opaque according to drv->instance_size.
>> There is no need to allocate it and overwrite opaque in
>> bdrv_backup_top_append().
>>
>> Reproducer:
>>
>>    $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 valgrind -q --leak-check=full tests/test-replication -p /replication/secondary/start
>>    ==29792== 24 bytes in 1 blocks are definitely lost in loss record 52 of 226
>>    ==29792==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
>>    ==29792==    by 0x4B07CE0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
>>    ==29792==    by 0x12BAB9: bdrv_open_driver (block.c:1289)
>>    ==29792==    by 0x12BEA9: bdrv_new_open_driver (block.c:1359)
>>    ==29792==    by 0x1D15CB: bdrv_backup_top_append (backup-top.c:190)
>>    ==29792==    by 0x1CC11A: backup_job_create (backup.c:439)
>>    ==29792==    by 0x1CD542: replication_start (replication.c:544)
>>    ==29792==    by 0x1401B9: replication_start_all (replication.c:52)
>>    ==29792==    by 0x128B50: test_secondary_start (test-replication.c:427)
>>    ...
>>
>> Fixes: 7df7868b9640 ("block: introduce backup-top filter driver")
>> Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
>> ---
>>   block/backup-top.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/backup-top.c b/block/backup-top.c
>> index 7cdb1f8eba..617217374d 100644
>> --- a/block/backup-top.c
>> +++ b/block/backup-top.c
>> @@ -196,7 +196,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>>       }
>>   
>>       top->total_sectors = source->total_sectors;
>> -    top->opaque = state = g_new0(BDRVBackupTopState, 1);
>> +    state = top->opaque;
>>   
>>       bdrv_ref(target);
>>       state->target = bdrv_attach_child(top, target, "target", &child_file, errp);
>>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Hmm, it was not my idea, I just copied it from mirror.. And there should be the same leak. and
> may be in other places:
> 
> # git grep 'opaque =.*g_new'
> block/backup-top.c:    top->opaque = state = g_new0(BDRVBackupTopState, 1);
> block/file-posix.c:    state->opaque = g_new0(BDRVRawReopenState, 1);
> block/gluster.c:    state->opaque = g_new0(BDRVGlusterReopenState, 1);
> block/iscsi.c:    bs->opaque = g_new0(struct IscsiLun, 1);
> block/mirror.c:    bs_opaque = g_new0(MirrorBDSOpaque, 1);
> block/raw-format.c:    reopen_state->opaque = g_new0(BDRVRawState, 1);
> block/sheepdog.c:    re_s = state->opaque = g_new0(BDRVSheepdogReopenState, 1);
> 
> 
> 

Thanks for reviewing.
As you say, block/mirror.c has similar code. But it does not cause the leak.
The difference is bdrv_mirror_top BlockDriver does not have .instance_size
whereas bdrv_backup_top_filter BlockDriver has .instance_size = sizeof(BDRVBackupTopState).
So when bdrv_open_driver() is called from mirror.c, g_malloc0(0) is
called allocating nothing.

Eiichi
Re: [PATCH] block/backup: fix memory leak in bdrv_backup_top_append()
Posted by Kevin Wolf 4 years, 3 months ago 
Am 23.12.2019 um 14:40 hat Eiichi Tsukata geschrieben:
> 
> 
> On 2019/12/23 21:40, Vladimir Sementsov-Ogievskiy wrote:
> > 23.12.2019 12:06, Eiichi Tsukata wrote:
> >> bdrv_open_driver() allocates bs->opaque according to drv->instance_size.
> >> There is no need to allocate it and overwrite opaque in
> >> bdrv_backup_top_append().
> >>
> >> Reproducer:
> >>
> >>    $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 valgrind -q --leak-check=full tests/test-replication -p /replication/secondary/start
> >>    ==29792== 24 bytes in 1 blocks are definitely lost in loss record 52 of 226
> >>    ==29792==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
> >>    ==29792==    by 0x4B07CE0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
> >>    ==29792==    by 0x12BAB9: bdrv_open_driver (block.c:1289)
> >>    ==29792==    by 0x12BEA9: bdrv_new_open_driver (block.c:1359)
> >>    ==29792==    by 0x1D15CB: bdrv_backup_top_append (backup-top.c:190)
> >>    ==29792==    by 0x1CC11A: backup_job_create (backup.c:439)
> >>    ==29792==    by 0x1CD542: replication_start (replication.c:544)
> >>    ==29792==    by 0x1401B9: replication_start_all (replication.c:52)
> >>    ==29792==    by 0x128B50: test_secondary_start (test-replication.c:427)
> >>    ...
> >>
> >> Fixes: 7df7868b9640 ("block: introduce backup-top filter driver")
> >> Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
> >> ---
> >>   block/backup-top.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/block/backup-top.c b/block/backup-top.c
> >> index 7cdb1f8eba..617217374d 100644
> >> --- a/block/backup-top.c
> >> +++ b/block/backup-top.c
> >> @@ -196,7 +196,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
> >>       }
> >>   
> >>       top->total_sectors = source->total_sectors;
> >> -    top->opaque = state = g_new0(BDRVBackupTopState, 1);
> >> +    state = top->opaque;
> >>   
> >>       bdrv_ref(target);
> >>       state->target = bdrv_attach_child(top, target, "target", &child_file, errp);
> >>
> > 
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > 
> > Hmm, it was not my idea, I just copied it from mirror.. And there should be the same leak. and
> > may be in other places:
> > 
> > # git grep 'opaque =.*g_new'
> > block/backup-top.c:    top->opaque = state = g_new0(BDRVBackupTopState, 1);

Fixed by this patch.

> > block/file-posix.c:    state->opaque = g_new0(BDRVRawReopenState, 1);
> > block/gluster.c:    state->opaque = g_new0(BDRVGlusterReopenState, 1);
> > block/raw-format.c:    reopen_state->opaque = g_new0(BDRVRawState, 1);
> > block/sheepdog.c:    re_s = state->opaque = g_new0(BDRVSheepdogReopenState, 1);

Doing this for reopen state is fine.

> > block/iscsi.c:    bs->opaque = g_new0(struct IscsiLun, 1);

This one looks kind of questionable. It basically builds its
BlockDriveState manually without using any of the block layer open
functions.

> > block/mirror.c:    bs_opaque = g_new0(MirrorBDSOpaque, 1);

Harmless as Eiichi explained below, but not nice either.

> Thanks for reviewing.
> As you say, block/mirror.c has similar code. But it does not cause the leak.
> The difference is bdrv_mirror_top BlockDriver does not have .instance_size
> whereas bdrv_backup_top_filter BlockDriver has .instance_size = sizeof(BDRVBackupTopState).
> So when bdrv_open_driver() is called from mirror.c, g_malloc0(0) is
> called allocating nothing.

I think it should still be changed just because it would make the code
cleaner. It's always better to use common infrastructure than
reimplementing it locally.

Kevin
Re: [PATCH] block/backup: fix memory leak in bdrv_backup_top_append()
Posted by Eiichi Tsukata 4 years, 3 months ago 
ping

On 2019/12/23 18:06, Eiichi Tsukata wrote:
> bdrv_open_driver() allocates bs->opaque according to drv->instance_size.
> There is no need to allocate it and overwrite opaque in
> bdrv_backup_top_append().
> 
> Reproducer:
> 
>   $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 valgrind -q --leak-check=full tests/test-replication -p /replication/secondary/start
>   ==29792== 24 bytes in 1 blocks are definitely lost in loss record 52 of 226
>   ==29792==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
>   ==29792==    by 0x4B07CE0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
>   ==29792==    by 0x12BAB9: bdrv_open_driver (block.c:1289)
>   ==29792==    by 0x12BEA9: bdrv_new_open_driver (block.c:1359)
>   ==29792==    by 0x1D15CB: bdrv_backup_top_append (backup-top.c:190)
>   ==29792==    by 0x1CC11A: backup_job_create (backup.c:439)
>   ==29792==    by 0x1CD542: replication_start (replication.c:544)
>   ==29792==    by 0x1401B9: replication_start_all (replication.c:52)
>   ==29792==    by 0x128B50: test_secondary_start (test-replication.c:427)
>   ...
> 
> Fixes: 7df7868b9640 ("block: introduce backup-top filter driver")
> Signed-off-by: Eiichi Tsukata <devel@etsukata.com>
> ---
>  block/backup-top.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/backup-top.c b/block/backup-top.c
> index 7cdb1f8eba..617217374d 100644
> --- a/block/backup-top.c
> +++ b/block/backup-top.c
> @@ -196,7 +196,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>      }
>  
>      top->total_sectors = source->total_sectors;
> -    top->opaque = state = g_new0(BDRVBackupTopState, 1);
> +    state = top->opaque;
>  
>      bdrv_ref(target);
>      state->target = bdrv_attach_child(top, target, "target", &child_file, errp);
>
Re: [PATCH] block/backup: fix memory leak in bdrv_backup_top_append()
Posted by Kevin Wolf 4 years, 3 months ago 
Am 23.12.2019 um 10:06 hat Eiichi Tsukata geschrieben:
> bdrv_open_driver() allocates bs->opaque according to drv->instance_size.
> There is no need to allocate it and overwrite opaque in
> bdrv_backup_top_append().
> 
> Reproducer:
> 
>   $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 valgrind -q --leak-check=full tests/test-replication -p /replication/secondary/start
>   ==29792== 24 bytes in 1 blocks are definitely lost in loss record 52 of 226
>   ==29792==    at 0x483AB1A: calloc (vg_replace_malloc.c:762)
>   ==29792==    by 0x4B07CE0: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.7)
>   ==29792==    by 0x12BAB9: bdrv_open_driver (block.c:1289)
>   ==29792==    by 0x12BEA9: bdrv_new_open_driver (block.c:1359)
>   ==29792==    by 0x1D15CB: bdrv_backup_top_append (backup-top.c:190)
>   ==29792==    by 0x1CC11A: backup_job_create (backup.c:439)
>   ==29792==    by 0x1CD542: replication_start (replication.c:544)
>   ==29792==    by 0x1401B9: replication_start_all (replication.c:52)
>   ==29792==    by 0x128B50: test_secondary_start (test-replication.c:427)
>   ...
> 
> Fixes: 7df7868b9640 ("block: introduce backup-top filter driver")
> Signed-off-by: Eiichi Tsukata <devel@etsukata.com>

Thanks, applied to the block layer.

Kevin

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.