[libvirt] [PATCH] daemon: fix fd leak on libvirtd

[Eric Blake]

On 12/22/2011 01:28 AM, ajia at redhat.com wrote:
> From: Alex Jia <ajia at 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 at 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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111223/1a5934da/attachment-0001.sig>