bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/4] Add a CLOEXEC recvfd


From: Bastien ROUCARIES
Subject: Re: [PATCH 3/4] Add a CLOEXEC recvfd
Date: Mon, 14 Mar 2011 10:21:00 +0100

On Sun, Mar 13, 2011 at 5:16 PM, Bruno Haible <address@hidden> wrote:
> Hi Bastien,
>
>> In order to avoid a race add a recvfd(int fd, int flags). flags could be 
>> O_CLOEXEC.
>> ---
>>  lib/passfd.c   |   58 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  lib/passfd.h   |    1 +
>>  m4/afunix.m4   |   22 +++++++++++++++++++++
>>  modules/passfd |    1 +
>
> This is pretty good as well. But there's no need for two functions recvfd and
> recvfd2. Better merge them into a single function. It's easy for the callers
> to pass a 0 flags argument. In the POSIX API, a separate function was
> introduced only when the previous function already existed for a long time,
> like pipe()/pipe2() and accept()/accept4().

recvfd exist under plan9 and I will prefer to use it in order to be
compatible....

Bastien

>
> Also, it is better to test 'flags & O_CLOEXEC' instead of 'flags == O_CLOEXEC'
> because
>  - The flags argument is conceptually a bit mask.
>  - O_CLOEXEC is 0 on some platforms (that don't support this flag), and
>    passing a 0 flags argument ought to _not_ set the close-on-exec flag.
>
> Other than this, I applied the usual untabification and .m4 file indentation.
>
> A possible simplification would be to assume that MSG_CMSG_CLOEXEC is a macro
> when it exists, and thus simplify "#if HAVE_MSG_CMSG_CLOEXEC" to
> "#if defined MSG_CMSG_CLOEXEC".
>
>
> 2011-03-13  Bastien Roucariès  <address@hidden>
>            Bruno Haible  <address@hidden>
>
>        passfd module, part 3.
>        * lib/passfd.h (recvfd): Add a flags argument.
>        * lib/passfd.c: Include <fcntl.h>, cloexec.h.
>        (recvfd): Add a flags argument.
>        * m4/afunix.m4 (gl_SOCKET_AFUNIX): Test whether MSG_CMSG_CLOEXEC
>        exists.
>        * modules/passfd (Depends-on): Add cloexec.
>        Suggested by Eric Blake.
>
> --- lib/passfd.c.orig   Sun Mar 13 16:26:20 2011
> +++ lib/passfd.c        Sun Mar 13 15:53:28 2011
> @@ -19,6 +19,7 @@
>  #include "passfd.h"
>
>  #include <errno.h>
> +#include <fcntl.h>
>  #include <stddef.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -35,6 +36,8 @@
>  # include <winsock2.h>
>  #endif
>
> +#include "cloexec.h"
> +
>  /* sendfd sends the file descriptor fd along the socket
>    to a process calling recvfd on the other end.
>
> @@ -84,11 +87,12 @@
>  }
>
>  /* recvfd receives a file descriptor through the socket.
> +   The flags are a bitmask, possibly including O_CLOEXEC (defined in 
> <fcntl.h>).
>
>    Return 0 on success, or -1 with errno set in case of error.
>  */
>  int
> -recvfd (int sock)
> +recvfd (int sock, int flags)
>  {
>   char recv = 0;
>   const int mone = -1;
> @@ -96,6 +100,12 @@
>   struct iovec iov[1];
>   struct msghdr msg;
>
> +  if ((flags & ~O_CLOEXEC) != 0)
> +    {
> +      errno = EINVAL;
> +      return -1;
> +    }
> +
>   /* send at least one char */
>   iov[0].iov_base = &recv;
>   iov[0].iov_len = 1;
> @@ -108,6 +118,11 @@
>  #if HAVE_UNIXSOCKET_SCM_RIGHTS_BSD44_WAY
>     struct cmsghdr *cmsg;
>     char buf[CMSG_SPACE (sizeof (fd))];
> +# if HAVE_MSG_CMSG_CLOEXEC
> +    int flags_recvmsg = (flags & O_CLOEXEC ? MSG_CMSG_CLOEXEC : 0);
> +# else
> +    int flags_recvmsg = 0;
> +# endif
>
>     msg.msg_control = buf;
>     msg.msg_controllen = sizeof (buf);
> @@ -119,7 +134,7 @@
>     memcpy (CMSG_DATA (cmsg), &mone, sizeof (mone));
>     msg.msg_controllen = cmsg->cmsg_len;
>
> -    if (recvmsg (sock, &msg, 0) < 0)
> +    if (recvmsg (sock, &msg, flags_recvmsg) < 0)
>       return -1;
>
>     cmsg = CMSG_FIRSTHDR (&msg);
> @@ -133,13 +148,43 @@
>       }
>
>     memcpy (&fd, CMSG_DATA (cmsg), sizeof (fd));
> +
> +# if !HAVE_MSG_CMSG_CLOEXEC
> +    /* set close-on-exec flag */
> +    if (flags & O_CLOEXEC)
> +      {
> +        if (set_cloexec_flag (fd, true) < 0)
> +          {
> +            int saved_errno = errno;
> +            (void) close (fd);
> +            errno = saved_errno;
> +            return -1;
> +          }
> +      }
> +# endif
> +
>     return fd;
> +
>  #elif HAVE_UNIXSOCKET_SCM_RIGHTS_BSD43_WAY
>     msg.msg_accrights = &fd;
>     msg.msg_accrightslen = sizeof (fd);
>     if (recvmsg (sock, &msg, 0) < 0)
>       return -1;
> +
> +    /* set close-on-exec flag */
> +    if (flags & O_CLOEXEC)
> +      {
> +        if (set_cloexec_flag (fd, true) < 0)
> +          {
> +            int saved_errno = errno;
> +            (void) close (fd);
> +            errno = saved_errno;
> +            return -1;
> +          }
> +      }
> +
>     return fd;
> +
>  #else
>     errno = ENOSYS;
>     return -1;
> --- lib/passfd.h.orig   Sun Mar 13 16:26:20 2011
> +++ lib/passfd.h        Sun Mar 13 15:43:03 2011
> @@ -23,7 +23,7 @@
>  #endif
>
>  extern int sendfd (int sock, int fd);
> -extern int recvfd (int sock);
> +extern int recvfd (int sock, int flags);
>
>  #ifdef __cplusplus
>  }
> --- m4/afunix.m4.orig   Sun Mar 13 16:26:20 2011
> +++ m4/afunix.m4        Sun Mar 13 15:58:06 2011
> @@ -1,4 +1,4 @@
> -# afunix.m4 serial 2
> +# afunix.m4 serial 3
>  dnl Copyright (C) 2011 Free Software Foundation, Inc.
>  dnl This file is free software; the Free Software Foundation
>  dnl gives unlimited permission to copy and/or distribute it,
> @@ -109,4 +109,31 @@
>     AC_DEFINE([HAVE_UNIXSOCKET_SCM_RIGHTS_BSD43_WAY], [1],
>       [Define to 1 if fd can be sent/received in the BSD4.3 way.])
>   fi
> +
> +  AC_MSG_CHECKING([for UNIX domain sockets recvmsg() MSG_CMSG_CLOEXEC flag])
> +  AC_CACHE_VAL([gl_cv_socket_unix_msg_cmsg_cloexec],
> +    [AC_COMPILE_IFELSE(
> +       [AC_LANG_PROGRAM(
> +          [[#include <sys/types.h>
> +            #ifdef HAVE_SYS_SOCKET_H
> +            #include <sys/socket.h>
> +            #endif
> +            #ifdef HAVE_SYS_UN_H
> +            #include <sys/un.h>
> +            #endif
> +            #ifdef HAVE_WINSOCK2_H
> +            #include <winsock2.h>
> +            #endif
> +            ]],
> +            [[int flags = MSG_CMSG_CLOEXEC;
> +              if (&flags) return 0;
> +            ]])],
> +       [gl_cv_socket_unix_msg_cmsg_cloexec=yes],
> +       [gl_cv_socket_unix_msg_cmsg_cloexec=no])
> +    ])
> +  AC_MSG_RESULT([$gl_cv_socket_unix_msg_cmsg_cloexec])
> +  if test $gl_cv_socket_unix_msg_cmsg_cloexec = yes; then
> +    AC_DEFINE([HAVE_MSG_CMSG_CLOEXEC], [1],
> +      [Define to 1 if recvmsg could be specified with MSG_CMSG_CLOEXEC.])
> +  fi
>  ])
> --- modules/passfd.orig Sun Mar 13 16:26:20 2011
> +++ modules/passfd      Sun Mar 13 16:03:40 2011
> @@ -8,6 +8,7 @@
>  m4/sockpfaf.m4
>
>  Depends-on:
> +cloexec
>  sys_socket
>  extensions
>
> --
> In memoriam Odette Sansom <http://en.wikipedia.org/wiki/Odette_Hallowes>
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]