On 12/22/2011 01:28 AM, ajia@redhat.com wrote:

From: Alex Jia <ajia@redhat.com>

The codes hasn't close a pipe decriptor statuspipe[0] before exiting,

s/decriptor/descriptor/

moreover, should also close all of opening fds on error path to avoid fd leaks.

In addition, I think other fds leak doesen't belong to libvirtd,

s/doesen't/doesn't/

so this patch hasn't fixed them.

Detected by valgrind. Leaks introduced in commit 4296cea.

* daemon/libvirtd.c: fix fd leak on libvirt daemon.

* How to reproduce? % service libvirtd stop % valgrind -v --track-fds=yes /usr/sbin/libvirtd --daemon

* Actual valgrind result:

==16804== FILE DESCRIPTORS: 7 open at exit. ==16804== Open file descriptor 7: ==16804== at 0x321FAD8B87: pipe (in /lib64/libc-2.12.so) ==16804== by 0x41F34D: daemonForkIntoBackground (libvirtd.c:186) ==16804== by 0x4207A0: main (libvirtd.c:1420)

fd's leaked at exit are sometimes the sign of a more serious bug lasting through the life of a program, but in this particular case, I'm not convinced. First, let's get a better picture of daemonForkIntoBackground. On success, this function calls fork() twice, meaning we are dealing with three processes: parent - fork intermediate child, wait for grandchild to exist, then exit intermediate child - prepare to daemonize, fork grandchild, then exit grandchild - now running as the daemon - return the fd On error cases, we must ensure that _exactly one_ process returns -1 to the caller, so that the caller can report the failure. However, note that this can be _any one_ of the three processes; with the implicit understanding that if we ever get past a successful fork(), then the process that does not return must exit without any further user-visible actions (not even an error message). That is, anywhere that we fail before fork(), the parent is responsible for returning an error, but anywhere after a fork(), the parent must call _exit().

Signed-off-by: Alex Jia <ajia@redhat.com> --- daemon/libvirtd.c | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index d7a03d7..b05f126 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -184,7 +184,7 @@ static int daemonForkIntoBackground(const char *argv0) { int statuspipe[2]; if (pipe(statuspipe) < 0) - return -1; + goto error;

This one's wrong. statuspipe is uninitialized, which means going to error here would risk closing an arbitrary fd.

int pid = fork(); switch (pid) { @@ -219,7 +219,7 @@ static int daemonForkIntoBackground(const char *argv0) case 0: return statuspipe[1]; case -1: - return -1; + goto error;

This one's wrong. It should go to cleanup, otherwise we leak stdinfd and stdoutfd.

default: _exit(0);

See below about _exit...

} @@ -232,7 +232,7 @@ static int daemonForkIntoBackground(const char *argv0) }

case -1: - return -1; + goto error;

This one's good.

default: { @@ -243,7 +243,7 @@ static int daemonForkIntoBackground(const char *argv0)

/* We wait to make sure the first child forked successfully */ if (virPidWait(pid, NULL) < 0) - return -1; + goto error;

This one's iffy both before and after your change - virPidWait returns -1 if the child had non-zero status, but in the child, we returned -1 instead of calling _exit(1), which means we end up with both sides of fork() returning to the caller, and thus double error output. The solution is to fix the child to exit with non-zero status, and transfer the error reporting burden to the parent in that case.

/* Now block until the second child initializes successfully */ again: @@ -257,9 +257,15 @@ static int daemonForkIntoBackground(const char *argv0) "--daemon for more info.\n"), argv0, virDaemonErrTypeToString(status)); } + VIR_FORCE_CLOSE(statuspipe[0]);

This one's pointless to all but valgrind, since _exit() will close it anyway (that is, we're leaking just before exit, but big deal - we know we're exiting; leaks are only bad if you keep on executing for a long time afterwards). But since cleaning it up makes valgrind analysis easier, I'm not opposed to it; however, that means we should also avoid the fd leak before the intermediate child calls _exit.

_exit(ret == 1 && status == 0 ? 0 : 1);

Ouch - we have an independent bug here. We did fprintf(), but not fflush(), and since we call _exit() instead of exit(), the error never gets printed! I'm proposing this as a more-complete v2. In the new control flow, we have the following possible paths: parent errors out before fork() -> return -1 with no leaks otherwise the parent fork()s, and: child errors out before fork() -> child calls _exit(1) and parent returns -1 child successfully calls fork() -> child calls _exit(0), grandchild returns fd, and: caller in grandchild detects error, so grandchild writes failure message to pipe before calling _exit, and parent calls exit(1) caller in grandchild writes success to pipe then continues, and parent calls exit(0) Oh, and while I was at it, I made things slightly more robust if you invoke libvirtd with stdin or stdout closed (a rare event, but easy enough to work around); we don't want to close our just-created stdinfd handed to the grandchild just because we happened to open("/dev/null") and get back a standard descriptor. v2 to follow. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org