Re: [Qemu-devel] [PATCH v2] Don't leak file descriptors

[Alexander Graf <agraf@suse.de>]

Kevin Wolf wrote:
> We're leaking file descriptors to child processes. Set FD_CLOEXEC on file
> descriptors that don't need to be passed to children to stop this misbehaviour.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/raw-posix.c  |    2 +-
>  gdbstub.c          |    6 +++
>  kvm-all.c          |    2 +-
>  migration-tcp.c    |    6 +-
>  migration-unix.c   |    6 +-
>  net.c              |    8 ++--
>  osdep.c            |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  posix-aio-compat.c |    2 +-
>  qemu-char.c        |    8 ++--
>  qemu-common.h      |    7 +++
>  qemu-sockets.c     |   10 ++--
>  qemu_socket.h      |    2 +
>  slirp/misc.c       |    4 +-
>  slirp/slirp.h      |    4 ++
>  slirp/socket.c     |    2 +-
>  slirp/tcp_subr.c   |    2 +-
>  slirp/udp.c        |    4 +-
>  vl.c               |   11 +++--
>  vnc.c              |    2 +-
>  19 files changed, 158 insertions(+), 34 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index f558976..0ee8ff7 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -153,7 +153,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
>          s->open_flags |= O_DSYNC;
>  
>      s->fd = -1;
> -    fd = open(filename, s->open_flags, 0644);
> +    fd = qemu_open(filename, s->open_flags, 0644);
>      if (fd < 0) {
>          ret = -errno;
>          if (ret == -EROFS)
> diff --git a/gdbstub.c b/gdbstub.c
> index 055093f..5a4f7d4 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2356,6 +2356,9 @@ static void gdb_accept(void)
>              perror("accept");
>              return;
>          } else if (fd >= 0) {
> +#ifndef _WIN32
> +            fcntl(fd, F_SETFD, FD_CLOEXEC);
> +#endif
>              break;
>          }
>      }
> @@ -2385,6 +2388,9 @@ static int gdbserver_open(int port)
>          perror("socket");
>          return -1;
>      }
> +#ifndef _WIN32
> +    fcntl(fd, F_SETFD, FD_CLOEXEC);
> +#endif
>  
>      /* allow fast reuse */
>      val = 1;
> diff --git a/kvm-all.c b/kvm-all.c
> index 1916ec6..fe6220c 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -414,7 +414,7 @@ int kvm_init(int smp_cpus)
>          s->slots[i].slot = i;
>  
>      s->vmfd = -1;
> -    s->fd = open("/dev/kvm", O_RDWR);
> +    s->fd = qemu_open("/dev/kvm", O_RDWR);
>      if (s->fd == -1) {
>          fprintf(stderr, "Could not access KVM kernel module: %m\n");
>          ret = -errno;
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 9ed92b4..dc8772c 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -99,7 +99,7 @@ MigrationState *tcp_start_outgoing_migration(const char *host_port,
>      s->state = MIG_STATE_ACTIVE;
>      s->mon_resume = NULL;
>      s->bandwidth_limit = bandwidth_limit;
> -    s->fd = socket(PF_INET, SOCK_STREAM, 0);
> +    s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>      if (s->fd == -1) {
>          qemu_free(s);
>          return NULL;
> @@ -139,7 +139,7 @@ static void tcp_accept_incoming_migration(void *opaque)
>      int c, ret;
>  
>      do {
> -        c = accept(s, (struct sockaddr *)&addr, &addrlen);
> +        c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
>      } while (c == -1 && socket_error() == EINTR);
>  
>      dprintf("accepted migration\n");
> @@ -186,7 +186,7 @@ int tcp_start_incoming_migration(const char *host_port)
>          return -EINVAL;
>      }
>  
> -    s = socket(PF_INET, SOCK_STREAM, 0);
> +    s = qemu_socket(PF_INET, SOCK_STREAM, 0);
>      if (s == -1)
>          return -socket_error();
>  
> diff --git a/migration-unix.c b/migration-unix.c
> index a26587a..d3de7ae 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -98,7 +98,7 @@ MigrationState *unix_start_outgoing_migration(const char *path,
>      s->state = MIG_STATE_ACTIVE;
>      s->mon_resume = NULL;
>      s->bandwidth_limit = bandwidth_limit;
> -    s->fd = socket(PF_UNIX, SOCK_STREAM, 0);
> +    s->fd = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (s->fd < 0) {
>          dprintf("Unable to open socket");
>          goto err_after_alloc;
> @@ -143,7 +143,7 @@ static void unix_accept_incoming_migration(void *opaque)
>      int c, ret;
>  
>      do {
> -        c = accept(s, (struct sockaddr *)&addr, &addrlen);
> +        c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
>      } while (c == -1 && socket_error() == EINTR);
>  
>      dprintf("accepted migration\n");
> @@ -184,7 +184,7 @@ int unix_start_incoming_migration(const char *path)
>  
>      dprintf("Attempting to start an incoming migration\n");
>  
> -    sock = socket(PF_UNIX, SOCK_STREAM, 0);
> +    sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
>          fprintf(stderr, "Could not open unix socket: %s\n", strerror(errno));
>          return -EINVAL;
> diff --git a/net.c b/net.c
> index 9ea66e3..cff6efd 100644
> --- a/net.c
> +++ b/net.c
> @@ -1515,7 +1515,7 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr)
>  	return -1;
>  
>      }
> -    fd = socket(PF_INET, SOCK_DGRAM, 0);
> +    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
>      if (fd < 0) {
>          perror("socket(PF_INET, SOCK_DGRAM)");
>          return -1;
> @@ -1693,7 +1693,7 @@ static void net_socket_accept(void *opaque)
>  
>      for(;;) {
>          len = sizeof(saddr);
> -        fd = accept(s->fd, (struct sockaddr *)&saddr, &len);
> +        fd = qemu_accept(s->fd, (struct sockaddr *)&saddr, &len);
>          if (fd < 0 && errno != EINTR) {
>              return;
>          } else if (fd >= 0) {
> @@ -1724,7 +1724,7 @@ static int net_socket_listen_init(VLANState *vlan,
>  
>      s = qemu_mallocz(sizeof(NetSocketListenState));
>  
> -    fd = socket(PF_INET, SOCK_STREAM, 0);
> +    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>      if (fd < 0) {
>          perror("socket");
>          return -1;
> @@ -1765,7 +1765,7 @@ static int net_socket_connect_init(VLANState *vlan,
>      if (parse_host_port(&saddr, host_str) < 0)
>          return -1;
>  
> -    fd = socket(PF_INET, SOCK_STREAM, 0);
> +    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>      if (fd < 0) {
>          perror("socket");
>          return -1;
> diff --git a/osdep.c b/osdep.c
> index fd8bbd7..039065e 100644
> --- a/osdep.c
> +++ b/osdep.c
> @@ -121,7 +121,7 @@ int qemu_create_pidfile(const char *filename)
>  #ifndef _WIN32
>      int fd;
>  
> -    fd = open(filename, O_RDWR | O_CREAT, 0600);
> +    fd = qemu_open(filename, O_RDWR | O_CREAT, 0600);
>      if (fd == -1)
>          return -1;
>  
> @@ -201,11 +201,113 @@ int inet_aton(const char *cp, struct in_addr *ia)
>      ia->s_addr = addr;
>      return 1;
>  }
> +
> +void qemu_set_cloexec(int fd)
> +{
> +}
> +
>  #else
> +
>  void socket_set_nonblock(int fd)
>  {
>      int f;
>      f = fcntl(fd, F_GETFL);
>      fcntl(fd, F_SETFL, f | O_NONBLOCK);
>  }
> +
> +void qemu_set_cloexec(int fd)
> +{
> +    int f;
> +    f = fcntl(fd, F_GETFD);
> +    fcntl(fd, F_SETFD, f | FD_CLOEXEC);
> +}
> +
> +#endif
> +
> +/*
> + * Opens a file with FD_CLOEXEC set
> + */
> +int qemu_open(const char *name, int flags, ...)
> +{
> +    int ret;
> +    int mode = 0;
> +
> +    if (flags & O_CREAT) {
> +        va_list ap;
> +
> +        va_start(ap, flags);
> +        mode = va_arg(ap, int);
> +        va_end(ap);
> +    }
> +
> +#ifdef O_CLOEXEC
> +    ret = open(name, flags | O_CLOEXEC, mode);
> +#else
> +    ret = open(name, flags, mode);
> +    if (ret >= 0) {
> +        qemu_set_cloexec(ret);
> +    }
>  #endif
> +
> +    return ret;
> +}
> +
> +#ifndef _WIN32
> +/*
> + * Creates a pipe with FD_CLOEXEC set on both file descriptors
> + */
> +int qemu_pipe(int pipefd[2])
> +{
> +    int ret;
> +
> +#ifdef O_CLOEXEC
> +    ret = pipe2(pipefd, O_CLOEXEC);
> +#else
> +    ret = pipe(pipefd);
> +    if (ret == 0) {
> +        qemu_set_cloexec(pipefd[0]);
> +        qemu_set_cloexec(pipefd[1]);
> +    }
> +#endif
> +
> +    return ret;
> +}
> +#endif
> +
> +/*
> + * Opens a socket with FD_CLOEXEC set
> + */
> +int qemu_socket(int domain, int type, int protocol)
> +{
> +    int ret;
> +
> +#ifdef SOCK_CLOEXEC
> +    ret = socket(domain, type | SOCK_CLOEXEC, protocol);
> +#else
> +    ret = socket(domain, type, protocol);
> +    if (ret >= 0) {
> +        qemu_set_cloexec(ret);
> +    }
> +#endif
> +
> +    return ret;
> +}
> +
> +/*
> + * Accept a connection and set FD_CLOEXEC
> + */
> +int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen)
> +{
> +    int ret;
> +
> +#ifdef SOCK_CLOEXEC
> +    ret = accept4(s, addr, addrlen, SOCK_CLOEXEC);
>   

On Anthony's staging tree:

cc1: warnings being treated as errors
osdep.c: In function ‘qemu_accept’:
osdep.c:304: error: implicit declaration of function ‘accept4’


Alex