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