On 12/22/2011 01:28 AM, ajia(a)redhat.com wrote: s/decriptor/descriptor/ s/doesen't/doesn't/ 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(). This one's wrong. statuspipe is uninitialized, which means going to error here would risk closing an arbitrary fd. This one's wrong. It should go to cleanup, otherwise we leak stdinfd and stdoutfd. See below about _exit... This one's good. 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. 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. 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Eric, thanks a lot, I think you're enjoying holiday, but you're always busy :-), Merry Christmas! Merry Christmas to everyone! Alex ----- Original Message ----- From: "Eric Blake" <eblake(a)redhat.com&gt; To: libvir-list(a)redhat.com Cc: "Alex Jia" <ajia(a)redhat.com&gt; Sent: Saturday, December 24, 2011 5:13:53 AM Subject: [PATCHv2] daemon: clean up daemonization Valgrind detected a pipe fd leak before the parent exits on success, introduced in commit 4296cea; by itself, the leak is not bad, since we immediately called _exit(), but we might as well be clean to make valgrind analysis easier. Meanwhile, if the daemon grandchild detects an error, the parent failed to flush the error message before exiting. Also, we had the possibility of both parent and child returning to the caller, such that the user could see duplicated reports of failure from the two return paths. And we might as well be robust to the (unlikely) situation of being started with stdin closed. * daemon/libvirtd.c (daemonForkIntoBackground): Use exit if an error message was generated, avoid fd leaks for valgrind's sake, avoid returning to caller in both parent and child, and don't close a just-dup'd stdin. Based on a report by Alex Jia. * 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) Signed-off-by: Alex Jia <ajia(a)redhat.com&gt; Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- daemon/libvirtd.c | 50 ++++++++++++++++++++++++++++++++++---------------- 1 files changed, 34 insertions(+), 16 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index d7a03d7..f52e73f 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -190,6 +190,7 @@ static int daemonForkIntoBackground(const char *argv0) switch (pid) { case 0: { + /* intermediate child */ int stdinfd = -1; int stdoutfd = -1; int nextpid; @@ -206,9 +207,9 @@ static int daemonForkIntoBackground(const char *argv0) goto cleanup; if (dup2(stdoutfd, STDERR_FILENO) != STDERR_FILENO) goto cleanup; - if (VIR_CLOSE(stdinfd) < 0) + if (stdinfd > STDERR_FILENO && VIR_CLOSE(stdinfd) < 0) goto cleanup; - if (VIR_CLOSE(stdoutfd) < 0) + if (stdoutfd > STDERR_FILENO && VIR_CLOSE(stdoutfd) < 0) goto cleanup; if (setsid() < 0) @@ -216,26 +217,28 @@ static int daemonForkIntoBackground(const char *argv0) nextpid = fork(); switch (nextpid) { - case 0: + case 0: /* grandchild */ return statuspipe[1]; - case -1: - return -1; - default: - _exit(0); + case -1: /* error */ + goto cleanup; + default: /* intermediate child succeeded */ + _exit(EXIT_SUCCESS); } cleanup: VIR_FORCE_CLOSE(stdoutfd); VIR_FORCE_CLOSE(stdinfd); - return -1; + VIR_FORCE_CLOSE(statuspipe[1]); + _exit(EXIT_FAILURE); } - case -1: - return -1; + case -1: /* error in parent */ + goto error; default: { + /* parent */ int ret; char status; @@ -243,23 +246,38 @@ 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; - /* Now block until the second child initializes successfully */ + /* If we get here, then the grandchild was spawned, so we + * must exit. Block until the second child initializes + * successfully */ again: ret = read(statuspipe[0], &status, 1); if (ret == -1 && errno == EINTR) goto again; - if (ret == 1 && status != 0) { + VIR_FORCE_CLOSE(statuspipe[0]); + + if (ret != 1) { + fprintf(stderr, + _("%s: error: unable to determine if daemon is " + "running: %s\n"), argv0, strerror(errno)); + exit(EXIT_FAILURE); + } else if (status != 0) { fprintf(stderr, - _("%s: error: %s. Check /var/log/messages or run without " - "--daemon for more info.\n"), argv0, + _("%s: error: %s. Check /var/log/messages or run " + "without --daemon for more info.\n"), argv0, virDaemonErrTypeToString(status)); + exit(EXIT_FAILURE); } - _exit(ret == 1 && status == 0 ? 0 : 1); + _exit(EXIT_SUCCESS); } } + +error: + VIR_FORCE_CLOSE(statuspipe[0]); + VIR_FORCE_CLOSE(statuspipe[1]); + return -1; } -- 1.7.7.4