RFC: malloc bug with custom non-contiguous morecore handler

[Adam Nemet <anemet at caviumnetworks dot com>]

[I originally sent this to libc-alpha but it was rejected by the moderator and
I was asked to resubmit here.]

I found this in an older glibc release but by looking at malloc.c from trunk
the bug probably still exists.

In sysmalloc, in the a non-contiguous case when top is relocated, old_top gets
freed.  This is done via an _int_free call *before* the allocation was
actually performed and the new top was adjusted.  If _int_free triggers a trim
then we free the space we just allocated in top.  Then the actual allocation
fails because there is not enough space in top.

The patch below fixes the issue.  Would this be interesting for mainline?
Assuming this hasn't been covered yet, I can try to come up with a testcase
for trunk glibc and submit an official patch.

Adam

	* include/malloc.h: Add trim to declaration of _int_free.
	* malloc/malloc.c: Adjust _int_free declaration.
	(sYSMALLOc, public_fREe, _int_realloc, _int_memalign): Adjust call to
	_init_free.  Call it non-trimming when freeing old-top in sYSMALLOc.
	(_int_free): Use new argument trim to decide whether to trim.
	* malloc/arena.c (free_atfork): Adjust call to _int_free.
	* malloc/hooks.c (free_check, free_starter): Likewise.

Index: glibc/include/malloc.h
===================================================================
--- glibc.orig/include/malloc.h	2008-07-17 12:30:52.000000000 -0700
+++ glibc/include/malloc.h	2008-07-17 12:31:07.000000000 -0700
@@ -13,7 +13,7 @@ struct malloc_state;
 typedef struct malloc_state *mstate;
 
 extern __malloc_ptr_t _int_malloc (mstate __m, size_t __size) attribute_hidden;
-extern void           _int_free (mstate __m, __malloc_ptr_t __ptr)
+extern void           _int_free (mstate __m, __malloc_ptr_t __ptr, int __trim)
      attribute_hidden;
 extern __malloc_ptr_t _int_realloc (mstate __m,
 				    __malloc_ptr_t __ptr,
Index: glibc/malloc/arena.c
===================================================================
--- glibc.orig/malloc/arena.c	2008-07-17 12:30:53.000000000 -0700
+++ glibc/malloc/arena.c	2008-07-17 12:31:20.000000000 -0700
@@ -212,7 +212,7 @@ free_atfork(Void_t* mem, const Void_t *c
   tsd_getspecific(arena_key, vptr);
   if(vptr != ATFORK_ARENA_PTR)
     (void)mutex_lock(&ar_ptr->mutex);
-  _int_free(ar_ptr, mem);
+  _int_free(ar_ptr, mem, 1);
   if(vptr != ATFORK_ARENA_PTR)
     (void)mutex_unlock(&ar_ptr->mutex);
 }
Index: glibc/malloc/hooks.c
===================================================================
--- glibc.orig/malloc/hooks.c	2008-07-17 12:30:52.000000000 -0700
+++ glibc/malloc/hooks.c	2008-07-17 12:31:20.000000000 -0700
@@ -295,7 +295,7 @@ free_check(mem, caller) Void_t* mem; con
 #if 0 /* Erase freed memory. */
   memset(mem, 0, chunksize(p) - (SIZE_SZ+1));
 #endif
-  _int_free(&main_arena, mem);
+  _int_free(&main_arena, mem, 1);
   (void)mutex_unlock(&main_arena.mutex);
 }
 
@@ -472,7 +472,7 @@ free_starter(mem, caller) Void_t* mem; c
     return;
   }
 #endif
-  _int_free(&main_arena, mem);
+  _int_free(&main_arena, mem, 1);
 }
 
 # endif	/* !defiend NO_STARTER */
Index: glibc/malloc/malloc.c
===================================================================
--- glibc.orig/malloc/malloc.c	2008-07-17 12:30:52.000000000 -0700
+++ glibc/malloc/malloc.c	2008-07-17 12:31:20.000000000 -0700
@@ -1513,7 +1513,7 @@ typedef struct malloc_chunk* mchunkptr;
 #if __STD_C
 
 Void_t*         _int_malloc(mstate, size_t);
-void            _int_free(mstate, Void_t*);
+void            _int_free(mstate, Void_t*, int);
 Void_t*         _int_realloc(mstate, Void_t*, size_t);
 Void_t*         _int_memalign(mstate, size_t, size_t);
 Void_t*         _int_valloc(mstate, size_t);
