[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]