[musl] __convert_scm_timestamps() probably breaks with struct timeval

[Markus Wichmann <nullplan@gmx.net>]

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

Hi all,

I couldn't think of a better subject. For unrelated reasons, today I
stumbled upon the abovenamed function. It looks through all control
messages in a msghdr, looking for SCM_TIMESTAMP_OLD and
SCM_TIMESTAMPNS_OLD, and converting an array two long into an array of
two long long.

This will work with struct timespec (or SCM_TIMESTAMPNS_OLD) on all
architectures, because struct timespec has been defined specifically for
this to work (due to the kernel accessing it the same way). But it will
break with struct timeval (or SCM_TIMESTAMP_OLD).

First of all, on some 32-bit archs (and this code is only used on 32-bit
archs) we have alignof(int64_t) == 4, and therefore sizeof (struct
timeval) == 12. Therefore CMSG_LEN(sizeof (long long[2])) !=
CMSG_LEN(struct timeval), and therefore, applications won't read the
newly converted cmsg. All the work for nothing.

Next, even if alignof(int64_t) == 8, struct timeval does not have the
same padding as struct timespec, and so on big endian architectures, the
code ends up writing the microseconds into the padding and clearing the
tv_usec field.

This can all be solved easily, if you just try to be a touch less
clever. I am attaching a patch that makes the intent more obvious. It's
not as flashy as the previous code. It doesn't have a switch with a goto
in it. I hope it finds your favor.

Ciao,
Markus

[-- Attachment #2: 0001-Fix-time64-conversion-of-SCM_TIMESTAMP.patch --]
[-- Type: text/x-diff, Size: 3014 bytes --]

From adf608ddce58d652fc074d54e580fb724c35705b Mon Sep 17 00:00:00 2001
From: Markus Wichmann <nullplan@gmx.net>
Date: Fri, 2 Jun 2023 16:41:56 +0200
Subject: [PATCH] Fix time64 conversion of SCM_TIMESTAMP.

On 32-bit archs, the newly created cmsg might get the wrong length (if
alignof(int64_t) == 4), and might write the microseconds in the wrong
place (if byte order is big endian).

Besides, the old code went a long way obscuring its intent to give
preference to SCM_TIMESTAMPNS. Don't know what it would matter, since
according to the documentation only one of these types can be enabled at
one time. Unless the docs are wrong.
---
 src/network/recvmsg.c | 47 ++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/src/network/recvmsg.c b/src/network/recvmsg.c
index 03641625..d990c620 100644
--- a/src/network/recvmsg.c
+++ b/src/network/recvmsg.c
@@ -12,39 +12,44 @@ void __convert_scm_timestamps(struct msghdr *msg, socklen_t csize)
 	if (SCM_TIMESTAMP == SCM_TIMESTAMP_OLD) return;
 	if (!msg->msg_control || !msg->msg_controllen) return;

-	struct cmsghdr *cmsg, *last=0;
-	long tmp;
-	long long tvts[2];
+	struct cmsghdr *cmsg, *last=0, *found=0;
 	int type = 0;
+	void *data;
+	size_t len;
+	struct timespec ts;
+	struct timeval tv;

 	for (cmsg=CMSG_FIRSTHDR(msg); cmsg; cmsg=CMSG_NXTHDR(msg, cmsg)) {
-		if (cmsg->cmsg_level==SOL_SOCKET) switch (cmsg->cmsg_type) {
-		case SCM_TIMESTAMP_OLD:
-			if (type) break;
-			type = SCM_TIMESTAMP;
-			goto common;
-		case SCM_TIMESTAMPNS_OLD:
-			type = SCM_TIMESTAMPNS;
-		common:
-			memcpy(&tmp, CMSG_DATA(cmsg), sizeof tmp);
-			tvts[0] = tmp;
-			memcpy(&tmp, CMSG_DATA(cmsg) + sizeof tmp, sizeof tmp);
-			tvts[1] = tmp;
-			break;
+		if (cmsg->cmsg_level==SOL_SOCKET) {
+			if ((cmsg->cmsg_type == SCM_TIMESTAMP_OLD && !found)
+			  || cmsg->cmsg_type == SCM_TIMESTAMPNS_OLD)
+				found = cmsg;
 		}
 		last = cmsg;
 	}
-	if (!last || !type) return;
-	if (CMSG_SPACE(sizeof tvts) > csize-msg->msg_controllen) {
+	if (!found) return;
+	const long *old = (void *)CMSG_DATA(found);
+	if (found->cmsg_type == SCM_TIMESTAMP_OLD) {
+		type = SCM_TIMESTAMP;
+		data = &tv;
+		len = sizeof tv;
+		tv = (struct timeval){.tv_sec = old[0], .tv_usec = old[1]};
+	} else {
+		type = SCM_TIMESTAMPNS;
+		data = &ts;
+		len = sizeof ts;
+		ts = (struct timespec){.tv_sec = old[0], .tv_nsec = old[1]};
+	}
+	if (CMSG_SPACE(len) > csize-msg->msg_controllen) {
 		msg->msg_flags |= MSG_CTRUNC;
 		return;
 	}
-	msg->msg_controllen += CMSG_SPACE(sizeof tvts);
+	msg->msg_controllen += CMSG_SPACE(len);
 	cmsg = CMSG_NXTHDR(msg, last);
 	cmsg->cmsg_level = SOL_SOCKET;
 	cmsg->cmsg_type = type;
-	cmsg->cmsg_len = CMSG_LEN(sizeof tvts);
-	memcpy(CMSG_DATA(cmsg), &tvts, sizeof tvts);
+	cmsg->cmsg_len = CMSG_LEN(len);
+	memcpy(CMSG_DATA(cmsg), data, len);
 }

 ssize_t recvmsg(int fd, struct msghdr *msg, int flags)
--
2.39.2