[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20260512125844.GG1827@brightrain.aerifal.cx>
Date: Tue, 12 May 2026 08:58:44 -0400
From: Rich Felker <dalias@...c.org>
To: Kalven Schraut <kalvens@...ision.com>
Cc: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: Re: posix_memalign with 4 GiB alignment returns pointer that
crashes when free is called
On Mon, May 11, 2026 at 11:31:08PM -0400, Rich Felker wrote:
> On Tue, May 12, 2026 at 02:21:25AM +0000, Kalven Schraut wrote:
> > Hi,
> > I found a crash in musl 1.2.5 on x86_64 where posix_memalign
> > returns success for a 4 GiB alignment, but the returned pointer
> > later crashes in free.
> >
> > Minimal repro:
> >
> > #include <stdlib.h>
> > #include <stdio.h>
> > int main(void) {
> > void *p = 0;
> > int ret = posix_memalign(&p, 1ULL << 32, (1ULL << 31) - 16);
> > printf("ret=%d ptr=%p\n", ret, p);
> > if (ret != 0) return ret;
> > free(p);
> > return 0;
> > }
> >
> > Observed:
> > ret=0 ptr=0x7fff00000000
> > Program received signal SIGSEGV, Segmentation fault.
> > get_meta (p=0x7fff00000000) at src/malloc/mallocng/meta.h:141
> > #0 get_meta
> > #1 __libc_free
> > #2 main
> >
> > I tested powers-of-two alignments. 2 GiB alignment frees successfully,
> > while 4 GiB, 8 GiB, and 16 GiB alignments crash on free. 32 GiB is
> > rejected with ENOMEM.
> >
> >
> > AI tooling helped me debug this issue and
> > It seems to think https://git.musl-libc.org/cgit/musl/tree/src/malloc/mallocng/meta.h#n139
> > uint32_t offset = (size_t)(p-g->mem->storage)/UNIT;
> > Is hitting a signed int overflow due to the large alignment
> >
> > This was discovered with a recent change done by rust based linter for js in github PR https://github.com/oxc-project/oxc/pull/22088
>
> Thanks. This looks rather harmless in practice, since such a large
> alignment doesn't make sense to use, but indeed it should be fixed. I
> think the correct fix is just changing the type of the local 'offset'
> to size_t.
Analysis for this fix, particularly how size and signedness of the
type of offset affect the code that follows:
1. As a side effect of signedness, tThe line assert(offset > 0xffff)
presently catches and traps on values that would be interpreted as
negative when assigned to int. However that wasn't really necessary.
What is potentially useful is ensuring that offset*UNIT does not
overflow, which would call for an additional check:
assert(offset <= PTRDIFF_MAX/UNIT)
This is a no-op for 64-bit (should be optimized out) but has an effect
for 32-bit (it asserts offset is at most 0xfffffff).
The only value of either (any) of these checks is hardening, so it
could be omitted since it wasn't checked before anyway. But it's an
extra signal we could use to deduce that out-of-bounds memory writes
have happened, and since it only appears in the case for gigantic,
overaligned memory, the relative cost is effectively zero.
2. As is the point, making offset have type size_t also changes the
result of offset*UNIT. That's the whole point.
3. The checks against the slot in the group using the size clsas and
index behave differently for an offset that would have been
interpreted as negative, but taken together the behavior does not
change. The first assert is no longer triggered but the second
necessarily is.
4. The check aginst maplen is not changed because the *4096UL already
coerced both sides to unsigned long.
Based on this, it's safe/fine to change to size_t, and optionally
(preferably) add the trap for offset*UNIT overflow.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
