[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Add support to send file descriptors over Unix sockets
From: |
Carl Fredrik Hammar |
Subject: |
Re: [PATCH] Add support to send file descriptors over Unix sockets |
Date: |
Sun, 1 Aug 2010 21:02:57 +0200 |
User-agent: |
Mutt/1.5.20 (2009-06-14) |
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
- Re: [PATCH] Add support to send file descriptors over Unix sockets,
Carl Fredrik Hammar <=