Re: [patch v1] malloc: fix set_max_fast "impossibly small" value

[Carlos O'Donell <carlos at redhat dot com>]

On 10/30/19 6:11 PM, DJ Delorie wrote:
> 
> From 83035919da9208a7e6666bdde60e110e2e301553 Mon Sep 17 00:00:00 2001
> From: DJ Delorie <dj@redhat.com>
> Date: Wed, 30 Oct 2019 18:03:14 -0400
> Subject: Base max_fast on alignment, not width, of bins

Does this fix bug 24903? If it does please add "(Bug 24903)" to the
first line of your git commit.

I assume this message is your git format-patch HEAD~1 output.

OK for master with the following fixed:
- Add "(Bug 24903)" to the git commit first line.
- Apply both commit message changes.
 
> set_max_fast set the "impossibly small" value based on,

s/set/sets/g

> eventually, MALLOC_ALIGNMENT.  The comparisons for the smallest
> chunk used, eventuall, MIN_CHUNK_SIZE.  Note that i386

s/, eventuall/is, eventually/g

> is the only platform where these are the same, so a smallest
> chunk *would* be put in a no-fastbins fastbin.
> 
> This change calculates the "impossibly small" value
> based on MIN_CHUNK_SIZE instead, so that we can know it will
> always be impossibly small.
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 5d3e82a8f6..70cc35a473 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1621,7 +1621,7 @@ static INTERNAL_SIZE_T global_max_fast;
>  
>  #define set_max_fast(s) \
>    global_max_fast = (((s) == 0)						      \
> -                     ? SMALLBIN_WIDTH : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK))
> +                     ? MIN_CHUNK_SIZE / 2 : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK))

OK.

My analysis:

SMALLBIN_WIDTH is MALLOC_ALIGNMENT:
1403 #define SMALLBIN_WIDTH    MALLOC_ALIGNMENT

Which on i386 is overriden here:
sysdeps/i386/malloc-alignment.h:
 22 #define MALLOC_ALIGNMENT 16

This is because we have to account for 16-byte aligned vector types.

This means SMALLBIN_WIDTH is 16, which for 32-bit targets is an aligned
and perfectly viable chunk size. 

1178 /* The smallest possible chunk */
1179 #define MIN_CHUNK_SIZE        (offsetof(struct malloc_chunk, fd_nextsize))

This is because MIN_CHUNK_SIZE is:

1048 struct malloc_chunk {
1049 
1050   INTERNAL_SIZE_T      mchunk_prev_size;  /* Size of previous chunk (if free).  */
1051   INTERNAL_SIZE_T      mchunk_size;       /* Size in bytes, including overhead. */
1052 
1053   struct malloc_chunk* fd;         /* double links -- used only if free. */
1054   struct malloc_chunk* bk;
1055 
1056   /* Only used for large blocks: pointer to next larger size.  */
1057   struct malloc_chunk* fd_nextsize; /* double links -- used only if free. */
1058   struct malloc_chunk* bk_nextsize;
1059 };

The offsetof returns 16-bytes.

On x86_64 we have SMALLBIN_WIDTH of 16, but the MIN_CHUNK_SIZE is 32, so it works.

It's the increase in MALLOC_ALIGNMENT to 16 for i686 which breaks this.

Your solution of MIN_CHUNK_SIZE/2 works.

On i686 it yields 8, a size that is too small.
On x86_64 it yields 16, a size that is too small.

Thus it looks like the new calculation in set_max_fast yields an impossibly small
value that we can use that no result of request2size() will return.

>  
>  static inline INTERNAL_SIZE_T
>  get_max_fast (void)
> 


-- 
Cheers,
Carlos.