* Memory leak in transfer_memory_block()?
@ 2020-06-18 5:35 Markus Armbruster
2020-06-18 6:49 ` Zhanghailiang
0 siblings, 1 reply; 3+ messages in thread
From: Markus Armbruster @ 2020-06-18 5:35 UTC (permalink / raw)
To: Hailiang Zhang; +Cc: qemu-devel, Michael Roth
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; ???
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?
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: Memory leak in transfer_memory_block()?
2020-06-18 5:35 Memory leak in transfer_memory_block()? Markus Armbruster
@ 2020-06-18 6:49 ` Zhanghailiang
2020-06-22 8:23 ` Markus Armbruster
0 siblings, 1 reply; 3+ messages in thread
From: Zhanghailiang @ 2020-06-18 6:49 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
> -----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.
> 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?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Memory leak in transfer_memory_block()?
2020-06-18 6:49 ` Zhanghailiang
@ 2020-06-22 8:23 ` Markus Armbruster
0 siblings, 0 replies; 3+ messages in thread
From: Markus Armbruster @ 2020-06-22 8:23 UTC (permalink / raw)
To: Zhanghailiang; +Cc: qemu-devel, Michael Roth
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?
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-22 8:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-18 5:35 Memory leak in transfer_memory_block()? Markus Armbruster
2020-06-18 6:49 ` Zhanghailiang
2020-06-22 8:23 ` Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).