[libvirt] [PATCH] util: avoid fds leak on virEventPollInit

[Alex Jia]

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==
>>
>> * 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