PATCH: Make chunk size a multiple of MALLOC_ALIGNMENT

[Carlos O'Donell]

On Thu, May 24, 2012 at 3:30 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> Hi,
>
> do_check_free_chunk has
>
> static void do_check_free_chunk(mstate av, mchunkptr p)
> {
>  INTERNAL_SIZE_T sz = p->size & ~(PREV_INUSE|NON_MAIN_ARENA);
>  mchunkptr next = chunk_at_offset(p, sz);
>
>  do_check_chunk(av, p);
>
>  /* Chunk must claim to be free ... */
>  assert(!inuse(p));
>  assert (!chunk_is_mmapped(p));
>
>  /* Unless a special marker, must have OK fields */
>  if ((unsigned long)(sz) >= MINSIZE)
>  {
>    assert((sz & MALLOC_ALIGN_MASK) == 0);
>
> If a free chunk >= MINSIZE, it must be a multiple of MALLOC_ALIGNMENT.

Agreed.

> However, when sysmalloc frees old top chunk with size >= MINSIZE, it
> doesn't make sure that the size is a multiple of MALLOC_ALIGNMENT:

Agreed.

>     /* Setup fencepost and free the old top chunk. */
>      /* The fencepost takes at least MINSIZE bytes, because it might
>         become the top chunk again later.  Note that a footer is set
>         up, too, although the chunk is marked in use. */
>      old_size -= MINSIZE;
>      set_head(chunk_at_offset(old_top, old_size + 2*SIZE_SZ), 0|PREV_INUSE);
>      if (old_size >= MINSIZE) {
>        set_head(chunk_at_offset(old_top, old_size), (2*SIZE_SZ)|PREV_INUSE);
>        set_foot(chunk_at_offset(old_top, old_size), (2*SIZE_SZ));
>        set_head(old_top, old_size|PREV_INUSE|NON_MAIN_ARENA);
>        _int_free(av, old_top, 1);
>      } else {
>
> This bug caused some test failures in one of nss packages on Linux/x32.
> This patch fixes it.  OK to install?

Why doesn't this trigger for any other architectures?

> Thanks.
>
>
> H.J.
> ---
>        [BZ #13576]
>        * malloc/malloc.c (sYSMALLOc): Free the old top chunk with a
>        multiple of MALLOC_ALIGNMENT in size.
>        (_int_free): Check chunk size is a multiple of MALLOC_ALIGNMENT.
>
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index bb26937..46826ab 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2390,11 +2390,12 @@ static void* sysmalloc(INTERNAL_SIZE_T nb, mstate av)
>       top(av) = chunk_at_offset(heap, sizeof(*heap));
>       set_head(top(av), (heap->size - sizeof(*heap)) | PREV_INUSE);
>
> -      /* Setup fencepost and free the old top chunk. */
> +      /* Setup fencepost and free the old top chunk with a multiple of
> +        MALLOC_ALIGNMENT in size. */

OK.

>       /* The fencepost takes at least MINSIZE bytes, because it might
>         become the top chunk again later.  Note that a footer is set
>         up, too, although the chunk is marked in use. */
> -      old_size -= MINSIZE;
> +      old_size = (old_size - MINSIZE) & ~MALLOC_ALIGN_MASK;

OK.

>       set_head(chunk_at_offset(old_top, old_size + 2*SIZE_SZ), 0|PREV_INUSE);
>       if (old_size >= MINSIZE) {
>        set_head(chunk_at_offset(old_top, old_size), (2*SIZE_SZ)|PREV_INUSE);
> @@ -3803,8 +3804,10 @@ _int_free(mstate av, mchunkptr p, int have_lock)
>       malloc_printerr (check_action, errstr, chunk2mem(p));
>       return;
>     }
> -  /* We know that each chunk is at least MINSIZE bytes in size.  */
> -  if (__builtin_expect (size < MINSIZE, 0))
> +  /* We know that each chunk is at least MINSIZE bytes in size of a
> +     multiple of MALLOC_ALIGNMENT.  */

Should e "or a" not "of a"?

> +  if (__builtin_expect (size < MINSIZE
> +                       || (size & MALLOC_ALIGN_MASK) != 0, 0))

Should this use aligned_OK?

The aligned_OK macro should be used in a lot more places :-(

>     {
>       errstr = "free(): invalid size";
>       goto errout;
> --
> 1.7.6.5
>

Cheers,
Carlos.