[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Memory leak in transfer_memory_block()?
From: |
Markus Armbruster |
Subject: |
Re: Memory leak in transfer_memory_block()? |
Date: |
Mon, 22 Jun 2020 10:23:01 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>> -----Original Message-----
>> From: Markus Armbruster [mailto:armbru@redhat.com]
>> Sent: Thursday, June 18, 2020 1:36 PM
>> To: Zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Cc: qemu-devel@nongnu.org; Michael Roth <mdroth@linux.vnet.ibm.com>
>> Subject: Memory leak in transfer_memory_block()?
>>
>> We appear to leak an Error object when ga_read_sysfs_file() fails with
>> errno != ENOENT unless caller passes true @sys2memblk:
>>
>> static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool
>> sys2memblk,
>> GuestMemoryBlockResponse
>> *result,
>> Error **errp)
>> {
>> [...]
>> if (local_err) {
>>
>> We have an Error object.
>>
>> /* treat with sysfs file that not exist in old kernel */
>> if (errno == ENOENT) {
>>
>> Case 1: ENOENT; we free it. Good.
>>
>> error_free(local_err);
>> if (sys2memblk) {
>> mem_blk->online = true;
>> mem_blk->can_offline = false;
>> } else if (!mem_blk->online) {
>> result->response =
>>
>> GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_NOT_SUPPORTED;
>> }
>> } else {
>>
>> Case 2: other than ENOENT
>>
>> if (sys2memblk) {
>>
>> Case 2a: sys2memblk; we pass it to the caller. Good.
>>
>> error_propagate(errp, local_err);
>> } else {
>>
>> Case 2b: !sys2memblk; ???
>>
>
> Good catch! I think we should pass the error info back to the caller,
> Let's record this error for debug when it happens.
I'll post a minimal fix to plug the leak, cc'ing you. I'll gladly
replace it by a patch that does more. Thanks!
>> result->response =
>>
>> GUEST_MEMORY_BLOCK_RESPONSE_TYPE_OPERATION_FAILED;
>> }
>> }
>> goto out2;
>> }
>> [...]
>> out2:
>> g_free(status);
>> close(dirfd);
>> out1:
>> if (!sys2memblk) {
>> result->has_error_code = true;
>> result->error_code = errno;
>> }
>> }
>>
>> What is supposed to be done with @local_err in case 2b?