On 12/22/2011 01:28 AM, ajia@xxxxxxxxxx wrote:
> From: Alex Jia <ajia@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list