[musl] struct mq_attr wrong on x32?

[musl] struct mq_attr wrong on x32?

[Markus Wichmann]

Hi all,

just something I saw in the code just now. I didn't test it, but I think
struct mq_attr is wrong for x32. The kernel defines the structure like
this:

struct mq_attr {
    __kernel_long_t mq_flags;       /* message queue flags                  */
    __kernel_long_t mq_maxmsg;      /* maximum number of messages           */
    __kernel_long_t mq_msgsize;     /* maximum message size                 */
    __kernel_long_t mq_curmsgs;     /* number of messages currently queued  */
    __kernel_long_t __reserved[4];  /* ignored for input, zeroed for output */
};

The type __kernel_long_t is defined as long on all architectures except
x32, where it is defined as long long. musl defines the same structure
with the type long on all architectures. POSIX requires these fields to
be typed as long, so the only choice we have is to define mq_attr as

struct mq_attr {
    long mq_flags;
    long __r0;
    long mq_maxmsg;
    long __r1;
    long mq_msgsize;
    long __r2;
    long mq_curmsgs;
    long __r3;
    long long __reserved[4]; /* sets correct structure alignment and size */
};

on x32 only. Yeah, I know, it is ugly. We could put this in a bits/
header, but it would have the same content on all architectures except
one, so that would be ugly as well. Auditing them would turn into a
find-the-differences puzzle. Oh well.

Thoughts?

Ciao,
Markus

Re: [musl] struct mq_attr wrong on x32?

[Rich Felker]

On Tue, Dec 17, 2024 at 07:46:28PM +0100, Markus Wichmann wrote:
> Hi all,
> 
> just something I saw in the code just now. I didn't test it, but I think
> struct mq_attr is wrong for x32. The kernel defines the structure like
> this:
> 
> struct mq_attr {
>     __kernel_long_t mq_flags;       /* message queue flags                  */
>     __kernel_long_t mq_maxmsg;      /* maximum number of messages           */
>     __kernel_long_t mq_msgsize;     /* maximum message size                 */
>     __kernel_long_t mq_curmsgs;     /* number of messages currently queued  */
>     __kernel_long_t __reserved[4];  /* ignored for input, zeroed for output */
> };
> 
> The type __kernel_long_t is defined as long on all architectures except
> x32, where it is defined as long long. musl defines the same structure
> with the type long on all architectures. POSIX requires these fields to
> be typed as long, so the only choice we have is to define mq_attr as
> 
> struct mq_attr {
>     long mq_flags;
>     long __r0;
>     long mq_maxmsg;
>     long __r1;
>     long mq_msgsize;
>     long __r2;
>     long mq_curmsgs;
>     long __r3;
>     long long __reserved[4]; /* sets correct structure alignment and size */
> };
> 
> on x32 only. Yeah, I know, it is ugly. We could put this in a bits/
> header, but it would have the same content on all architectures except
> one, so that would be ugly as well. Auditing them would turn into a
> find-the-differences puzzle. Oh well.

I recall something about this coming up before. I'll dig and see what
I can find. It's not clear to me if we should fix this in the
interface type (leaving behind any broken stuff already compiled using
the mismatched definition) or translate it in libc (keeping ABI same
and fixing existing binaries too).

Rich

Re: [musl] struct mq_attr wrong on x32?

[Markus Wichmann]

Am Sat, Dec 21, 2024 at 01:15:41AM -0500 schrieb Rich Felker:
> On Tue, Dec 17, 2024 at 07:46:28PM +0100, Markus Wichmann wrote:
>
> > [struct mq_attr is wrong on x32]
> I recall something about this coming up before. I'll dig and see what
> I can find. It's not clear to me if we should fix this in the
> interface type (leaving behind any broken stuff already compiled using
> the mismatched definition) or translate it in libc (keeping ABI same
> and fixing existing binaries too).
>
> Rich

I also found struct rusage to be wrong in a similar way. The kernel
struct is defined in terms of __kernel_long_t again, and musl
substitutes that for long. So that would add a few translations.

At this point, since the wrong definitions are already published,
translation makes the most sense, of course.

Ciao,
Markus

Re: [musl] struct mq_attr wrong on x32?

[Thorsten Glaser]

On Sat, 21 Dec 2024, Markus Wichmann wrote:

>At this point, since the wrong definitions are already published,
>translation makes the most sense, of course.

Might not even be needed, as the new and old structures are
the same in memory, so while it’s an API change (correction)
it’s not an ABI change, as x32 is little endian.

bye,
//mirabilos
-- 
Yay for having to rewrite other people's Bash scripts because bash
suddenly stopped supporting the bash extensions they make use of
	-- Tonnerre Lombard in #nosec

Re: [musl] struct mq_attr wrong on x32?

[Markus Wichmann]

Am Sun, Dec 22, 2024 at 12:02:54AM +0100 schrieb Thorsten Glaser:
> On Sat, 21 Dec 2024, Markus Wichmann wrote:
>
> >At this point, since the wrong definitions are already published,
> >translation makes the most sense, of course.
>
> Might not even be needed, as the new and old structures are
> the same in memory, so while it’s an API change (correction)
> it’s not an ABI change, as x32 is little endian.
>
> bye,
> //mirabilos

