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

[Daniel Veillard]

On Tue, Jul 19, 2011 at 12:19:34PM +0200, Matthias Bolte wrote:
> 2011/7/19 Daniel Veillard <veillard at redhat.com>:
> > On Tue, Jul 19, 2011 at 11:00:21AM +0200, 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;
> >>      }
> >
> >  The only problem I have is why is a simple "virsh list" exhibiting
> > one of those 2 errors, they sounds like hard cornercases, and not
> > something to be triggered in such a simple operation. Valgrind had to
> > emulate one of those code path to raise the error, and that sounds
> > weird.
> >
> > Daniel
> 
> I'm not sure that I understand what you mean. The FD leak that
> valgrind detected is there and expected as virsh (indirectly) calls
> virEventPollInit, but there is not counterpart to virEventPollInit to
> close those FDs again. This is an expected static leak, you're
> supposed to call virEventPollInit at most once in your program. So
> there is actually nothing to fix here.

  Yes, just that the patch was about the error paths, if we didn't go
through the error paths then fine, and yes a static leak is fine,
libxml2 has one too

> The patch I suggested is unrelated to the valfrind detected leak. In
> case virEventPollAddHandle fails we know that virEventPollInit will
> fail and that the calling app should fail too. In that case we can
> close the FDs. This is just an 'improvement' that's not strictly
> necessary. It just avoids the static leak in an error case.

  yeah, the program is likely to exit anyway, but cleaning this up
makes sense,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/