[PATCH] backup-top: fix a memory leak in bdrv_backup_top_append()

pannengyuan@huawei.com posted 1 patch 4 years, 3 months ago
Test docker-quick@centos7 passed
Test docker-mingw@fedora passed
Test FreeBSD passed
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200120074725.22948-1-pannengyuan@huawei.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
block/backup-top.c | 1 +
1 file changed, 1 insertion(+)
[PATCH] backup-top: fix a memory leak in bdrv_backup_top_append()
Posted by pannengyuan@huawei.com 4 years, 3 months ago
From: Pan Nengyuan <pannengyuan@huawei.com>

top->opaque is aleardy malloced in bdrv_new_open_driver(), and then change
the pointer but without freeing it. It will cause a memory leak, the leak
stack is as follow:

Direct leak of 24 byte(s) in 1 object(s) allocated from:
  #0 0x7ff6f7be4970 (/lib64/libasan.so.5+0xef970)  ??:?
  #1 0x7ff6f723849d (/lib64/libglib-2.0.so.0+0x5249d)  ??:?
  #2 0x564c0d44caae (./x86_64-softmmu/qemu-system-x86_64+0x3b40aae)  /mnt/sdb/qemu/block.c:1289
  #3 0x564c0d44dbaf (./x86_64-softmmu/qemu-system-x86_64+0x3b41baf)  /mnt/sdb/qemu/block.c:1359
  #4 0x564c0d71618f (./x86_64-softmmu/qemu-system-x86_64+0x3e0a18f)  /mnt/sdb/qemu/block/backup-top.c:190
  #5 0x564c0d7001be (./x86_64-softmmu/qemu-system-x86_64+0x3df41be)  /mnt/sdb/qemu/block/backup.c:439
  #6 0x564c0c8ebef8 (./x86_64-softmmu/qemu-system-x86_64+0x2fdfef8)  /mnt/sdb/qemu/blockdev.c:3580
  #7 0x564c0c8ed0cb (./x86_64-softmmu/qemu-system-x86_64+0x2fe10cb)  /mnt/sdb/qemu/blockdev.c:3690
  #8 0x564c0c8ed177 (./x86_64-softmmu/qemu-system-x86_64+0x2fe1177)  /mnt/sdb/qemu/blockdev.c:3704
  #9 0x564c0d316388 (./x86_64-softmmu/qemu-system-x86_64+0x3a0a388)  /mnt/sdb/qemu/build/qapi/qapi-commands-block-core.c:439
  #10 0x564c0d7ff7fa (./x86_64-softmmu/qemu-system-x86_64+0x3ef37fa)  /mnt/sdb/qemu/qapi/qmp-dispatch.c:132
  #11 0x564c0d7ffcb8 (./x86_64-softmmu/qemu-system-x86_64+0x3ef3cb8)  /mnt/sdb/qemu/qapi/qmp-dispatch.c:175 (discriminator 4)
  #12 0x564c0d2704ef (./x86_64-softmmu/qemu-system-x86_64+0x39644ef)  /mnt/sdb/qemu/monitor/qmp.c:145
  #13 0x564c0d2712de (./x86_64-softmmu/qemu-system-x86_64+0x39652de)  /mnt/sdb/qemu/monitor/qmp.c:234 (discriminator 4)

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
 block/backup-top.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/backup-top.c b/block/backup-top.c
index 818d3f26b4..d565f05520 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -196,6 +196,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
     }
 
     top->total_sectors = source->total_sectors;
+    g_free(top->opaque);
     top->opaque = state = g_new0(BDRVBackupTopState, 1);
 
     bdrv_ref(target);
-- 
2.21.0.windows.1



Re: [PATCH] backup-top: fix a memory leak in bdrv_backup_top_append()
Posted by Vladimir Sementsov-Ogievskiy 4 years, 3 months ago
Hi Pan!

Better is to drop extra allocation. Correct patch already posted here:
https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg05067.html

