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

Message ID 20230423160548.126576-1-bugaevc@gmail.com
State Superseded
Headers
Series [v2,1/4] hurd: Don't pass FD_CLOEXEC in CMSG_DATA |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Sergey Bugaev April 23, 2023, 4:05 p.m. UTC
  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(-)
  

Comments

Samuel Thibault April 24, 2023, 9:01 p.m. UTC | #1
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
>
  
Sergey Bugaev April 24, 2023, 9:13 p.m. UTC | #2
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
  

Patch

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;
 		 }));