On Fri, Dec 23, 2011 at 02:13:53PM -0700, Eric Blake wrote:
> 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@xxxxxxxxxx>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> 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;
> }
>
ACK to the Chrismas patch :-)
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