I think we might be talking past each other. The musl definition of
these structures is a copy of the kernel definition, but substituting
long for "__kernel_long_t". On x32, "long" is a 32-bit type, but
"__kernel_long_t" is a 64-bit type. For that reason, the offsets of all
the fields but the first are off. And the size as well, maybe leading to
memory corruption. And of course, only x32 is affected by this.

In most other cases where "__kernel_long_t" makes an appearance in the
kernel headers, the structures are already arch-dependent (e.g. the SysV
IPC stuff), so adjusting the x32 version for this discrepancy is not an
issue, but the two cases I found here are supposed to be
arch-independent, but aren't.

musl cares about ABI, so we need a translation solution here, and I
don't really see an elegant way that avoids both conditional compilation
and useless translation on the other architectures (where the types
match). Might be conditional compilation is the smaller evil here.

Ciao,
Markus

Re: [musl] struct mq_attr wrong on x32?

[Rich Felker]

On Sat, Dec 21, 2024 at 06:50:55PM +0100, Markus Wichmann wrote:
> Am Sat, Dec 21, 2024 at 01:15:41AM -0500 schrieb Rich Felker:
> > On Tue, Dec 17, 2024 at 07:46:28PM +0100, Markus Wichmann wrote:
> >
> > > [struct mq_attr is wrong on x32]
> > I recall something about this coming up before. I'll dig and see what
> > I can find. It's not clear to me if we should fix this in the
> > interface type (leaving behind any broken stuff already compiled using
> > the mismatched definition) or translate it in libc (keeping ABI same
> > and fixing existing binaries too).
> >
> > Rich
> 
> I also found struct rusage to be wrong in a similar way. The kernel
> struct is defined in terms of __kernel_long_t again, and musl
> substitutes that for long. So that would add a few translations.

struct rusage is already translated.

Re: [musl] struct mq_attr wrong on x32?

[Thorsten Glaser]

On Sun, 22 Dec 2024, Markus Wichmann wrote:

>For that reason, the offsets of all the fields but the first are off.

Ah. I somehow misread that part and thought only the sizes were off.
Sorry.

>musl cares about ABI, so we need a translation solution here, and I

Right.

bye,
//mirabilos
-- 
[16:04:33] bkix: "veni vidi violini"
[16:04:45] bkix: "ich kam, sah und vergeigte"...

Re: [musl] struct mq_attr wrong on x32?

[Rich Felker]

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

On Sat, Dec 21, 2024 at 06:50:55PM +0100, Markus Wichmann wrote:
> Am Sat, Dec 21, 2024 at 01:15:41AM -0500 schrieb Rich Felker:
> > On Tue, Dec 17, 2024 at 07:46:28PM +0100, Markus Wichmann wrote:
> >
> > > [struct mq_attr is wrong on x32]
> > I recall something about this coming up before. I'll dig and see what
> > I can find. It's not clear to me if we should fix this in the
> > interface type (leaving behind any broken stuff already compiled using
> > the mismatched definition) or translate it in libc (keeping ABI same
> > and fixing existing binaries too).
> >
> > Rich
> 
> I also found struct rusage to be wrong in a similar way. The kernel
> struct is defined in terms of __kernel_long_t again, and musl
> substitutes that for long. So that would add a few translations.
> 
> At this point, since the wrong definitions are already published,
> translation makes the most sense, of course.

I think these x32-specific implementations for the functions would
work. They're tested to compile but not runtime tested.

Arguably it might be better to put the code conditionally in the main
source files; I'm not sure.

Rich

[-- Attachment #2: mq_open.c --]
[-- Type: text/plain, Size: 533 bytes --]

#include <mqueue.h>
#include <fcntl.h>
#include <stdarg.h>
#include "syscall.h"

mqd_t mq_open(const char *name, int flags, ...)
{
	mode_t mode = 0;
	struct mq_attr *attr = 0;
	long long attrbuf[8];
	if (*name == '/') name++;
	if (flags & O_CREAT) {
		va_list ap;
		va_start(ap, flags);
		mode = va_arg(ap, mode_t);
		attr = va_arg(ap, struct mq_attr *);
		if (attr) for (int i=0; i<8; i++)
			attrbuf[i] = *(long *)((char *)attr + i*sizeof(long));
		va_end(ap);
	}
	return syscall(SYS_mq_open, name, flags, mode, attr?attrbuf:0);
}

[-- Attachment #3: mq_setattr.c --]
[-- Type: text/plain, Size: 454 bytes --]

#include <mqueue.h>
#include "syscall.h"

int mq_setattr(mqd_t mqd, const struct mq_attr *restrict new, struct mq_attr *restrict old)
{
	long long attr[8];
	if (new) for (int i=0; i<8; i++)
		attr[i] = *(long *)((char *)new + i*sizeof(long));
	int ret = __syscall(SYS_mq_getsetattr, mqd, new?attr:0, old?attr:0);
	if (ret < 0) return __syscall_ret(ret);
	if (old) for (int i=0; i<8; i++)
		*(long *)((char *)old + i*sizeof(long)) = attr[i];
	return 0;
}

Re: [musl] struct mq_attr wrong on x32?

[Markus Wichmann]

Am Sun, Dec 22, 2024 at 07:54:05AM -0500 schrieb Rich Felker:
> I think these x32-specific implementations for the functions would
> work. They're tested to compile but not runtime tested.
>

Ooh, right, arch-specific implementations. That would work.

The functions look good to me.

Ciao,
Markus