segfault after reallocation (Was: malloc: Change mmap chunk layout)

  Hello,

In Debian GNU/Hurd, starting with glibc 2.43, we see odd perfectly
reproducible segfaults in clang++, moc and others. What we observe is
some pointers unexpectedly getting NULL. I bisected that into

commit 614cfd0f8a2820aed54f9745077c7da0e6643bac
Author: Wilco Dijkstra <wilco.dijkstra@arm.com>
Date:   Fri Aug 8 14:11:13 2025 +0000

    malloc: Change mmap chunk layout

    Change the mmap chunk layout to be identical to a normal chunk.  This makes it
    safe for tcache to hold mmap chunks and simplifies size calculations in
    memsize and musable.  Add mmap_base() and mmap_size() macros to simplify code.

    Reviewed-by: Cupertino Miranda <cupertino.miranda@oracle.com>

Running with MALLOC_MMAP_MAX_=0 does not reproduce the issue. I tracked
the behavior down to a crash after a reallocation for mmap-backed
memory, and noticed something odd: in __libc_realloc, in the mmap case,
after failing to use mremap_chunk, we have

  if (chunk_is_mmapped (oldp))
    {
      [...]
      memcpy (newmem, oldmem, oldsize - CHUNK_HDR_SZ);
      munmap_chunk (oldp);
      return newmem;

Shouldn't this be

  if (chunk_is_mmapped (oldp))
    {
      [...]
      memcpy (newmem, oldmem, memsize (oldp));
      munmap_chunk (oldp);
      return newmem;

like in the other places of the file? Doing this change does make sense
to me and does fix the segfaults for me, see the proposed patch below.

Samuel


commit 7b031195e20d539f61e6465d843485b839375b0f
Author: Samuel Thibault <samuel.thibault@ens-lyon.org>
Date:   Mon Sep 28 01:23:15 2026 +0200

    realloc: Fix mmap non-mremap reallocation case
    
    This case was missing copying SIZE_SZ bytes from the old allocation.
    
    Fixes commit 614cfd0f8a2820aed54f9745077c7da0e6643bac
    ("malloc: Change mmap chunk layout"), which went into glibc 2.43.
  

Hi Sam,

> Shouldn't this be
>
>  if (chunk_is_mmapped (oldp))
>    {
>      [...]
>      memcpy (newmem, oldmem, memsize (oldp));
>      munmap_chunk (oldp);
>      return newmem;
>
> like in the other places of the file? Doing this change does make sense
> to me and does fix the segfaults for me, see the proposed patch below.

Yes it looks like we missed that case...

> -      memcpy (newmem, oldmem, oldsize - CHUNK_HDR_SZ);
> +      memcpy (newmem, oldmem, memsize (oldp));
>       munmap_chunk (oldp);
>       return newmem;

I'd use "usable" here since that already contains memsize.

I wonder how easy it is to test this - I think it can only happen if you don't
have mremap (or if it fails but you can still allocate memory).

Cheers,
Wilco
  

Hi Sam,

Did you create a PR for this? That would be useful for backporting.

On testing, something like this confirms the bug if HAVE_MREMAP is 0 
(missing error checking):

#define N1 (1024*1024*2 - 3 * sizeof (size_t))
#define N2 (N1 + 65536)
void realloc_bug (void)
{
  char *p = (char*)malloc (N1);
  for (int i = 0; i < N1; i++)
    p[i] = i;

  p = (char*) realloc (p, N2);
  for (int i = 0; i < N1; i++)
    if (p[i] != (char)i)
      printf("FAIL %d\n", i);
}

Cheers,
Wilco
  

Hello,

Wilco Dijkstra, le lun. 28 sept. 2026 13:03:28 +0000, a ecrit:
> > Shouldn't this be
> >
> >  if (chunk_is_mmapped (oldp))
> >    {
> >      [...]
> >      memcpy (newmem, oldmem, memsize (oldp));
> >      munmap_chunk (oldp);
> >      return newmem;
> >
> > like in the other places of the file? Doing this change does make sense
> > to me and does fix the segfaults for me, see the proposed patch below.
> 
> Yes it looks like we missed that case...

Ok, thanks for confirming that I'm not out of my mind :)

> > -      memcpy (newmem, oldmem, oldsize - CHUNK_HDR_SZ);
> > +      memcpy (newmem, oldmem, memsize (oldp));
> >       munmap_chunk (oldp);
> >       return newmem;
> 
> I'd use "usable" here since that already contains memsize.

Ok, that's less coherent with other such memcpy calls, but coherent with
the code above.

> I wonder how easy it is to test this - I think it can only happen if you don't
> have mremap (or if it fails but you can still allocate memory).

Yes, that's it.

Wilco Dijkstra, le lun. 28 sept. 2026 16:43:43 +0000, a ecrit:
> Did you create a PR for this? That would be useful for backporting.

I have now sent a formal patch to libc-alpha.

> On testing, something like this confirms the bug if HAVE_MREMAP is 0 
> (missing error checking):
> 
> #define N1 (1024*1024*2 - 3 * sizeof (size_t))
> #define N2 (N1 + 65536)
> void realloc_bug (void)
> {
>   char *p = (char*)malloc (N1);
>   for (int i = 0; i < N1; i++)
>     p[i] = i;
> 
>   p = (char*) realloc (p, N2);
>   for (int i = 0; i < N1; i++)
>     if (p[i] != (char)i)
>       printf("FAIL %d\n", i);
> }

Yes, we'd need a malloc tunable to be able to disable mremap
dynamically.

Samuel