Re: PATCH: Make chunk size a multiple of MALLOC_ALIGNMENT

["Carlos O'Donell" <carlos at systemhalted dot org>]

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.