[libvirt] [PATCH] util: avoid fds leak on virEventPollInit
Alex Jia
ajia at redhat.com
Tue Jul 19 09:24:13 UTC 2011
On 07/19/2011 05:12 PM, Alex Jia wrote:
> On 07/19/2011 05:00 PM, Matthias Bolte wrote:
>> 2011/7/19<ajia at redhat.com>:
>>> * src/util/event_poll.c: fix file descriptors leak on virEventPollInit.
>>>
>>> Detected in valgrind run:
>>> ==1254==
>>> ==1254== FILE DESCRIPTORS: 6 open at exit.
>>> ==1254== Open file descriptor 5:
>>> ==1254== at 0x30736D9D47: pipe2 (syscall-template.S:82)
>>> ==1254== by 0x4DD6267: rpl_pipe2 (pipe2.c:61)
>>> ==1254== by 0x4C4C1C5: virEventPollInit (event_poll.c:648)
>>> ==1254== by 0x4C4AA94: virEventRegisterDefaultImpl (event.c:208)
>>> ==1254== by 0x42150C: main (virsh.c:13790)
>>> ==1254==
>>> ==1254== Open file descriptor 4:
>>> ==1254== at 0x30736D9D47: pipe2 (syscall-template.S:82)
>>> ==1254== by 0x4DD6267: rpl_pipe2 (pipe2.c:61)
>>> ==1254== by 0x4C4C1C5: virEventPollInit (event_poll.c:648)
>>> ==1254== by 0x4C4AA94: virEventRegisterDefaultImpl (event.c:208)
>>> ==1254== by 0x42150C: main (virsh.c:13790)
>>> ==1254==
Hi Matthias,
In fact, if everything is okay, we will always open the above 2 fds, I
think it should be a expected result
based on your comments, right?
Thanks,
Alex
>>>
>>> * how to reproduce?
>>> % valgrind -v --track-fds=yes virsh list
>>>
>>> ---
>>> src/util/event_poll.c | 13 ++++++++++---
>>> 1 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/util/event_poll.c b/src/util/event_poll.c
>>> index 285ba50..1e4ef96 100644
>>> --- a/src/util/event_poll.c
>>> +++ b/src/util/event_poll.c
>>> @@ -639,6 +639,8 @@ static void virEventPollHandleWakeup(int watch
>>> ATTRIBUTE_UNUSED,
>>>
>>> int virEventPollInit(void)
>>> {
>>> + int ret = -1;
>>> +
>>> if (virMutexInit(&eventLoop.lock)< 0) {
>>> virReportSystemError(errno, "%s",
>>> _("Unable to initialize mutex"));
>>> @@ -648,7 +650,7 @@ int virEventPollInit(void)
>>> if (pipe2(eventLoop.wakeupfd, O_CLOEXEC | O_NONBLOCK)< 0) {
>>> virReportSystemError(errno, "%s",
>>> _("Unable to setup wakeup pipe"));
>>> - return -1;
>>> + goto cleanup;
>>> }
>>>
>>> if (virEventPollAddHandle(eventLoop.wakeupfd[0],
>>> @@ -657,10 +659,15 @@ int virEventPollInit(void)
>>> virEventError(VIR_ERR_INTERNAL_ERROR,
>>> _("Unable to add handle %d to event loop"),
>>> eventLoop.wakeupfd[0]);
>>> - return -1;
>>> + goto cleanup;
>>> }
>>>
>>> - return 0;
>>> + ret = 0;
>>> +
>>> +cleanup:
>>> + close(eventLoop.wakeupfd[0]);
>>> + close(eventLoop.wakeupfd[1]);
>>> + return ret;
>>> }
>> NACK, as this is wrong. You're closing the pipe in all cases in
>> virEventPollInit, but the pipe is supposed to be used afterwards. As
>> there is no virEventPollDeinit this FD leak has to be considered as a
>> static leak that's expected and not to be fixed. Also close() is
>> forbidden and make syntax-check should have told you that.
>>
>> What can be done here is closing the pipe in case
>> virEventPollAddHandle fails, for example like this:
>>
>> diff --git a/src/util/event_poll.c b/src/util/event_poll.c
>> index 285ba50..a62f960 100644
>> --- a/src/util/event_poll.c
>> +++ b/src/util/event_poll.c
>> @@ -657,6 +657,8 @@ int virEventPollInit(void)
>> virEventError(VIR_ERR_INTERNAL_ERROR,
>> _("Unable to add handle %d to event loop"),
>> eventLoop.wakeupfd[0]);
>> + VIR_CLOSE(eventLoop.wakeupfd[0]);
>> + VIR_CLOSE(eventLoop.wakeupfd[1]);
>> return -1;
>> }
>>
> Yeah, only fix the above section you mentioned instead.
>
> Thanks,
> Alex
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list