[PATCH v2 1/4] hurd: Don't pass FD_CLOEXEC in CMSG_DATA

[PATCH v2 1/4] hurd: Don't pass FD_CLOEXEC in CMSG_DATA

[Sergey Bugaev]

It is of no concern to the receiving process whether or not the sender
process wants to close its copy of sent file descriptor upon exec, and
it should not influence whether or not the received file descriptor gets
the FD_CLOEXEC flag set in the receiving process.

The latter should in fact be dependent on the MSG_CMSG_CLOEXEC flag
being passed to the recvmsg () call, which is going to be implemented
in the following commit.

Fixes 344e755248ce02c0f8d095d11cc49e340703d926
"hurd: Support sending file descriptors over Unix sockets"

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/recvmsg.c | 5 +++--
 sysdeps/mach/hurd/sendmsg.c | 7 +++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/sysdeps/mach/hurd/recvmsg.c b/sysdeps/mach/hurd/recvmsg.c
index 39de86f6..c08eb499 100644
--- a/sysdeps/mach/hurd/recvmsg.c
+++ b/sysdeps/mach/hurd/recvmsg.c
@@ -189,7 +189,8 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
     if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
       {
 	/* SCM_RIGHTS support.  */
-	/* The fd's flags are passed in the control data.  */
+	/* The fd's flags are passed in the control data, but there currently
+	   aren't any defined other than FD_CLOEXEC which we don't respect.  */
 	int *fds = (int *) CMSG_DATA (cmsg);
 	nfds = (cmsg->cmsg_len - CMSG_ALIGN (sizeof (struct cmsghdr)))
 	       / sizeof (int);
@@ -200,7 +201,7 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
 	    if (err)
 	      goto cleanup;
 	    fds[j] = opened_fds[newfds] = _hurd_intern_fd (newports[newfds],
-							   fds[j], 0);
+							   0, 0);
 	    if (fds[j] == -1)
 	      {
 		err = errno;
diff --git a/sysdeps/mach/hurd/sendmsg.c b/sysdeps/mach/hurd/sendmsg.c
index 5871d1d8..2f19797b 100644
--- a/sysdeps/mach/hurd/sendmsg.c
+++ b/sysdeps/mach/hurd/sendmsg.c
@@ -138,8 +138,11 @@ __libc_sendmsg (int fd, const struct msghdr *message, int flags)
 					     0, 0, 0, 0);
 		   if (! err)
 		     nports++;
-		   /* We pass the flags in the control data.  */
-		   fds[i] = descriptor->flags;
+		   /* We pass the file descriptor flags in the control data.
+		      The only currently defined flag is FD_CLOEXEC, which we
+		      don't want to pass and so we mask it out, so this will
+		      always be 0 currently.  */
+		   fds[i] = descriptor->flags & ~FD_CLOEXEC;
 		   err;
 		 }));
 
-- 
2.40.0

[PATCH v2 2/4] hurd: Implement MSG_CMSG_CLOEXEC

[Sergey Bugaev]

This is a new flag that can be passed to recvmsg () to make it
atomically set the CLOEXEC flag on all the file descriptors received
using the SCM_RIGHTS mechanism. This is useful for all the same reasons
that the other XXX_CLOEXEC flags are useful: namely, it provides
atomicity with respect to another thread of the same process calling
(fork and then) exec at the same time.

This flag is already supported on Linux and FreeBSD. The flag's value,
0x40000, is choosen to match FreeBSD's.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/bits/socket.h | 5 ++++-
 sysdeps/mach/hurd/recvmsg.c     | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/sysdeps/mach/hurd/bits/socket.h b/sysdeps/mach/hurd/bits/socket.h
index 2d78a916..c2392bed 100644
--- a/sysdeps/mach/hurd/bits/socket.h
+++ b/sysdeps/mach/hurd/bits/socket.h
@@ -197,8 +197,11 @@ enum
 #define MSG_WAITALL MSG_WAITALL
     MSG_DONTWAIT	= 0x80,	/* This message should be nonblocking.  */
 #define MSG_DONTWAIT MSG_DONTWAIT
-    MSG_NOSIGNAL	= 0x0400	/* Do not generate SIGPIPE on EPIPE.  */
+    MSG_NOSIGNAL	= 0x0400,	/* Do not generate SIGPIPE on EPIPE.  */
 #define MSG_NOSIGNAL MSG_NOSIGNAL
