Re: PATCH: Make chunk size a multiple of MALLOC_ALIGNMENT

["H.J. Lu" <hjl dot tools at gmail dot com>]

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;