Bug 14562

Summary: threaded programs with x32 abi randomly crash with arena.c:661: heap_trim: Assertion `p->size == (0|0x1)' failed
Product: glibc Reporter: Mike Frysinger <vapier>
Component: mallocAssignee: H.J. Lu <hjl.tools>
Status: RESOLVED FIXED    
Severity: normal CC: drepper.fsp, dschepler, fweimer, hjl.tools
Priority: P2 Flags: fweimer: security-
Version: 2.16   
Target Milestone: 2.16   
URL: https://bugs.gentoo.org/show_bug.cgi?id=394175
Host: Target:
Build: Last reconfirmed:

Description Mike Frysinger 2012-09-07 23:49:32 UTC 
test case:
- use glibc-2.16 with x32 abi
- get a bunch of files (like ~150; perl man pages are a sample input)
- run `bzip2 -k <files>`
- run `pbzip2 -k -f <files> -p4`

see crash:
pbzip2: arena.c:661: heap_trim: Assertion `p->size == (0|0x1)' failed.

this has happened with a other threaded programs (git & squashfs); see gentoo bug for some more details
Comment 1 H.J. Lu 2012-09-08 00:42:46 UTC 
hjl/x32/release/2.15 branch is OK.
Comment 2 H.J. Lu 2012-09-08 01:03:14 UTC 
Copy malloc from hjl/x32/release/2.15 branch makes the
problem goes away.
Comment 3 H.J. Lu 2012-09-08 04:03:06 UTC 
The bug is triggered by MALLOC_ALIGNMENT=16.
Comment 4 H.J. Lu 2012-09-08 14:52:34 UTC 
663	    assert(p->size == (0|PREV_INUSE)); /* must be fencepost */
(gdb) p p
$34 = (mchunkptr) 0xf55feff8
(gdb) 

p isn't 16-byte aligned.
Comment 5 H.J. Lu 2012-09-08 15:30:13 UTC 
The size of top chunk must be a multiple of MALLOC_ALIGNMENT.
But _int_new_arena has

  /* Set up the top chunk, with proper alignment. */
  ptr = (char *)(a + 1);
  misalign = (unsigned long)chunk2mem(ptr) & MALLOC_ALIGN_MASK;
  if (misalign > 0)
    ptr += MALLOC_ALIGNMENT - misalign;
  top(a) = (mchunkptr)ptr;
  set_head(top(a), (((char*)h + h->size) - ptr) | PREV_INUSE);

It doesn't check size requirement.
Comment 6 H.J. Lu 2012-09-08 20:43:47 UTC 
I am testing this patch:

diff --git a/malloc/arena.c b/malloc/arena.c
index f88b41d..ac5afc4 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -655,14 +655,22 @@ heap_trim(heap_info *heap, size_t pad)
   mchunkptr top_chunk = top(ar_ptr), p, bck, fwd;
   heap_info *prev_heap;
   long new_size, top_size, extra;
+  unsigned long misalign;
 
   /* Can this heap go away completely? */
   while(top_chunk == chunk_at_offset(heap, sizeof(*heap))) {
     prev_heap = heap->prev;
     p = chunk_at_offset(prev_heap, prev_heap->size - (MINSIZE-2*SIZE_SZ));
+    /* fencepost must be properly aligned.  */
+    misalign = ((unsigned long) p) & MALLOC_ALIGN_MASK;
+    if (misalign > 0)
+      {
+	p = (mchunkptr)(((unsigned long) p) & ~MALLOC_ALIGN_MASK);
+	misalign = MALLOC_ALIGNMENT - misalign;
+      }
     assert(p->size == (0|PREV_INUSE)); /* must be fencepost */
     p = prev_chunk(p);
-    new_size = chunksize(p) + (MINSIZE-2*SIZE_SZ);
+    new_size = chunksize(p) + (MINSIZE-2*SIZE_SZ) + misalign;
     assert(new_size>0 && new_size<(long)(2*MINSIZE));
     if(!prev_inuse(p))
       new_size += p->prev_size;
Comment 7 H.J. Lu 2012-09-08 20:48:37 UTC 
This is the right one:

diff --git a/malloc/arena.c b/malloc/arena.c
index f88b41d..8d52109 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -655,14 +655,18 @@ heap_trim(heap_info *heap, size_t pad)
   mchunkptr top_chunk = top(ar_ptr), p, bck, fwd;
   heap_info *prev_heap;
   long new_size, top_size, extra;
+  unsigned long misalign;
 
   /* Can this heap go away completely? */
   while(top_chunk == chunk_at_offset(heap, sizeof(*heap))) {
     prev_heap = heap->prev;
     p = chunk_at_offset(prev_heap, prev_heap->size - (MINSIZE-2*SIZE_SZ));
+    /* fencepost must be properly aligned.  */
+    misalign = ((unsigned long) p) & MALLOC_ALIGN_MASK;
+    p = (mchunkptr)(((unsigned long) p) & ~MALLOC_ALIGN_MASK);
     assert(p->size == (0|PREV_INUSE)); /* must be fencepost */
     p = prev_chunk(p);
-    new_size = chunksize(p) + (MINSIZE-2*SIZE_SZ);
+    new_size = chunksize(p) + (MINSIZE-2*SIZE_SZ) + misalign;
     assert(new_size>0 && new_size<(long)(2*MINSIZE));
     if(!prev_inuse(p))
       new_size += p->prev_size;
Comment 8 H.J. Lu 2012-09-08 20:58:39 UTC 
This one is better:

diff --git a/malloc/arena.c b/malloc/arena.c
index f88b41d..4727d1e 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -654,15 +654,18 @@ heap_trim(heap_info *heap, size_t pad)
   unsigned long pagesz = GLRO(dl_pagesize);
   mchunkptr top_chunk = top(ar_ptr), p, bck, fwd;
   heap_info *prev_heap;
-  long new_size, top_size, extra;
+  long new_size, top_size, extra, misalign;
 
   /* Can this heap go away completely? */
   while(top_chunk == chunk_at_offset(heap, sizeof(*heap))) {
     prev_heap = heap->prev;
     p = chunk_at_offset(prev_heap, prev_heap->size - (MINSIZE-2*SIZE_SZ));
+    /* fencepost must be properly aligned.  */
+    misalign = ((long) p) & MALLOC_ALIGN_MASK;
+    p = (mchunkptr)(((unsigned long) p) & ~MALLOC_ALIGN_MASK);
     assert(p->size == (0|PREV_INUSE)); /* must be fencepost */
     p = prev_chunk(p);
-    new_size = chunksize(p) + (MINSIZE-2*SIZE_SZ);
+    new_size = chunksize(p) + (MINSIZE-2*SIZE_SZ) + misalign;
     assert(new_size>0 && new_size<(long)(2*MINSIZE));
     if(!prev_inuse(p))
       new_size += p->prev_size;
Comment 9 H.J. Lu 2012-09-09 18:29:59 UTC 
Please try hjl/pr14562/2.16 branch.
Comment 10 Mike Frysinger 2012-09-10 05:42:25 UTC 
(In reply to comment #9)

seems to be the same fix as in comment #8 which i've been running locally.  it fixed the known bugs i was hitting, and haven't seen any new ones related to this.  there are other bugs, but those are porting issues :).
Comment 11 Daniel Schepler 2012-09-10 17:59:14 UTC 
(In reply to comment #9)
> Please try hjl/pr14562/2.16 branch.

As I already commented on the x32 list, for me this fixes among other things: gij running ecj, and the GraphicsMagick, perl and pixman testsuites.