This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: PATCH: Make chunk size a multiple of MALLOC_ALIGNMENT
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;