+    MSG_CMSG_CLOEXEC	= 0x40000	/* Atomically set close-on-exec flag
+					   for file descriptors in SCM_RIGHTS.  */
+#define MSG_CMSG_CLOEXEC MSG_CMSG_CLOEXEC
   };
 
 
diff --git a/sysdeps/mach/hurd/recvmsg.c b/sysdeps/mach/hurd/recvmsg.c
index c08eb499..9a37a053 100644
--- a/sysdeps/mach/hurd/recvmsg.c
+++ b/sysdeps/mach/hurd/recvmsg.c
@@ -197,11 +197,12 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
 
 	for (j = 0; j < nfds; j++)
 	  {
+	    int fd_flags = (flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
 	    err = reauthenticate (ports[i], &newports[newfds]);
 	    if (err)
 	      goto cleanup;
 	    fds[j] = opened_fds[newfds] = _hurd_intern_fd (newports[newfds],
-							   0, 0);
+							   fd_flags, 0);
 	    if (fds[j] == -1)
 	      {
 		err = errno;
-- 
2.40.0

[PATCH v2 3/4] hurd: Only deallocate addrport when it's valid

[Sergey Bugaev]

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/recv.c     | 3 ++-
 sysdeps/mach/hurd/recvfrom.c | 3 ++-
 sysdeps/mach/hurd/recvmsg.c  | 3 ++-
 sysdeps/mach/hurd/sendmsg.c  | 5 +++--
 sysdeps/mach/hurd/sendto.c   | 2 +-
 5 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/sysdeps/mach/hurd/recv.c b/sysdeps/mach/hurd/recv.c
index 3bd5c16f..1783e38d 100644
--- a/sysdeps/mach/hurd/recv.c
+++ b/sysdeps/mach/hurd/recv.c
@@ -54,7 +54,8 @@ __recv (int fd, void *buf, size_t n, int flags)
   if (err)
     return __hurd_sockfail (fd, flags, err);
 
-  __mach_port_deallocate (__mach_task_self (), addrport);
+  if (MACH_PORT_VALID (addrport))
+    __mach_port_deallocate (__mach_task_self (), addrport);
   __vm_deallocate (__mach_task_self (), (vm_address_t) cdata, clen);
 
   if (bufp != buf)
diff --git a/sysdeps/mach/hurd/recvfrom.c b/sysdeps/mach/hurd/recvfrom.c
index 1cd5f917..6f2c927a 100644
--- a/sysdeps/mach/hurd/recvfrom.c
+++ b/sysdeps/mach/hurd/recvfrom.c
@@ -94,7 +94,8 @@ __recvfrom (int fd, void *buf, size_t n, int flags, __SOCKADDR_ARG addrarg,
   else if (addr_len != NULL)
     *addr_len = 0;
 
-  __mach_port_deallocate (__mach_task_self (), addrport);
+  if (MACH_PORT_VALID (addrport))
+    __mach_port_deallocate (__mach_task_self (), addrport);
 
   /* Toss control data; we don't care.  */
   __vm_deallocate (__mach_task_self (), (vm_address_t) cdata, clen);
diff --git a/sysdeps/mach/hurd/recvmsg.c b/sysdeps/mach/hurd/recvmsg.c
index 9a37a053..9cf3de48 100644
--- a/sysdeps/mach/hurd/recvmsg.c
+++ b/sysdeps/mach/hurd/recvmsg.c
@@ -135,7 +135,8 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
   else if (message->msg_name != NULL)
     message->msg_namelen = 0;
 
-  __mach_port_deallocate (__mach_task_self (), aport);
+  if (MACH_PORT_VALID (aport))
+    __mach_port_deallocate (__mach_task_self (), aport);
 
   if (buf == data)
     buf += len;
diff --git a/sysdeps/mach/hurd/sendmsg.c b/sysdeps/mach/hurd/sendmsg.c
index 2f19797b..f9ad7699 100644
--- a/sysdeps/mach/hurd/sendmsg.c
+++ b/sysdeps/mach/hurd/sendmsg.c
@@ -198,8 +198,9 @@ __libc_sendmsg (int fd, const struct msghdr *message, int flags)
 						   message->msg_controllen,
 						   &amount);
 			      LIBC_CANCEL_RESET (cancel_oldtype);
-			      __mach_port_deallocate (__mach_task_self (),
-						      aport);
+			      if (MACH_PORT_VALID (aport))
+				__mach_port_deallocate (__mach_task_self (),
+							aport);
 			    }
 			  err;
 			}));
