On Tue, Jul 19, 2011 at 11:00:21AM +0200, Matthias Bolte wrote:
> 2011/7/19 <ajia@xxxxxxxxxx>:
> > * 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
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list