Re: Memory leak in transfer_memory_block()?

[Markus Armbruster]

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?