diff --git a/sysdeps/mach/hurd/sendto.c b/sysdeps/mach/hurd/sendto.c
index 5a960de8..777af1c4 100644
--- a/sysdeps/mach/hurd/sendto.c
+++ b/sysdeps/mach/hurd/sendto.c
@@ -94,7 +94,7 @@ __sendto (int fd,
 			  err;
 			}));
 
-  if (aport != MACH_PORT_NULL)
+  if (MACH_PORT_VALID (aport))
     __mach_port_deallocate (__mach_task_self (), aport);
 
   return err ? __hurd_sockfail (fd, flags, err) : wrote;
-- 
2.40.0

[PATCH v2 4/4] socket: Add a test for MSG_CMSG_CLOEXEC

[Sergey Bugaev]

This checks that:
* We can send and receive fds over Unix domain sockets using SCM_RIGHTS;
* msg_controllen, cmsg_level, cmsg_type, cmsg_len are all filled in
  correctly on receive;
* Most importantly, the received fd has or has not the close-on-exec
  flag set depending on whether we pass MSG_CMSG_CLOEXEC to recvmsg ().

Checked on i686-gnu and x86_64-linux-gnu.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 socket/Makefile           |   1 +
 socket/tst-cmsg_cloexec.c | 126 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 127 insertions(+)
 create mode 100644 socket/tst-cmsg_cloexec.c

diff --git a/socket/Makefile b/socket/Makefile
index fffed7dd..94951ae3 100644
--- a/socket/Makefile
+++ b/socket/Makefile
@@ -35,6 +35,7 @@ tests := \
   tst-accept4 \
   tst-sockopt \
   tst-cmsghdr \
+  tst-cmsg_cloexec \
   # tests
 
 tests-internal := \
