[v2,2/4] hurd: Implement MSG_CMSG_CLOEXEC

  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(-)
  

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
>
  

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
  

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