[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