20.01.2020 10:47, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> top->opaque is aleardy malloced in bdrv_new_open_driver(), and then change
> the pointer but without freeing it. It will cause a memory leak, the leak
> stack is as follow:
> 
> Direct leak of 24 byte(s) in 1 object(s) allocated from:
>    #0 0x7ff6f7be4970 (/lib64/libasan.so.5+0xef970)  ??:?
>    #1 0x7ff6f723849d (/lib64/libglib-2.0.so.0+0x5249d)  ??:?
>    #2 0x564c0d44caae (./x86_64-softmmu/qemu-system-x86_64+0x3b40aae)  /mnt/sdb/qemu/block.c:1289
>    #3 0x564c0d44dbaf (./x86_64-softmmu/qemu-system-x86_64+0x3b41baf)  /mnt/sdb/qemu/block.c:1359
>    #4 0x564c0d71618f (./x86_64-softmmu/qemu-system-x86_64+0x3e0a18f)  /mnt/sdb/qemu/block/backup-top.c:190
>    #5 0x564c0d7001be (./x86_64-softmmu/qemu-system-x86_64+0x3df41be)  /mnt/sdb/qemu/block/backup.c:439
>    #6 0x564c0c8ebef8 (./x86_64-softmmu/qemu-system-x86_64+0x2fdfef8)  /mnt/sdb/qemu/blockdev.c:3580
>    #7 0x564c0c8ed0cb (./x86_64-softmmu/qemu-system-x86_64+0x2fe10cb)  /mnt/sdb/qemu/blockdev.c:3690
>    #8 0x564c0c8ed177 (./x86_64-softmmu/qemu-system-x86_64+0x2fe1177)  /mnt/sdb/qemu/blockdev.c:3704
>    #9 0x564c0d316388 (./x86_64-softmmu/qemu-system-x86_64+0x3a0a388)  /mnt/sdb/qemu/build/qapi/qapi-commands-block-core.c:439
>    #10 0x564c0d7ff7fa (./x86_64-softmmu/qemu-system-x86_64+0x3ef37fa)  /mnt/sdb/qemu/qapi/qmp-dispatch.c:132
>    #11 0x564c0d7ffcb8 (./x86_64-softmmu/qemu-system-x86_64+0x3ef3cb8)  /mnt/sdb/qemu/qapi/qmp-dispatch.c:175 (discriminator 4)
>    #12 0x564c0d2704ef (./x86_64-softmmu/qemu-system-x86_64+0x39644ef)  /mnt/sdb/qemu/monitor/qmp.c:145
>    #13 0x564c0d2712de (./x86_64-softmmu/qemu-system-x86_64+0x39652de)  /mnt/sdb/qemu/monitor/qmp.c:234 (discriminator 4)
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
>   block/backup-top.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/block/backup-top.c b/block/backup-top.c
> index 818d3f26b4..d565f05520 100644
> --- a/block/backup-top.c
> +++ b/block/backup-top.c
> @@ -196,6 +196,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>       }
>   
>       top->total_sectors = source->total_sectors;
> +    g_free(top->opaque);
>       top->opaque = state = g_new0(BDRVBackupTopState, 1);
>   
>       bdrv_ref(target);
> 


-- 
Best regards,
Vladimir
Re: [PATCH] backup-top: fix a memory leak in bdrv_backup_top_append()
Posted by Pan Nengyuan 4 years, 3 months ago

On 1/20/2020 4:38 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi Pan!
> 
> Better is to drop extra allocation. Correct patch already posted here:
> https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg05067.html
> 

Yes, it's better.

Thanks.

> 20.01.2020 10:47, pannengyuan@huawei.com wrote:
>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>
>> top->opaque is aleardy malloced in bdrv_new_open_driver(), and then change
>> the pointer but without freeing it. It will cause a memory leak, the leak
>> stack is as follow:
>>
>> Direct leak of 24 byte(s) in 1 object(s) allocated from:
>>    #0 0x7ff6f7be4970 (/lib64/libasan.so.5+0xef970)  ??:?
>>    #1 0x7ff6f723849d (/lib64/libglib-2.0.so.0+0x5249d)  ??:?
>>    #2 0x564c0d44caae (./x86_64-softmmu/qemu-system-x86_64+0x3b40aae)  /mnt/sdb/qemu/block.c:1289
>>    #3 0x564c0d44dbaf (./x86_64-softmmu/qemu-system-x86_64+0x3b41baf)  /mnt/sdb/qemu/block.c:1359
>>    #4 0x564c0d71618f (./x86_64-softmmu/qemu-system-x86_64+0x3e0a18f)  /mnt/sdb/qemu/block/backup-top.c:190
>>    #5 0x564c0d7001be (./x86_64-softmmu/qemu-system-x86_64+0x3df41be)  /mnt/sdb/qemu/block/backup.c:439
>>    #6 0x564c0c8ebef8 (./x86_64-softmmu/qemu-system-x86_64+0x2fdfef8)  /mnt/sdb/qemu/blockdev.c:3580
>>    #7 0x564c0c8ed0cb (./x86_64-softmmu/qemu-system-x86_64+0x2fe10cb)  /mnt/sdb/qemu/blockdev.c:3690
>>    #8 0x564c0c8ed177 (./x86_64-softmmu/qemu-system-x86_64+0x2fe1177)  /mnt/sdb/qemu/blockdev.c:3704
>>    #9 0x564c0d316388 (./x86_64-softmmu/qemu-system-x86_64+0x3a0a388)  /mnt/sdb/qemu/build/qapi/qapi-commands-block-core.c:439
>>    #10 0x564c0d7ff7fa (./x86_64-softmmu/qemu-system-x86_64+0x3ef37fa)  /mnt/sdb/qemu/qapi/qmp-dispatch.c:132
>>    #11 0x564c0d7ffcb8 (./x86_64-softmmu/qemu-system-x86_64+0x3ef3cb8)  /mnt/sdb/qemu/qapi/qmp-dispatch.c:175 (discriminator 4)
>>    #12 0x564c0d2704ef (./x86_64-softmmu/qemu-system-x86_64+0x39644ef)  /mnt/sdb/qemu/monitor/qmp.c:145
>>    #13 0x564c0d2712de (./x86_64-softmmu/qemu-system-x86_64+0x39652de)  /mnt/sdb/qemu/monitor/qmp.c:234 (discriminator 4)
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>>   block/backup-top.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/block/backup-top.c b/block/backup-top.c
>> index 818d3f26b4..d565f05520 100644
>> --- a/block/backup-top.c
>> +++ b/block/backup-top.c
>> @@ -196,6 +196,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>>       }
>>   
>>       top->total_sectors = source->total_sectors;
>> +    g_free(top->opaque);
>>       top->opaque = state = g_new0(BDRVBackupTopState, 1);
>>   
>>       bdrv_ref(target);
>>
> 
>