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;
}
--
1.7.7.4
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list