diff --git a/socket/tst-cmsg_cloexec.c b/socket/tst-cmsg_cloexec.c
new file mode 100644
index 00000000..a18f4912
--- /dev/null
+++ b/socket/tst-cmsg_cloexec.c
@@ -0,0 +1,126 @@
+/* Smoke test for MSG_CMSG_CLOEXEC.
+   Copyright (C) 2021-2023 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <support/xunistd.h>
+#include <support/check.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <string.h>
+#include <fcntl.h>
+
+static void
+send_fd (int sockfd, int fd)
+{
+  char data[] = "hello";
+  struct iovec iov = { .iov_base = data, .iov_len = sizeof (data) };
+
+  union
+  {
+    struct cmsghdr header;
+    char bytes[CMSG_SPACE (sizeof (fd))];
+  } cmsg_storage;
+
+  struct msghdr msg =
+    {
+      .msg_iov = &iov,
+      .msg_iovlen = 1,
+      .msg_control = cmsg_storage.bytes,
+      .msg_controllen = sizeof (cmsg_storage)
+    };
+
+  memset (&cmsg_storage, 0, sizeof (cmsg_storage));
+
+  struct cmsghdr *cmsg = CMSG_FIRSTHDR (&msg);
+  cmsg->cmsg_level = SOL_SOCKET;
+  cmsg->cmsg_type = SCM_RIGHTS;
+  cmsg->cmsg_len = CMSG_LEN (sizeof (fd));
+  memcpy (CMSG_DATA (cmsg), &fd, sizeof (fd));
+
+  ssize_t nsent = sendmsg (sockfd, &msg, 0);
+  if (nsent < 0)
+    FAIL_EXIT1 ("sendmsg (%d): %m", sockfd);
+  TEST_COMPARE (nsent, sizeof (data));
+}
+
+static int
+recv_fd (int sockfd, int flags)
+{
+  char buffer[100];
+  struct iovec iov = { .iov_base = buffer, .iov_len = sizeof (buffer) };
+
+  union
+  {
+    struct cmsghdr header;
+    char bytes[100];
+  } cmsg_storage;
+
+  struct msghdr msg =
+    {
+      .msg_iov = &iov,
+      .msg_iovlen = 1,
+      .msg_control = cmsg_storage.bytes,
+      .msg_controllen = sizeof (cmsg_storage)
+    };
+
+  ssize_t nrecv = recvmsg (sockfd, &msg, flags);
+  if (nrecv < 0)
+    FAIL_EXIT1 ("recvmsg (%d): %m", sockfd);
+
+  TEST_COMPARE (msg.msg_controllen, CMSG_SPACE (sizeof (int)));
+  struct cmsghdr *cmsg = CMSG_FIRSTHDR (&msg);
+  TEST_COMPARE (cmsg->cmsg_level, SOL_SOCKET);
+  TEST_COMPARE (cmsg->cmsg_type, SCM_RIGHTS);
+  TEST_COMPARE (cmsg->cmsg_len, CMSG_LEN (sizeof (int)));
+
+  int fd;
+  memcpy (&fd, CMSG_DATA (cmsg), sizeof (fd));
+  return fd;
+}
+
+static int
+do_test (void)
+{
+  int sockfd[2];
+  int newfd;
+  int flags;
+  int rc = socketpair (AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0, sockfd);
+  if (rc < 0)
+    FAIL_EXIT1 ("socketpair: %m");
+
+  send_fd (sockfd[1], STDIN_FILENO);
+  newfd = recv_fd (sockfd[0], 0);
+  TEST_VERIFY_EXIT (newfd > 0);
+  flags = fcntl (newfd, F_GETFD, 0);
+  TEST_VERIFY_EXIT (flags != -1);
+  TEST_VERIFY (!(flags & FD_CLOEXEC));
+  xclose (newfd);
+
+  send_fd (sockfd[1], STDIN_FILENO);
+  newfd = recv_fd (sockfd[0], MSG_CMSG_CLOEXEC);
+  TEST_VERIFY_EXIT (newfd > 0);
+  flags = fcntl (newfd, F_GETFD, 0);
+  TEST_VERIFY_EXIT (flags != -1);
+  TEST_VERIFY (flags & FD_CLOEXEC);
+  xclose (newfd);
+
+  xclose (sockfd[0]);
+  xclose (sockfd[1]);
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.40.0

Re: [PATCH v2 3/4] hurd: Only deallocate addrport when it's valid

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le dim. 23 avril 2023 19:05:47 +0300, a ecrit:
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/recv.c     | 3 ++-
>  sysdeps/mach/hurd/recvfrom.c | 3 ++-
>  sysdeps/mach/hurd/recvmsg.c  | 3 ++-
>  sysdeps/mach/hurd/sendmsg.c  | 5 +++--
>  sysdeps/mach/hurd/sendto.c   | 2 +-
>  5 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/recv.c b/sysdeps/mach/hurd/recv.c
> index 3bd5c16f..1783e38d 100644
> --- a/sysdeps/mach/hurd/recv.c
> +++ b/sysdeps/mach/hurd/recv.c
> @@ -54,7 +54,8 @@ __recv (int fd, void *buf, size_t n, int flags)
>    if (err)
>      return __hurd_sockfail (fd, flags, err);
>  
> -  __mach_port_deallocate (__mach_task_self (), addrport);
> +  if (MACH_PORT_VALID (addrport))
> +    __mach_port_deallocate (__mach_task_self (), addrport);
>    __vm_deallocate (__mach_task_self (), (vm_address_t) cdata, clen);
>  
>    if (bufp != buf)
> diff --git a/sysdeps/mach/hurd/recvfrom.c b/sysdeps/mach/hurd/recvfrom.c
> index 1cd5f917..6f2c927a 100644
> --- a/sysdeps/mach/hurd/recvfrom.c
> +++ b/sysdeps/mach/hurd/recvfrom.c
> @@ -94,7 +94,8 @@ __recvfrom (int fd, void *buf, size_t n, int flags, __SOCKADDR_ARG addrarg,
>    else if (addr_len != NULL)
>      *addr_len = 0;
>  
> -  __mach_port_deallocate (__mach_task_self (), addrport);
> +  if (MACH_PORT_VALID (addrport))
> +    __mach_port_deallocate (__mach_task_self (), addrport);
>  
>    /* Toss control data; we don't care.  */
>    __vm_deallocate (__mach_task_self (), (vm_address_t) cdata, clen);
> diff --git a/sysdeps/mach/hurd/recvmsg.c b/sysdeps/mach/hurd/recvmsg.c
> index 9a37a053..9cf3de48 100644
> --- a/sysdeps/mach/hurd/recvmsg.c
> +++ b/sysdeps/mach/hurd/recvmsg.c
> @@ -135,7 +135,8 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
>    else if (message->msg_name != NULL)
>      message->msg_namelen = 0;
>  
> -  __mach_port_deallocate (__mach_task_self (), aport);
> +  if (MACH_PORT_VALID (aport))
> +    __mach_port_deallocate (__mach_task_self (), aport);
>  
>    if (buf == data)
>      buf += len;
> diff --git a/sysdeps/mach/hurd/sendmsg.c b/sysdeps/mach/hurd/sendmsg.c
> index 2f19797b..f9ad7699 100644
> --- a/sysdeps/mach/hurd/sendmsg.c
> +++ b/sysdeps/mach/hurd/sendmsg.c
> @@ -198,8 +198,9 @@ __libc_sendmsg (int fd, const struct msghdr *message, int flags)
>  						   message->msg_controllen,
>  						   &amount);
>  			      LIBC_CANCEL_RESET (cancel_oldtype);
> -			      __mach_port_deallocate (__mach_task_self (),
> -						      aport);
> +			      if (MACH_PORT_VALID (aport))
> +				__mach_port_deallocate (__mach_task_self (),
> +							aport);
>  			    }
>  			  err;
>  			}));
> diff --git a/sysdeps/mach/hurd/sendto.c b/sysdeps/mach/hurd/sendto.c
> index 5a960de8..777af1c4 100644
> --- a/sysdeps/mach/hurd/sendto.c
> +++ b/sysdeps/mach/hurd/sendto.c
> @@ -94,7 +94,7 @@ __sendto (int fd,
>  			  err;
>  			}));
>  
> -  if (aport != MACH_PORT_NULL)
> +  if (MACH_PORT_VALID (aport))
>      __mach_port_deallocate (__mach_task_self (), aport);
>  
>    return err ? __hurd_sockfail (fd, flags, err) : wrote;
> -- 
> 2.40.0
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH v2 1/4] hurd: Don't pass FD_CLOEXEC in CMSG_DATA

