[musl] __convert_scm_timestamps() probably breaks with struct timeval

[musl] __convert_scm_timestamps() probably breaks with struct timeval

[Markus Wichmann]

[-- 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

Re: [musl] __convert_scm_timestamps() probably breaks with struct timeval

[Rich Felker]

On Fri, Jun 02, 2023 at 04:49:37PM +0200, Markus Wichmann wrote:
> 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.

I'm confused what problem you're seeing. There is no conversion
in-place. The function finds a timeval or timespec in the old cmsg
array, extracts the value to a local variable that's a pair of long
long, and finally *appends* that as a new cmsg to the cmsg array.

Rich

Re: [musl] __convert_scm_timestamps() probably breaks with struct timeval

[Alexey Izbyshev]

On 2023-06-02 17:49, Markus Wichmann wrote:
> 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.

Hi,

timeval is specified in alltypes.h.in as:

STRUCT timeval { time_t tv_sec; suseconds_t tv_usec; };

And suseconds_t as:

TYPEDEF _Int64 suseconds_t;

So timeval and timespec structs have the same layout in musl, and it 
matches long long[2]. Maybe you assumed that suseconds_t is a 32-bit 
type?

Alexey

Re: [musl] __convert_scm_timestamps() probably breaks with struct timeval

[Markus Wichmann]

Hello,

never mind. I overlooked that you define suseconds_t as _Int64. I
thought it was long. Of course, that changes everything.

Wondering why I thought it was long, I looked at POSIX again, and it
says:

|The implementation shall support one or more programming environments
|in which the [width] of [...] suseconds_t [...] [is] no greater than the
|width of type long.

(Quoted from https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html)

So suseconds_t is not supposed to be larger than long. But I suppose you
can only pick your poison here. Disregard that one requirement of POSIX
or make timeval and timespec have different representations.

Ciao,
Markus

Re: [musl] __convert_scm_timestamps() probably breaks with struct timeval

[Rich Felker]

On Fri, Jun 02, 2023 at 05:55:23PM +0200, Markus Wichmann wrote:
> Hello,
> 
> never mind. I overlooked that you define suseconds_t as _Int64. I
> thought it was long. Of course, that changes everything.
> 
> Wondering why I thought it was long, I looked at POSIX again, and it
> says:
> 
> |The implementation shall support one or more programming environments
> |in which the [width] of [...] suseconds_t [...] [is] no greater than the
> |width of type long.
> 
> (Quoted from https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html)

That's legacy cruft from C89 days that used to require an environment
where off_t is no wider than long, which probably led to the whole
paragraph being ignored.

> So suseconds_t is not supposed to be larger than long. But I suppose you
> can only pick your poison here. Disregard that one requirement of POSIX
> or make timeval and timespec have different representations.

It's not about whether timeval and timespec have different
representations; that'd be fine, and in fact they do!

Rather, it's about whether the user and kernelspace (64-bit) timeval
types are in agreement. If not, there's a lot more mess to deal with.

Rich