PATCH: Make chunk size a multiple of MALLOC_ALIGNMENT

[H.J. Lu]

On Thu, May 24, 2012 at 4:36 PM, Carlos O'Donell
<carlos@systemhalted.org> wrote:
> 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?

It is very rare, even with MALLOC_ALIGNMENT. >  (2*SIZE_SZ).
I only saw it once on Linux/x32.

>>       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"?

Fixed.

>> +  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 :-(
>

Fixed.  Here is the updated patch.  OK to install?

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 447b622..28039b4 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -2396,11 +2396,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. */
       /* 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;
       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);
@@ -3809,8 +3810,9 @@ _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 or a
+     multiple of MALLOC_ALIGNMENT.  */
+  if (__builtin_expect (size < MINSIZE || !aligned_OK (size), 0))
     {
       errstr = "free(): invalid size";
       goto errout;