[Samuel Thibault]

Sergey Bugaev, le dim. 23 avril 2023 19:05:45 +0300, a ecrit:
> It is of no concern to the receiving process whether or not the sender
> process wants to close its copy of sent file descriptor upon exec, and
> it should not influence whether or not the received file descriptor gets
> the FD_CLOEXEC flag set in the receiving process.
> 
> The latter should in fact be dependent on the MSG_CMSG_CLOEXEC flag
> being passed to the recvmsg () call, which is going to be implemented
> in the following commit.
> 
> Fixes 344e755248ce02c0f8d095d11cc49e340703d926
> "hurd: Support sending file descriptors over Unix sockets"
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/recvmsg.c | 5 +++--
>  sysdeps/mach/hurd/sendmsg.c | 7 +++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/recvmsg.c b/sysdeps/mach/hurd/recvmsg.c
> index 39de86f6..c08eb499 100644
> --- a/sysdeps/mach/hurd/recvmsg.c
> +++ b/sysdeps/mach/hurd/recvmsg.c
> @@ -189,7 +189,8 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
>      if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
>        {
>  	/* SCM_RIGHTS support.  */
> -	/* The fd's flags are passed in the control data.  */
> +	/* The fd's flags are passed in the control data, but there currently
> +	   aren't any defined other than FD_CLOEXEC which we don't respect.  */
>  	int *fds = (int *) CMSG_DATA (cmsg);
>  	nfds = (cmsg->cmsg_len - CMSG_ALIGN (sizeof (struct cmsghdr)))
>  	       / sizeof (int);
> @@ -200,7 +201,7 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
>  	    if (err)
>  	      goto cleanup;
>  	    fds[j] = opened_fds[newfds] = _hurd_intern_fd (newports[newfds],
> -							   fds[j], 0);
> +							   0, 0);

The two patches actually make me realize that there was a confusion here
between FD_* flags and O_* flags. _hurd_intern_fd definitely takes O_*
flags (and translates O_CLOEXEC to FD_CLOEXEC). If we are to transport
some flags, it'd probably rather be O_* flags. I'll commit that way.

