Re: [PATCH] Add support to send file descriptors over Unix sockets

[Carl Fredrik Hammar]

Hi,

On Tue, Jul 27, 2010 at 08:08:36PM +0200, Emilio Pozuelo Monfort wrote:
> 
> This patch adds support for SCM_RIGHTS to glibc. It works fine
> when sending file descriptors from a socket() or a socketpair() call,
> but not from e.g. an open() call, as I've mentioned in another mail.
> That's not because of my patch though.

OK, I feel I could have been bit more thorough with this review
but I found plenty of things to keep you busy none the less.  ;-)
First, a couple of overall points.

What about CTTYs?  To be honest, I don't know much about them myself.
They sound like something very process specific so I don't *think* we need
to transfer anything, but do we need to set them to something on receive?
I don't have time to investigate further ATM.  Could you investigate,
and e.g. see what happens in Linux?

Another thing that crossed my mind is what'll happen if SCM_CREDS is
also sent in the future?  Because it also involves port, your code will
fail because of the j != nports test.  I think this is acceptable for
now since the current code just fail silently anyway.

> The attached testcase runs fine with the #if set to 1 (i.e. sending
> socket fds).

Could you also make use of the sent fds so we can be absolutely sure
it works?

> diff --git a/sysdeps/mach/hurd/recvmsg.c b/sysdeps/mach/hurd/recvmsg.c
> index 33897b8..cda246e 100644
> --- a/sysdeps/mach/hurd/recvmsg.c
> +++ b/sysdeps/mach/hurd/recvmsg.c
> @@ -1,4 +1,4 @@
> -/* Copyright (C) 2001, 2002 Free Software Foundation, Inc.
> +/* Copyright (C) 2001, 2002, 2010 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
> @@ -33,13 +33,37 @@ __libc_recvmsg (int fd, struct msghdr *message, int flags)
>    addr_port_t aport;
>    char *data = NULL;
>    mach_msg_type_number_t len = 0;
> -  mach_port_t *ports;
> +  mach_port_t *ports, *newports;
>    mach_msg_type_number_t nports = 0;
> +  struct cmsghdr *cmsg;
>    char *cdata = NULL;
>    mach_msg_type_number_t clen = 0;
>    size_t amount;
>    char *buf;
> -  int i;
> +  int nfds, *fds;
> +  int i, j;
> +
> +  auth_t auth;
> +
> +  error_t reauthenticate (mach_port_t port, mach_port_t *result)
> +    {
> +      error_t err;
> +      mach_port_t ref;
> +      if (*result != MACH_PORT_NULL)
> +     return 0;

I don't really see the point of this test.
I guess it's a copy-paste relic.  :-)

> +      ref = __mach_reply_port ();
> +      do
> +     err = __io_reauthenticate (port, ref, MACH_MSG_TYPE_MAKE_SEND);
> +      while (err == EINTR);

I'm not sure if __io_reauthenticate can be interrupted since it is
a simpleroutine.  Oh well, better safe than sorry, I guess.

> +      if (!err)
> +     do
> +       err = __auth_user_authenticate (auth,
> +                                       ref, MACH_MSG_TYPE_MAKE_SEND,
> +                                       result);
> +     while (err == EINTR);
> +      __mach_port_destroy (__mach_task_self (), ref);
> +      return err;
> +    }

Just to be clear, __mach_port_destroy should be used here
since __mach_reply_port does create a new port.

>  
>    /* Find the total number of bytes to be read.  */
>    amount = 0;
> @@ -136,6 +160,84 @@ __libc_recvmsg (int fd, struct msghdr *message, int 
> flags)
>      message->msg_controllen = clen;
>    memcpy (message->msg_control, cdata, message->msg_controllen);
>  
> +  /* SCM_RIGHTS ports.  */
> +  if (nports > 0)
> +    {
> +      auth = getauth ();
> +      newports = __alloca (nports * sizeof (mach_port_t));
> +
> +      /* Reauthenticate all ports here.  */
> +      for (i = 0; i < nports; i++)
> +     {
> +       newports[i] = MACH_PORT_NULL;

No need to set it to null if you remove the test in reauthenticate.

> +       err = reauthenticate (ports[i], &newports[i]);
> +       __mach_port_destroy (__mach_task_self (), ports[i]);
> +       if (err)
> +         {
> +           for (j = 0; j < i; j++)
> +             __mach_port_destroy (__mach_task_self (), newports[j]);
> +           for (j = i+1; j < nports; j++)
> +             __mach_port_destroy (__mach_task_self (), ports[j]);
> +
> +           __vm_deallocate (__mach_task_self (), (vm_address_t) cdata, clen);
> +           __hurd_fail (err);
> +         }
> +     }

Use __mach_port_deallocate instead of __mach_port_destroy.
If reauthenticate fails, shouldn't we go directly to cleanup?

> +      j = 0;
> +      for (cmsg = CMSG_FIRSTHDR (message);
> +        cmsg;
> +        cmsg = CMSG_NXTHDR (message, cmsg))
> +     {
> +       if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
> +         {
> +           fds = (int *) CMSG_DATA (cmsg);
> +           nfds = (cmsg->cmsg_len - CMSG_ALIGN (sizeof (struct cmsghdr)))
> +                  / sizeof (int);
> +
> +           for (i = 0; i < nfds && j < nports; i++)
> +             {
> +               /* The fd's flags are passed in the control data.  */
> +               if ((fds[i] = _hurd_intern_fd (newports[j++], fds[i], 0))
> +                   == -1)

Since you break this line anyway you might as well do:
  fds[i] = _hurd_intern_fd (newports[j++], fds[i], 0);
  if (fds[i] == -1)

> +                 {
> +                   err = errno;
> +                   goto cleanup;
> +                 }
> +             }
> +         }
> +     }
> +      if (j != nports)
> +     /* Clean up all the file descriptors.  */
> +     {
> +       err = EGRATUITOUS;
> + cleanup:

Labels should line up with the opening brace.  Some Hurd code indents it
with one space instead, but never relative to the left margin.  However,
in this case I think it is better to break things up a bit to separate
the cleanup code from the loop logic:

  if (j != nports)
    err = EGRATUITOUS;

cleanup:
  if (err)
    ...

I was also going to object to using EGRATUITOUS, but it *does* look
like it is used when system servers such as pflocal returs bogus data,
so I guess it'll do.

> +       nports = j;
> +       j = 0;
> +       for (cmsg = CMSG_FIRSTHDR (message);
> +            cmsg;
> +            cmsg = CMSG_NXTHDR (message, cmsg))
> +         {
> +           if (cmsg->cmsg_level == SOL_SOCKET
> +               && cmsg->cmsg_type == SCM_RIGHTS)
> +             {
> +               fds = (int *) CMSG_DATA (cmsg);
> +               nfds = (cmsg->cmsg_len
> +                       - CMSG_ALIGN (sizeof (struct cmsghdr)))
> +                       / sizeof (int);

The / should line up with (.

> +               for (i = 0; i < nfds && j < nports; i++, j++)
> +                 _hurd_fd_close (_hurd_fd_get (fds[i]));
> +             }
> +         }
> +
> +       for (; j < nports; j++)
> +         __mach_port_destroy (__mach_task_self (), newports[j]);
> +

Use __mach_port_deallocate, not __mach_port_destroy.

> +       __vm_deallocate (__mach_task_self (), (vm_address_t) cdata, clen);
> +       __hurd_fail (err);
> +     }
> +    }
> +
>    __vm_deallocate (__mach_task_self (), (vm_address_t) cdata, clen);
>  
>    return (buf - data);
> diff --git a/sysdeps/mach/hurd/sendmsg.c b/sysdeps/mach/hurd/sendmsg.c
> index 118fd59..7c6c666 100644
> --- a/sysdeps/mach/hurd/sendmsg.c
> +++ b/sysdeps/mach/hurd/sendmsg.c
> @@ -1,4 +1,4 @@
> -/* Copyright (C) 2001,2002,2004 Free Software Foundation, Inc.
> +/* Copyright (C) 2001,2002,2004,2010 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
> @@ -32,6 +32,10 @@ ssize_t
>  __libc_sendmsg (int fd, const struct msghdr *message, int flags)
>  {
>    error_t err = 0;
> +  struct cmsghdr *cmsg;
> +  mach_port_t *ports = NULL;
> +  mach_msg_type_number_t nports = 0;
> +  int *fds, nfds;
>    struct sockaddr_un *addr = message->msg_name;
>    socklen_t addr_len = message->msg_namelen;
>    addr_port_t aport = MACH_PORT_NULL;
> @@ -101,6 +105,48 @@ __libc_sendmsg (int fd, const struct msghdr *message, 
> int flags)
>       }
>      }
>  
> +  /* SCM_RIGHTS support: get the number of fds to send.  */
> +  for (cmsg = CMSG_FIRSTHDR (message); cmsg; cmsg = CMSG_NXTHDR (message, 
> cmsg))

Break this line.

> +    if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
> +      nports += (cmsg->cmsg_len - CMSG_ALIGN (sizeof (struct cmsghdr)))
> +             / sizeof (int);
> +
> +  if (nports)
> +    ports = __alloca (nports * sizeof (mach_port_t));
> +
> +  nports = 0;
> +  for (cmsg = CMSG_FIRSTHDR (message); cmsg; cmsg = CMSG_NXTHDR (message, 
> cmsg))

Break this line.

> +    {
> +      if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
> +     {
> +       fds = (int *) CMSG_DATA (cmsg);
> +       nfds = (cmsg->cmsg_len - CMSG_ALIGN (sizeof (struct cmsghdr)))
> +              / sizeof (int);
> +
> +       for (i = 0; i < nfds; i++)
> +         {
> +           err = HURD_DPORT_USE
> +             (fds[i],
> +              ({
> +                err = __io_restrict_auth (port, &ports[nports++],
> +                                          0, 0, 0, 0);
> +                /* We pass the flags in the control data.  */
> +                fds[i] = descriptor->flags;
> +              }));
> +
> +           if (err)
> +             {
> +               for (i = 0; i < nports - 1; i++)
> +                 __mach_port_deallocate (__mach_task_self (), ports[i]);
> +
> +               if (dealloc)
> +                 __vm_deallocate (__mach_task_self (), data.addr, len);
> +               __hurd_fail (err);
> +             }
> +         }
> +     }
> +    }

You need to break the loop if you get an error, and you should probably
move the cleanup to after __libc_sendmsg.

It's probably best if you only increment nports after a successful
__io_restrict_auth.  It's much easier to keep track of "the number of
restricted ports" than "the number of restricted ports but -1 if there's
an error".

I also just realized that you'll have to restore fds[i] at least on EINTR,
so the caller can retry, but you might as well do it on all errors to
be safe.  Also, don't forget to comment tricky requirements like this.

>    if (addr)
>      {
>        if (addr->sun_family == AF_LOCAL)
> @@ -143,8 +189,9 @@ __libc_sendmsg (int fd, const struct msghdr *message, int 
> flags)
>                             /* Send the data.  */
>                             err = __socket_send (port, aport,
>                                                  flags, data.ptr, len,
> -                                                NULL,
> -                                                MACH_MSG_TYPE_COPY_SEND, 0,
> +                                                ports,
> +                                                MACH_MSG_TYPE_MOVE_SEND,
> +                                                nports,
>                                                  message->msg_control,
>                                                  message->msg_controllen,
>                                                  &amount);

As Roland said, we can't use MACH_MSG_TYPE_MOVE_SEND here (which I
wrongly suggested).  You'll need to add cleanup code here.

Regards,
  Fredrik