diff mbox series

[OpenWrt-Devel,v2,ubus] ubusd/libubus-io: fix socket descriptor passing

Message ID 20191227141519.15626-1-ynezz@true.cz
State Accepted
Delegated to: Petr Štetiar
Headers show
Series [OpenWrt-Devel,v2,ubus] ubusd/libubus-io: fix socket descriptor passing | expand

Commit Message

Petr Štetiar Dec. 27, 2019, 2:15 p.m. UTC 
In commit 5d7ca8309d0a ("ubusd/libubus-io: fix variable sized struct
position warning") the position of cmsghdr struct has been changed in
order to fix clang-9 compiler warning, but it has introduced regression
in at least `logread` which hanged indefinitely.

So this patch reworks the socket descriptor passing in a way recommended
in the `cmsg(3)` manual page.

Ref: http://lists.infradead.org/pipermail/openwrt-devel/2019-December/020840.html
Fixes: 5d7ca8309d0a ("ubusd/libubus-io: fix variable sized struct position warning")
Reported-by: Hannu Nyman <hannu.nyman@welho.com>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
---

 changes since v1:

  - removed unnecessary rename of the `msghdr` variable to new `msg` name

 libubus-io.c | 80 ++++++++++++++++++++++++++++------------------------
 ubusd.c      | 35 ++++++++++++-----------
 ubusd_main.c | 41 ++++++++++++++-------------
 3 files changed, 83 insertions(+), 73 deletions(-)

Comments

Hannu Nyman Dec. 27, 2019, 2:44 p.m. UTC | #1 
Petr Štetiar kirjoitti 27.12.2019 klo 16.15:
> In commit 5d7ca8309d0a ("ubusd/libubus-io: fix variable sized struct
> position warning") the position of cmsghdr struct has been changed in
> order to fix clang-9 compiler warning, but it has introduced regression
> in at least `logread` which hanged indefinitely.
>
> So this patch reworks the socket descriptor passing in a way recommended
> in the `cmsg(3)` manual page.
>
> Ref: http://lists.infradead.org/pipermail/openwrt-devel/2019-December/020840.html
> Fixes: 5d7ca8309d0a ("ubusd/libubus-io: fix variable sized struct position warning")
> Reported-by: Hannu Nyman <hannu.nyman@welho.com>
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---

That seems to take care of the logread problem. Thanks.

But neither collectd not nlbwmon starts. They are still broken with the 
current ubus.


Looking now at the (again working) system log reveals:

root@router2:~# logread | grep collectd
Fri Dec 27 16:23:37 2019 daemon.info procd: Not starting instance 
collectd::instance1, command not set
root@router2:~# logread | grep nlbwmon
Fri Dec 27 16:23:33 2019 daemon.info procd: Not starting instance 
nlbwmon::instance1, command not set
diff mbox series

Patch

diff --git a/libubus-io.c b/libubus-io.c
index ba1016d0fa09..3561ac462eb9 100644
--- a/libubus-io.c
+++ b/libubus-io.c
@@ -59,23 +59,24 @@  static void wait_data(int fd, bool write)
 
 static int writev_retry(int fd, struct iovec *iov, int iov_len, int sock_fd)
 {
-	static struct {
-		int fd;
-		struct cmsghdr h;
-	} fd_buf = {
-		.h = {
-			.cmsg_len = sizeof(fd_buf),
-			.cmsg_level = SOL_SOCKET,
-			.cmsg_type = SCM_RIGHTS,
-		}
-	};
-	struct msghdr msghdr = {
-		.msg_iov = iov,
-		.msg_iovlen = iov_len,
-		.msg_control = &fd_buf,
-		.msg_controllen = sizeof(fd_buf),
-	};
+	uint8_t fd_buf[CMSG_SPACE(sizeof(int))] = { 0 };
+	struct msghdr msghdr = { 0 };
+	struct cmsghdr *cmsg;
 	int len = 0;
+	int *pfd;
+
+	msghdr.msg_iov = iov,
+	msghdr.msg_iovlen = iov_len,
+	msghdr.msg_control = fd_buf;
+	msghdr.msg_controllen = sizeof(fd_buf);
+
+	cmsg = CMSG_FIRSTHDR(&msghdr);
+	cmsg->cmsg_type = SCM_RIGHTS;
+	cmsg->cmsg_level = SOL_SOCKET;
+	cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+
+	pfd = (int *) CMSG_DATA(cmsg);
+	msghdr.msg_controllen = cmsg->cmsg_len;
 
 	do {
 		ssize_t cur_len;
@@ -84,7 +85,7 @@  static int writev_retry(int fd, struct iovec *iov, int iov_len, int sock_fd)
 			msghdr.msg_control = NULL;
 			msghdr.msg_controllen = 0;
 		} else {
-			fd_buf.fd = sock_fd;
+			*pfd = sock_fd;
 		}
 
 		cur_len = sendmsg(fd, &msghdr, 0);
@@ -156,33 +157,38 @@  int __hidden ubus_send_msg(struct ubus_context *ctx, uint32_t seq,
 
 static int recv_retry(struct ubus_context *ctx, struct iovec *iov, bool wait, int *recv_fd)
 {
-	int bytes, total = 0;
-	int fd = ctx->sock.fd;
-	static struct {
-		int fd;
-		struct cmsghdr h;
-	} fd_buf = {
-		.h = {
-			.cmsg_type = SCM_RIGHTS,
-			.cmsg_level = SOL_SOCKET,
-			.cmsg_len = sizeof(fd_buf),
-		},
-	};
-	struct msghdr msghdr = {
-		.msg_iov = iov,
-		.msg_iovlen = 1,
-	};
+	uint8_t fd_buf[CMSG_SPACE(sizeof(int))] = { 0 };
+	struct msghdr msghdr = { 0 };
+	struct cmsghdr *cmsg;
+	int total = 0;
+	int bytes;
+	int *pfd;
+	int fd;
+
+	fd = ctx->sock.fd;
+
+	msghdr.msg_iov = iov,
+	msghdr.msg_iovlen = 1,
+	msghdr.msg_control = fd_buf;
+	msghdr.msg_controllen = sizeof(fd_buf);
+
+	cmsg = CMSG_FIRSTHDR(&msghdr);
+	cmsg->cmsg_type = SCM_RIGHTS;
+	cmsg->cmsg_level = SOL_SOCKET;
+	cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+
+	pfd = (int *) CMSG_DATA(cmsg);
 
 	while (iov->iov_len > 0) {
 		if (recv_fd) {
-			msghdr.msg_control = &fd_buf;
-			msghdr.msg_controllen = sizeof(fd_buf);
+			msghdr.msg_control = fd_buf;
+			msghdr.msg_controllen = cmsg->cmsg_len;
 		} else {
 			msghdr.msg_control = NULL;
 			msghdr.msg_controllen = 0;
 		}
 
-		fd_buf.fd = -1;
+		*pfd = -1;
 		bytes = recvmsg(fd, &msghdr, 0);
 		if (!bytes)
 			return -1;
@@ -199,7 +205,7 @@  static int recv_retry(struct ubus_context *ctx, struct iovec *iov, bool wait, in
 			return 0;
 
 		if (recv_fd)
-			*recv_fd = fd_buf.fd;
+			*recv_fd = *pfd;
 
 		recv_fd = NULL;
 
diff --git a/ubusd.c b/ubusd.c
index 0d43977c0bde..5993653785e0 100644
--- a/ubusd.c
+++ b/ubusd.c
@@ -82,27 +82,28 @@  void ubus_msg_free(struct ubus_msg_buf *ub)
 
 ssize_t ubus_msg_writev(int fd, struct ubus_msg_buf *ub, size_t offset)
 {
+	uint8_t fd_buf[CMSG_SPACE(sizeof(int))] = { 0 };
 	static struct iovec iov[2];
-	static struct {
-		int fd;
-		struct cmsghdr h;
-	} fd_buf = {
-		.h = {
-			.cmsg_len = sizeof(fd_buf),
-			.cmsg_level = SOL_SOCKET,
-			.cmsg_type = SCM_RIGHTS,
-		},
-	};
-	struct msghdr msghdr = {
-		.msg_iov = iov,
-		.msg_iovlen = ARRAY_SIZE(iov),
-		.msg_control = &fd_buf,
-		.msg_controllen = sizeof(fd_buf),
-	};
+	struct msghdr msghdr = { 0 };
 	struct ubus_msghdr hdr;
+	struct cmsghdr *cmsg;
 	ssize_t ret;
+	int *pfd;
 
-	fd_buf.fd = ub->fd;
+	msghdr.msg_iov = iov;
+	msghdr.msg_iovlen = ARRAY_SIZE(iov);
+	msghdr.msg_control = fd_buf;
+	msghdr.msg_controllen = sizeof(fd_buf);
+
+	cmsg = CMSG_FIRSTHDR(&msghdr);
+	cmsg->cmsg_type = SCM_RIGHTS;
+	cmsg->cmsg_level = SOL_SOCKET;
+	cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+
+	pfd = (int *) CMSG_DATA(cmsg);
+	msghdr.msg_controllen = cmsg->cmsg_len;
+
+	*pfd = ub->fd;
 	if (ub->fd < 0 || offset) {
 		msghdr.msg_control = NULL;
 		msghdr.msg_controllen = 0;
diff --git a/ubusd_main.c b/ubusd_main.c
index 81868c1482bc..f977ff30d12d 100644
--- a/ubusd_main.c
+++ b/ubusd_main.c
@@ -50,22 +50,25 @@  static void handle_client_disconnect(struct ubus_client *cl)
 static void client_cb(struct uloop_fd *sock, unsigned int events)
 {
 	struct ubus_client *cl = container_of(sock, struct ubus_client, sock);
+	uint8_t fd_buf[CMSG_SPACE(sizeof(int))] = { 0 };
+	struct msghdr msghdr = { 0 };
 	struct ubus_msg_buf *ub;
 	static struct iovec iov;
-	static struct {
-		int fd;
-		struct cmsghdr h;
-	} fd_buf = {
-		.h = {
-			.cmsg_type = SCM_RIGHTS,
-			.cmsg_level = SOL_SOCKET,
-			.cmsg_len = sizeof(fd_buf),
-		}
-	};
-	struct msghdr msghdr = {
-		.msg_iov = &iov,
-		.msg_iovlen = 1,
-	};
+	struct cmsghdr *cmsg;
+	int *pfd;
+
+	msghdr.msg_iov = &iov,
+	msghdr.msg_iovlen = 1,
+	msghdr.msg_control = fd_buf;
+	msghdr.msg_controllen = sizeof(fd_buf);
+
+	cmsg = CMSG_FIRSTHDR(&msghdr);
+	cmsg->cmsg_type = SCM_RIGHTS;
+	cmsg->cmsg_level = SOL_SOCKET;
+	cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+
+	pfd = (int *) CMSG_DATA(cmsg);
+	msghdr.msg_controllen = cmsg->cmsg_len;
 
 	/* first try to tx more pending data */
 	while ((ub = ubus_msg_head(cl))) {
@@ -100,14 +103,14 @@  retry:
 		int offset = cl->pending_msg_offset;
 		int bytes;
 
-		fd_buf.fd = -1;
+		*pfd = -1;
 
 		iov.iov_base = ((char *) &cl->hdrbuf) + offset;
 		iov.iov_len = sizeof(cl->hdrbuf) - offset;
 
 		if (cl->pending_msg_fd < 0) {
-			msghdr.msg_control = &fd_buf;
-			msghdr.msg_controllen = sizeof(fd_buf);
+			msghdr.msg_control = fd_buf;
+			msghdr.msg_controllen = cmsg->cmsg_len;
 		} else {
 			msghdr.msg_control = NULL;
 			msghdr.msg_controllen = 0;
@@ -117,8 +120,8 @@  retry:
 		if (bytes < 0)
 			goto out;
 
-		if (fd_buf.fd >= 0)
-			cl->pending_msg_fd = fd_buf.fd;
+		if (*pfd >= 0)
+			cl->pending_msg_fd = *pfd;
 
 		cl->pending_msg_offset += bytes;
 		if (cl->pending_msg_offset < (int) sizeof(cl->hdrbuf))