@@ -2922,7 +2922,7 @@ static Void_t* sYSMALLOc(nb, av) INTERNA
 	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, chunk2mem(old_top));
+	_int_free(av, chunk2mem(old_top), 1);
       } else {
 	set_head(old_top, (old_size + 2*SIZE_SZ)|PREV_INUSE);
 	set_foot(old_top, (old_size + 2*SIZE_SZ));
@@ -3156,9 +3156,10 @@ static Void_t* sYSMALLOc(nb, av) INTERNA
           chunk_at_offset(old_top, old_size + 2*SIZE_SZ)->size =
             (2*SIZE_SZ)|PREV_INUSE;
 
-          /* If possible, release the rest. */
+          /* If possible, release the rest.  We haven't done
+	     allocation yet so top should not be trimmed.  */
           if (old_size >= MINSIZE) {
-            _int_free(av, chunk2mem(old_top));
+            _int_free(av, chunk2mem(old_top), 0);
           }
 
         }
@@ -3444,7 +3445,7 @@ public_fREe(Void_t* mem)
 #else
   (void)mutex_lock(&ar_ptr->mutex);
 #endif
-  _int_free(ar_ptr, mem);
+  _int_free(ar_ptr, mem, 1);
   (void)mutex_unlock(&ar_ptr->mutex);
 }
 #ifdef libc_hidden_def
@@ -4280,7 +4281,7 @@ _int_malloc(mstate av, size_t bytes)
 */
 
 void
-_int_free(mstate av, Void_t* mem)
+_int_free(mstate av, Void_t* mem, int trim)
 {
   mchunkptr       p;           /* chunk corresponding to mem */
   INTERNAL_SIZE_T size;        /* its size */
@@ -4468,8 +4469,9 @@ _int_free(mstate av, Void_t* mem)
 
       if (av == &main_arena) {
 #ifndef MORECORE_CANNOT_TRIM
-	if ((unsigned long)(chunksize(av->top)) >=
-	    (unsigned long)(mp_.trim_threshold))
+	if (trim
+	    && ((unsigned long)(chunksize(av->top)) >=
+		(unsigned long)(mp_.trim_threshold)))
 	  sYSTRIm(mp_.top_pad, av);
 #endif
       } else {
@@ -4752,7 +4754,7 @@ _int_realloc(mstate av, Void_t* oldmem, 
             }
           }
 
-          _int_free(av, oldmem);
+          _int_free(av, oldmem, 1);
           check_inuse_chunk(av, newp);
           return chunk2mem(newp);
         }
@@ -4776,7 +4778,7 @@ _int_realloc(mstate av, Void_t* oldmem, 
 	       (av != &main_arena ? NON_MAIN_ARENA : 0));
       /* Mark remainder as inuse so free() won't complain */
       set_inuse_bit_at_offset(remainder, remainder_size);
-      _int_free(av, chunk2mem(remainder));
+      _int_free(av, chunk2mem(remainder), 1);
     }
 
     check_inuse_chunk(av, newp);
@@ -4835,7 +4837,7 @@ _int_realloc(mstate av, Void_t* oldmem, 
       newmem = _int_malloc(av, nb - MALLOC_ALIGN_MASK);
       if (newmem != 0) {
         MALLOC_COPY(newmem, oldmem, oldsize - 2*SIZE_SZ);
-        _int_free(av, oldmem);
+        _int_free(av, oldmem, 1);
       }
     }
     return newmem;
@@ -4929,7 +4931,7 @@ _int_memalign(mstate av, size_t alignmen
 	     (av != &main_arena ? NON_MAIN_ARENA : 0));
     set_inuse_bit_at_offset(newp, newsize);
     set_head_size(p, leadsize | (av != &main_arena ? NON_MAIN_ARENA : 0));
-    _int_free(av, chunk2mem(p));
+    _int_free(av, chunk2mem(p), 1);
     p = newp;
 
     assert (newsize >= nb &&
@@ -4945,7 +4947,7 @@ _int_memalign(mstate av, size_t alignmen
       set_head(remainder, remainder_size | PREV_INUSE |
 	       (av != &main_arena ? NON_MAIN_ARENA : 0));
       set_head_size(p, nb);
-      _int_free(av, chunk2mem(remainder));
+      _int_free(av, chunk2mem(remainder), 1);
     }
   }