>  	    if (fds[j] == -1)
>  	      {
>  		err = errno;
> diff --git a/sysdeps/mach/hurd/sendmsg.c b/sysdeps/mach/hurd/sendmsg.c
> index 5871d1d8..2f19797b 100644
> --- a/sysdeps/mach/hurd/sendmsg.c
> +++ b/sysdeps/mach/hurd/sendmsg.c
> @@ -138,8 +138,11 @@ __libc_sendmsg (int fd, const struct msghdr *message, int flags)
>  					     0, 0, 0, 0);
>  		   if (! err)
>  		     nports++;
> -		   /* We pass the flags in the control data.  */
> -		   fds[i] = descriptor->flags;
> +		   /* We pass the file descriptor flags in the control data.
> +		      The only currently defined flag is FD_CLOEXEC, which we
> +		      don't want to pass and so we mask it out, so this will
> +		      always be 0 currently.  */
> +		   fds[i] = descriptor->flags & ~FD_CLOEXEC;
>  		   err;
>  		 }));
>  
> -- 
> 2.40.0
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH v2 2/4] hurd: Implement MSG_CMSG_CLOEXEC

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le dim. 23 avril 2023 19:05:46 +0300, a ecrit:
> This is a new flag that can be passed to recvmsg () to make it
> atomically set the CLOEXEC flag on all the file descriptors received
> using the SCM_RIGHTS mechanism. This is useful for all the same reasons
> that the other XXX_CLOEXEC flags are useful: namely, it provides
> atomicity with respect to another thread of the same process calling
> (fork and then) exec at the same time.
> 
> This flag is already supported on Linux and FreeBSD. The flag's value,
> 0x40000, is choosen to match FreeBSD's.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/bits/socket.h | 5 ++++-
>  sysdeps/mach/hurd/recvmsg.c     | 3 ++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/bits/socket.h b/sysdeps/mach/hurd/bits/socket.h
> index 2d78a916..c2392bed 100644
> --- a/sysdeps/mach/hurd/bits/socket.h
> +++ b/sysdeps/mach/hurd/bits/socket.h
> @@ -197,8 +197,11 @@ enum
>  #define MSG_WAITALL MSG_WAITALL
>      MSG_DONTWAIT	= 0x80,	/* This message should be nonblocking.  */
>  #define MSG_DONTWAIT MSG_DONTWAIT
> -    MSG_NOSIGNAL	= 0x0400	/* Do not generate SIGPIPE on EPIPE.  */
> +    MSG_NOSIGNAL	= 0x0400,	/* Do not generate SIGPIPE on EPIPE.  */
>  #define MSG_NOSIGNAL MSG_NOSIGNAL
> +    MSG_CMSG_CLOEXEC	= 0x40000	/* Atomically set close-on-exec flag
> +					   for file descriptors in SCM_RIGHTS.  */
> +#define MSG_CMSG_CLOEXEC MSG_CMSG_CLOEXEC
>    };
>  
>  
> diff --git a/sysdeps/mach/hurd/recvmsg.c b/sysdeps/mach/hurd/recvmsg.c
> index c08eb499..9a37a053 100644
> --- a/sysdeps/mach/hurd/recvmsg.c
> +++ b/sysdeps/mach/hurd/recvmsg.c
> @@ -197,11 +197,12 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
>  
>  	for (j = 0; j < nfds; j++)
>  	  {
> +	    int fd_flags = (flags & MSG_CMSG_CLOEXEC) ? O_CLOEXEC : 0;
>  	    err = reauthenticate (ports[i], &newports[newfds]);
>  	    if (err)
>  	      goto cleanup;
>  	    fds[j] = opened_fds[newfds] = _hurd_intern_fd (newports[newfds],
> -							   0, 0);
> +							   fd_flags, 0);
>  	    if (fds[j] == -1)
>  	      {
>  		err = errno;
> -- 
> 2.40.0
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.

Re: [PATCH v2 1/4] hurd: Don't pass FD_CLOEXEC in CMSG_DATA

[Sergey Bugaev]

On Tue, Apr 25, 2023 at 12:02 AM Samuel Thibault
<samuel.thibault@gnu.org> wrote:
> The two patches actually make me realize that there was a confusion here
> between FD_* flags and O_* flags. _hurd_intern_fd definitely takes O_*
> flags (and translates O_CLOEXEC to FD_CLOEXEC). If we are to transport
> some flags, it'd probably rather be O_* flags. I'll commit that way.

Right, good point.

Sergey

Re: [PATCH v2 2/4] hurd: Implement MSG_CMSG_CLOEXEC

[Sergey Bugaev]

On Tue, Apr 25, 2023 at 12:10 AM Samuel Thibault
<samuel.thibault@gnu.org> wrote:
> Applied, thanks!

Thank you -- but I see you changed it to say "fds[j] | fd_flags".

For one thing it would be nice of you to indicate that this was your
change, not mine, because as things are it looks like I wrote that,
but I didn't. Linux docs (I was about to write "kernel docs", heh)
suggest this pattern:

> it is recommended that you add a line between the last
> Signed-off-by header and yours, indicating the nature of your
> changes. While there is nothing mandatory about this, it seems like
> prepending the description with your mail and/or name, all enclosed
> in square brackets, is noticeable enough to make it obvious that you
> are responsible for last-minute changes. Example :
>
> Signed-off-by: Random J Developer <random@developer.example.org>
> [lucky@maintainer.example.org: struct foo moved from foo.c to foo.h]
> Signed-off-by: Lucky K Maintainer <lucky@maintainer.example.org>

But on the technical side of things, I don't think we should take
whatever integer arrives in the message and use it as flags. We never
check it for sanity; who knows what might be there; the fd management
subsystem is not generally written with the assumption that 'flags'
might be attacker-controlled/malicious. I don't see how anything
actually bad could happen in this case, but it could specify O_CLOEXEC
and/or O_IGNORE_CTTY when we don't want them, for instance.

Sergey

Re: [PATCH v2 2/4] hurd: Implement MSG_CMSG_CLOEXEC

[Samuel Thibault]

Sergey Bugaev, le mar. 25 avril 2023 00:35:58 +0300, a ecrit:
> On Tue, Apr 25, 2023 at 12:10 AM Samuel Thibault
> <samuel.thibault@gnu.org> wrote:
> > Applied, thanks!
> 
> Thank you -- but I see you changed it to say "fds[j] | fd_flags".
> 
> For one thing it would be nice of you to indicate that this was your
> change, not mine,

That change is not actually in this commit, but in the previous where I
left fds[j] as it was. In this commit we just add | fd_flags.

> because as things are it looks like I wrote that,
> but I didn't. Linux docs (I was about to write "kernel docs", heh)
> suggest this pattern:
> 
> > it is recommended that you add a line between the last
> > Signed-off-by header and yours, indicating the nature of your
> > changes. While there is nothing mandatory about this, it seems like
> > prepending the description with your mail and/or name, all enclosed
> > in square brackets, is noticeable enough to make it obvious that you
> > are responsible for last-minute changes. Example :
> >
> > Signed-off-by: Random J Developer <random@developer.example.org>
> > [lucky@maintainer.example.org: struct foo moved from foo.c to foo.h]
> > Signed-off-by: Lucky K Maintainer <lucky@maintainer.example.org>

Ah, I didn't know about that style.

> But on the technical side of things, I don't think we should take
> whatever integer arrives in the message and use it as flags. We never
> check it for sanity; who knows what might be there;

Right. I have added the filtering, then. Yes, "& 0" looks odd, but I'd
rather keep in code the documentation of how we believe it's supposed to
work, for people to find it later whenever needed.

Samuel

Re: [PATCH v2 4/4] socket: Add a test for MSG_CMSG_CLOEXEC

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le dim. 23 avril 2023 19:05:48 +0300, a ecrit:
> This checks that:
> * We can send and receive fds over Unix domain sockets using SCM_RIGHTS;
> * msg_controllen, cmsg_level, cmsg_type, cmsg_len are all filled in
>   correctly on receive;
> * Most importantly, the received fd has or has not the close-on-exec
>   flag set depending on whether we pass MSG_CMSG_CLOEXEC to recvmsg ().
> 
> Checked on i686-gnu and x86_64-linux-gnu.
> 
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  socket/Makefile           |   1 +
>  socket/tst-cmsg_cloexec.c | 126 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 127 insertions(+)
>  create mode 100644 socket/tst-cmsg_cloexec.c
> 
> diff --git a/socket/Makefile b/socket/Makefile
> index fffed7dd..94951ae3 100644
> --- a/socket/Makefile
> +++ b/socket/Makefile
> @@ -35,6 +35,7 @@ tests := \
>    tst-accept4 \
>    tst-sockopt \
>    tst-cmsghdr \
> +  tst-cmsg_cloexec \
>    # tests
>  
>  tests-internal := \
> diff --git a/socket/tst-cmsg_cloexec.c b/socket/tst-cmsg_cloexec.c
> new file mode 100644
> index 00000000..a18f4912
> --- /dev/null
> +++ b/socket/tst-cmsg_cloexec.c
> @@ -0,0 +1,126 @@
> +/* Smoke test for MSG_CMSG_CLOEXEC.
> +   Copyright (C) 2021-2023 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <support/xunistd.h>
> +#include <support/check.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <string.h>
> +#include <fcntl.h>
> +
> +static void
> +send_fd (int sockfd, int fd)
> +{
> +  char data[] = "hello";
> +  struct iovec iov = { .iov_base = data, .iov_len = sizeof (data) };
> +
> +  union
> +  {
> +    struct cmsghdr header;
> +    char bytes[CMSG_SPACE (sizeof (fd))];
> +  } cmsg_storage;
> +
> +  struct msghdr msg =
> +    {
> +      .msg_iov = &iov,
> +      .msg_iovlen = 1,
> +      .msg_control = cmsg_storage.bytes,
> +      .msg_controllen = sizeof (cmsg_storage)
> +    };
> +
> +  memset (&cmsg_storage, 0, sizeof (cmsg_storage));
> +
> +  struct cmsghdr *cmsg = CMSG_FIRSTHDR (&msg);
> +  cmsg->cmsg_level = SOL_SOCKET;
> +  cmsg->cmsg_type = SCM_RIGHTS;
> +  cmsg->cmsg_len = CMSG_LEN (sizeof (fd));
> +  memcpy (CMSG_DATA (cmsg), &fd, sizeof (fd));
> +
> +  ssize_t nsent = sendmsg (sockfd, &msg, 0);
> +  if (nsent < 0)
> +    FAIL_EXIT1 ("sendmsg (%d): %m", sockfd);
> +  TEST_COMPARE (nsent, sizeof (data));
> +}
> +
> +static int
> +recv_fd (int sockfd, int flags)
> +{
> +  char buffer[100];
> +  struct iovec iov = { .iov_base = buffer, .iov_len = sizeof (buffer) };
> +
> +  union
> +  {
> +    struct cmsghdr header;
> +    char bytes[100];
> +  } cmsg_storage;
> +
> +  struct msghdr msg =
> +    {
> +      .msg_iov = &iov,
> +      .msg_iovlen = 1,
> +      .msg_control = cmsg_storage.bytes,
> +      .msg_controllen = sizeof (cmsg_storage)
> +    };
> +
> +  ssize_t nrecv = recvmsg (sockfd, &msg, flags);
> +  if (nrecv < 0)
> +    FAIL_EXIT1 ("recvmsg (%d): %m", sockfd);
> +
> +  TEST_COMPARE (msg.msg_controllen, CMSG_SPACE (sizeof (int)));
> +  struct cmsghdr *cmsg = CMSG_FIRSTHDR (&msg);
> +  TEST_COMPARE (cmsg->cmsg_level, SOL_SOCKET);
> +  TEST_COMPARE (cmsg->cmsg_type, SCM_RIGHTS);
> +  TEST_COMPARE (cmsg->cmsg_len, CMSG_LEN (sizeof (int)));
> +
> +  int fd;
> +  memcpy (&fd, CMSG_DATA (cmsg), sizeof (fd));
> +  return fd;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int sockfd[2];
> +  int newfd;
> +  int flags;
> +  int rc = socketpair (AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0, sockfd);
> +  if (rc < 0)
> +    FAIL_EXIT1 ("socketpair: %m");
> +
> +  send_fd (sockfd[1], STDIN_FILENO);
> +  newfd = recv_fd (sockfd[0], 0);
> +  TEST_VERIFY_EXIT (newfd > 0);
> +  flags = fcntl (newfd, F_GETFD, 0);
> +  TEST_VERIFY_EXIT (flags != -1);
> +  TEST_VERIFY (!(flags & FD_CLOEXEC));
> +  xclose (newfd);
> +
> +  send_fd (sockfd[1], STDIN_FILENO);
> +  newfd = recv_fd (sockfd[0], MSG_CMSG_CLOEXEC);
> +  TEST_VERIFY_EXIT (newfd > 0);
> +  flags = fcntl (newfd, F_GETFD, 0);
> +  TEST_VERIFY_EXIT (flags != -1);
> +  TEST_VERIFY (flags & FD_CLOEXEC);
> +  xclose (newfd);
> +
> +  xclose (sockfd[0]);
> +  xclose (sockfd[1]);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> -- 
> 2.40.0
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.