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

On 07/19/2011 05:00 PM, Matthias Bolte wrote: > 2011/7/19<ajia(a)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 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list