[v1] malloc: set NON_MAIN_ARENA flag for reclaimed memalign chunk (BZ #30101)

  From 61bd502ecac4d63f04c74bfc491ca675660d26b7 Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Mon, 3 Apr 2023 17:33:03 -0400
Subject: malloc: set NON_MAIN_ARENA flag for reclaimed memalign chunk (BZ #30101)

Based on these comments in malloc.c:

   size field is or'ed with NON_MAIN_ARENA if the chunk was obtained
   from a non-main arena.  This is only set immediately before handing
   the chunk to the user, if necessary.

   The NON_MAIN_ARENA flag is never set for unsorted chunks, so it
   does not have to be taken into account in size comparisons.

When we pull a chunk off the unsorted list (or any list) we need to
make sure that flag is set properly before returning the chunk.
  

* DJ Delorie via Libc-alpha:

> From 61bd502ecac4d63f04c74bfc491ca675660d26b7 Mon Sep 17 00:00:00 2001
> From: DJ Delorie <dj@redhat.com>
> Date: Mon, 3 Apr 2023 17:33:03 -0400
> Subject: malloc: set NON_MAIN_ARENA flag for reclaimed memalign chunk (BZ #30101)
>
> Based on these comments in malloc.c:
>
>    size field is or'ed with NON_MAIN_ARENA if the chunk was obtained
>    from a non-main arena.  This is only set immediately before handing
>    the chunk to the user, if necessary.
>
>    The NON_MAIN_ARENA flag is never set for unsorted chunks, so it
>    does not have to be taken into account in size comparisons.
>
> When we pull a chunk off the unsorted list (or any list) we need to
> make sure that flag is set properly before returning the chunk.
>
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 0315ac5d16..66e7ca57dd 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -5147,6 +5147,8 @@ _int_memalign (mstate av, size_t alignment, size_t bytes)
>        p = victim;
>        m = chunk2mem (p);
>        set_inuse (p);
> +      if (av != &main_arena)
> +	set_non_main_arena (p);
>      }
>    else
>      {

The change looks reasonable.

Can we add a test for this?  Maybe run the existing memalign tests on a
second thread as well?

Thanks,
Florian
  

On 4/3/23 18:12, DJ Delorie via Libc-alpha wrote:
> 
> From 61bd502ecac4d63f04c74bfc491ca675660d26b7 Mon Sep 17 00:00:00 2001
> From: DJ Delorie <dj@redhat.com>
> Date: Mon, 3 Apr 2023 17:33:03 -0400
> Subject: malloc: set NON_MAIN_ARENA flag for reclaimed memalign chunk (BZ #30101)
> 
> Based on these comments in malloc.c:
> 
>    size field is or'ed with NON_MAIN_ARENA if the chunk was obtained
>    from a non-main arena.  This is only set immediately before handing
>    the chunk to the user, if necessary.
> 
>    The NON_MAIN_ARENA flag is never set for unsorted chunks, so it
>    does not have to be taken into account in size comparisons.
> 
> When we pull a chunk off the unsorted list (or any list) we need to
> make sure that flag is set properly before returning the chunk.
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 0315ac5d16..66e7ca57dd 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -5147,6 +5147,8 @@ _int_memalign (mstate av, size_t alignment, size_t bytes)
>        p = victim;
>        m = chunk2mem (p);
>        set_inuse (p);
> +      if (av != &main_arena)
> +	set_non_main_arena (p);

My preference is to:

(a) Fix both cases where this happens. The other is here:

5199   /* Also give back spare room at the end */
5200   if (!chunk_is_mmapped (p))
5201     {      
5202       size = chunksize (p);
5203       if ((unsigned long) (size) > (unsigned long) (nb + MINSIZE))
5204         {
5205           remainder_size = size - nb;
5206           remainder = chunk_at_offset (p, nb);
5207           set_head (remainder, remainder_size | PREV_INUSE |
5208                     (av != &main_arena ? NON_MAIN_ARENA : 0));
5209           set_head_size (p, nb);
5210           _int_free (av, remainder, 1);
5211         }
5212     }

(b) Remove the comment that says NON_MAIN_ARENA flag is never set,
    and adjust the comment to say it's always set.

I want a *strong* invariant here that the chunks have their flags set
correctly when placed into any of the lists, to do otherwise is incredibly
confusing and is the root cause of the assertion triggering (very good of
you to add it in the first place).

>      }
>    else
>      {
>