Re: [PATCH 08/11] dbus: Remove memcpy and fix setting of FD_CLOEXEC on FDs

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

Hi Andrew,

On 04/28/2016 10:32 PM, Andrew Zaborowski wrote:
> Make sure the size of the FD buffer passed to recvmsg is properly
> aligned, etc. and remove the memcpy which seems unneeded.  Also it looks
> like the fcntl() calls operated on one dbus socket FD instead of the
> message FDs.
> ---
>   ell/dbus.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/ell/dbus.c b/ell/dbus.c
> index 70da849..fc0f243 100644
> --- a/ell/dbus.c
> +++ b/ell/dbus.c
> @@ -638,7 +638,8 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
>   	ssize_t len;
>   	void *header, *body;
>   	size_t header_size, body_size;
> -	int fds[16];
> +	uint8_t fd_buf[CMSG_SPACE(16 * sizeof(int))];

'man cmsg' recommends doing:
            union {
                /* ancillary data buffer, wrapped in a union in order to 
ensure
                   it is suitably aligned */
                char buf[CMSG_SPACE(sizeof myfds)];
                struct cmsghdr align;
            } u;

And it looks like systemd does the same.  This is probably what we 
should do as well.

> +	int *fds = NULL;
>   	uint32_t num_fds = 0;
>
>   	len = recv(fd, &hdr, DBUS_HEADER_SIZE, MSG_PEEK);
> @@ -659,8 +660,8 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
>   	memset(&msg, 0, sizeof(msg));
>   	msg.msg_iov = iov;
>   	msg.msg_iovlen = 2;
> -	msg.msg_control = &fds;
> -	msg.msg_controllen = CMSG_SPACE(16 * sizeof(int));
> +	msg.msg_control = &fd_buf;
> +	msg.msg_controllen = sizeof(fd_buf);
>
>   	len = recvmsg(fd, &msg, MSG_CMSG_CLOEXEC);
>   	if (len < 0)
> @@ -675,19 +676,18 @@ static struct l_dbus_message *classic_recv_message(struct l_dbus *dbus)
>   			continue;
>
>   		num_fds = (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int);
> -
> -		memcpy(fds, CMSG_DATA(cmsg), num_fds * sizeof(int));
> +		fds = (void *) CMSG_DATA(cmsg);
>
>   		/* Set FD_CLOEXEC on all file descriptors */
>   		for (i = 0; i < num_fds; i++) {
>   			long flags;
>
> -			flags = fcntl(fd, F_GETFD, NULL);
> +			flags = fcntl(fds[i], F_GETFD, NULL);
>   			if (flags < 0)
>   				continue;
>
>   			if (!(flags & FD_CLOEXEC))
> -				fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
> +				fcntl(fds[i], F_SETFD, flags | FD_CLOEXEC);
>                   }
>   	}
>
>

Regards,
-Denis