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

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> ---
>   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 <address@hidden>

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