Improvements in freelist handling for Memcheck. See #250065.

(Philippe Waroquiers, philippe.waroquiers@skynet.be)

This patch provides three improvements in the way the free list is 
handled in memcheck.

First improvement: a new command line option --freelist-big-blocks
(default 1000000) specifies the size of "free list big blocks". 
Such big blocks will be put on the free list, but will be re-cycled first
(i.e. in preference to block having a smaller size).
This fixes the bug https://bugs.kde.org/show_bug.cgi?id=250065.
Technically, the freed list is divided in two lists : small
and big blocks. Blocks are first released from the big block list.

Second improvement: the blocks of the freed list are re-cycled before
a new block is malloc-ed, not after a block is freed.
This gives better error messages for dangling pointer errors
when doing many frees without doing malloc between the frees.
(this does not uses more memory).

Third improvement: a block bigger than the free list volume will be
put in the free list (till a malloc is done, so as the needed memory
is not bigger than before) but will be put at the beginning of the
free list, rather than at the end. So, allocating then freeing such a
block does not cause any blocks in the free list to be released.

Results of the improvements above, with the new regression test
memcheck/test/big_blocks_freed_list: with the patch, 7 errors
are detected, 6 are giving the (correct) allocation stack.
Without the patch, only 6 errors are detected, 5 errors without
allocation stack, 1 with a (wrong) allocation stack.



git-svn-id: svn://svn.valgrind.org/valgrind/trunk@12202

memcheck/docs/mc-manual.xml

@@ -790,6 +790,24 @@ criteria:</para>
     </listitem>
   </varlistentry>
 
+  <varlistentry id="opt.freelist-big-blocks" xreflabel="--freelist-big-blocks">
+    <term>
+      <option><![CDATA[--freelist-big-blocks=<number> [default: 1000000] ]]></option>
+    </term>
+    <listitem>
+      <para>When making blocks from the queue of freed blocks available
+      for re-allocation, Memcheck will in priority re-circulate the blocks
+      with a size greater or equal to <option>--freelist-big-blocks</option>.
+      This ensures that freeing big blocks (in particular freeing blocks bigger than
+      <option>--freelist-vol</option>) does not immediately lead to a re-circulation
+      of all (or a lot of) the small blocks in the free list. In other words,
+      this option increases the likelihood to discover dangling pointers
+      for the "small" blocks, even when big blocks are freed.</para>
+      <para>Setting a value of 0 means that all the blocks are re-circulated
+      in a FIFO order. </para>
+    </listitem>
+  </varlistentry>
+
   <varlistentry id="opt.workaround-gcc296-bugs" xreflabel="--workaround-gcc296-bugs">
     <term>
       <option><![CDATA[--workaround-gcc296-bugs=<yes|no> [default: no] ]]></option>

memcheck/mc_errors.c

@@ -1103,18 +1103,15 @@ static void describe_addr ( Addr a, /*OUT*/AddrInfo* ai )
       return;
    }
    /* -- Search for a recently freed block which might bracket it. -- */
-   mc = MC_(get_freed_list_head)();
-   while (mc) {
-      if (addr_is_in_MC_Chunk_default_REDZONE_SZB(mc, a)) {
-         ai->tag = Addr_Block;
-         ai->Addr.Block.block_kind = Block_Freed;
-         ai->Addr.Block.block_desc = "block";
-         ai->Addr.Block.block_szB  = mc->szB;
-         ai->Addr.Block.rwoffset   = (Word)a - (Word)mc->data;
-         ai->Addr.Block.lastchange = mc->where;
-         return;
-      }
-      mc = mc->next; 
+   mc = MC_(get_freed_block_bracketting)( a );
+   if (mc) {
+      ai->tag = Addr_Block;
+      ai->Addr.Block.block_kind = Block_Freed;
+      ai->Addr.Block.block_desc = "block";
+      ai->Addr.Block.block_szB  = mc->szB;
+      ai->Addr.Block.rwoffset   = (Word)a - (Word)mc->data;
+      ai->Addr.Block.lastchange = mc->where;
+      return;
    }
    /* -- Search for a currently malloc'd block which might bracket it. -- */
    VG_(HT_ResetIter)(MC_(malloc_list));

memcheck/mc_include.h

@@ -96,7 +96,10 @@ void MC_(move_mempool)    ( Addr poolA, Addr poolB );
 void MC_(mempool_change)  ( Addr pool, Addr addrA, Addr addrB, SizeT size );
 Bool MC_(mempool_exists)  ( Addr pool );
 
-MC_Chunk* MC_(get_freed_list_head)( void );
+/* Searches for a recently freed block which might bracket Addr a.
+   Return the MC_Chunk* for this block or NULL if no bracketting block
+   is found. */
+MC_Chunk* MC_(get_freed_block_bracketting)( Addr a );
 
 /* For tracking malloc'd blocks.  Nb: it's quite important that it's a
    VgHashTable, because VgHashTable allows duplicate keys without complaint.
@@ -424,6 +427,10 @@ extern Bool MC_(clo_partial_loads_ok);
 /* Max volume of the freed blocks queue. */
 extern Long MC_(clo_freelist_vol);
 
+/* Blocks with a size >= MC_(clo_freelist_big_blocks) will be put
+   in the "big block" freed blocks queue. */
+extern Long MC_(clo_freelist_big_blocks);
+
 /* Do leak check at exit?  default: NO */
 extern LeakCheckMode MC_(clo_leak_check);
 

memcheck/mc_main.c

@@ -4701,6 +4701,7 @@ static Bool mc_expensive_sanity_check ( void )
 
 Bool          MC_(clo_partial_loads_ok)       = False;
 Long          MC_(clo_freelist_vol)           = 20*1000*1000LL;
+Long          MC_(clo_freelist_big_blocks)    =  1*1000*1000LL;
 LeakCheckMode MC_(clo_leak_check)             = LC_Summary;
 VgRes         MC_(clo_leak_resolution)        = Vg_HighRes;
 Bool          MC_(clo_show_reachable)         = False;
@@ -4761,7 +4762,11 @@ static Bool mc_process_cmd_line_options(Char* arg)
 
    else if VG_BINT_CLO(arg, "--freelist-vol",  MC_(clo_freelist_vol), 
                                                0, 10*1000*1000*1000LL) {}
-   
+
+   else if VG_BINT_CLO(arg, "--freelist-big-blocks",
+                       MC_(clo_freelist_big_blocks),
+                       0, 10*1000*1000*1000LL) {}
+
    else if VG_XACT_CLO(arg, "--leak-check=no",
                             MC_(clo_leak_check), LC_Off) {}
    else if VG_XACT_CLO(arg, "--leak-check=summary",
@@ -4831,7 +4836,8 @@ static void mc_print_usage(void)
 "    --undef-value-errors=no|yes      check for undefined value errors [yes]\n"
 "    --track-origins=no|yes           show origins of undefined values? [no]\n"
 "    --partial-loads-ok=no|yes        too hard to explain here; see manual [no]\n"
-"    --freelist-vol=<number>          volume of freed blocks queue [20000000]\n"
+"    --freelist-vol=<number>          volume of freed blocks queue      [20000000]\n"
+"    --freelist-big-blocks=<number>   releases first blocks with size >= [1000000]\n"
 "    --workaround-gcc296-bugs=no|yes  self explanatory [no]\n"
 "    --ignore-ranges=0xPP-0xQQ[,0xRR-0xSS]   assume given addresses are OK\n"
 "    --malloc-fill=<hexnumber>        fill malloc'd areas with given value\n"
@@ -5839,6 +5845,13 @@ static void mc_post_clo_init ( void )
       MC_(clo_leak_check) = LC_Full;
    }
 
+   if (MC_(clo_freelist_big_blocks) >= MC_(clo_freelist_vol))
+      VG_(message)(Vg_UserMsg,
+                   "Warning: --freelist-big-blocks value %lld has no effect\n"
+                   "as it is >= to --freelist-vol value %lld\n",
+                   MC_(clo_freelist_big_blocks),
+                   MC_(clo_freelist_vol));
+
    tl_assert( MC_(clo_mc_level) >= 1 && MC_(clo_mc_level) <= 3 );
 
    if (MC_(clo_mc_level) == 3) {

memcheck/mc_malloc_wrappers.c

@@ -70,69 +70,111 @@ VgHashTable MC_(malloc_list) = NULL;
 VgHashTable MC_(mempool_list) = NULL;
    
 /* Records blocks after freeing. */
-static MC_Chunk* freed_list_start  = NULL;
-static MC_Chunk* freed_list_end    = NULL;
+/* Blocks freed by the client are queued in one of two lists of
+   freed blocks not yet physically freed:
+   "big blocks" freed list.
+   "small blocks" freed list
+   The blocks with a size >= MC_(clo_freelist_big_blocks)
+   are linked in the big blocks freed list.
+   This allows a client to allocate and free big blocks
+   (e.g. bigger than VG_(clo_freelist_vol)) without losing
+   immediately all protection against dangling pointers.
+   position [0] is for big blocks, [1] is for small blocks. */
+static MC_Chunk* freed_list_start[2]  = {NULL, NULL};
+static MC_Chunk* freed_list_end[2]    = {NULL, NULL};
 
 /* Put a shadow chunk on the freed blocks queue, possibly freeing up
    some of the oldest blocks in the queue at the same time. */
 static void add_to_freed_queue ( MC_Chunk* mc )
 {
    const Bool show = False;
+   const int l = (mc->szB >= MC_(clo_freelist_big_blocks) ? 0 : 1);
 
-   /* Put it at the end of the freed list */
-   if (freed_list_end == NULL) {
-      tl_assert(freed_list_start == NULL);
-      freed_list_end    = freed_list_start = mc;
-      VG_(free_queue_volume) = (Long)mc->szB;
+   /* Put it at the end of the freed list, unless the block
+      would be directly released any way : in this case, we
+      put it at the head of the freed list. */
+   if (freed_list_end[l] == NULL) {
+      tl_assert(freed_list_start[l] == NULL);
+      mc->next = NULL;
+      freed_list_end[l]    = freed_list_start[l] = mc;
    } else {
-      tl_assert(freed_list_end->next == NULL);
-      freed_list_end->next = mc;
-      freed_list_end       = mc;
-      VG_(free_queue_volume) += (Long)mc->szB;
-      if (show)
-         VG_(printf)("mc_freelist: acquire: volume now %lld\n", 
-                     VG_(free_queue_volume));
-   }
-   VG_(free_queue_length)++;
-   mc->next = NULL;
-
-   /* Release enough of the oldest blocks to bring the free queue
-      volume below vg_clo_freelist_vol. */
-
-   while (VG_(free_queue_volume) > MC_(clo_freelist_vol)) {
-      MC_Chunk* mc1;
-
-      tl_assert(freed_list_start != NULL);
-      tl_assert(freed_list_end != NULL);
-
-      mc1 = freed_list_start;
-      VG_(free_queue_volume) -= (Long)mc1->szB;
-      VG_(free_queue_length)--;
-      if (show)
-         VG_(printf)("mc_freelist: discard: volume now %lld\n", 
-                     VG_(free_queue_volume));
-      tl_assert(VG_(free_queue_volume) >= 0);
-
-      if (freed_list_start == freed_list_end) {
-         freed_list_start = freed_list_end = NULL;
+      tl_assert(freed_list_end[l]->next == NULL);
+      if (mc->szB >= MC_(clo_freelist_vol)) {
+         mc->next = freed_list_start[l];
+         freed_list_start[l] = mc;
       } else {
-         freed_list_start = mc1->next;
+         mc->next = NULL;
+         freed_list_end[l]->next = mc;
+         freed_list_end[l]       = mc;
       }
-      mc1->next = NULL; /* just paranoia */
+   }
+   VG_(free_queue_volume) += (Long)mc->szB;
+   if (show)
+      VG_(printf)("mc_freelist: acquire: volume now %lld\n", 
+                  VG_(free_queue_volume));
+   VG_(free_queue_length)++;
+}
 
-      /* free MC_Chunk */
-      if (MC_AllocCustom != mc1->allockind)
-         VG_(cli_free) ( (void*)(mc1->data) );
-      VG_(free) ( mc1 );
+/* Release enough of the oldest blocks to bring the free queue
+   volume below vg_clo_freelist_vol. 
+   Start with big block list first.
+   On entry, VG_(free_queue_volume) must be > MC_(clo_freelist_vol).
+   On exit, VG_(free_queue_volume) will be <= MC_(clo_freelist_vol). */
+static void release_oldest_block(void)
+{
+   const Bool show = False;
+   int i;
+   tl_assert (VG_(free_queue_volume) > MC_(clo_freelist_vol));
+   tl_assert (freed_list_start[0] != NULL || freed_list_start[1] != NULL);
+
+   for (i = 0; i < 2; i++) {
+      while (VG_(free_queue_volume) > MC_(clo_freelist_vol)
+             && freed_list_start[i] != NULL) {
+         MC_Chunk* mc1;
+
+         tl_assert(freed_list_end[i] != NULL);
+         
+         mc1 = freed_list_start[i];
+         VG_(free_queue_volume) -= (Long)mc1->szB;
+         VG_(free_queue_length)--;
+         if (show)
+            VG_(printf)("mc_freelist: discard: volume now %lld\n", 
+                        VG_(free_queue_volume));
+         tl_assert(VG_(free_queue_volume) >= 0);
+         
+         if (freed_list_start[i] == freed_list_end[i]) {
+            freed_list_start[i] = freed_list_end[i] = NULL;
+         } else {
+            freed_list_start[i] = mc1->next;
+         }
+         mc1->next = NULL; /* just paranoia */
+
+         /* free MC_Chunk */
+         if (MC_AllocCustom != mc1->allockind)
+            VG_(cli_free) ( (void*)(mc1->data) );
+         VG_(free) ( mc1 );
+      }
    }
 }
 
-MC_Chunk* MC_(get_freed_list_head)(void)
+MC_Chunk* MC_(get_freed_block_bracketting) (Addr a)
 {
-   return freed_list_start;
+   int i;
+   for (i = 0; i < 2; i++) {
+      MC_Chunk*  mc;
+      mc = freed_list_start[i];
+      while (mc) {
+         if (VG_(addr_is_in_block)( a, mc->data, mc->szB,
+                                    MC_MALLOC_REDZONE_SZB ))
+            return mc;
+         mc = mc->next;
+      }
+   }
+   return NULL;
 }
 
-/* Allocate its shadow chunk, put it on the appropriate list. */
+/* Allocate a shadow chunk, put it on the appropriate list.
+   If needed, release oldest blocks from freed list. */
 static
 MC_Chunk* create_MC_Chunk ( ExeContext* ec, Addr p, SizeT szB,
                             MC_AllocKind kind)
@@ -143,6 +185,11 @@ MC_Chunk* create_MC_Chunk ( ExeContext* ec, Addr p, SizeT szB,
    mc->allockind = kind;
    mc->where     = ec;
 
+   /* Each time a new MC_Chunk is created, release oldest blocks
+      if the free list volume is exceeded. */
+   if (VG_(free_queue_volume) > MC_(clo_freelist_vol))
+      release_oldest_block();
+
    /* Paranoia ... ensure the MC_Chunk is off-limits to the client, so
       the mc->data field isn't visible to the leak checker.  If memory
       management is working correctly, any pointer returned by VG_(malloc)
@@ -296,6 +343,10 @@ void die_and_free_mem ( ThreadId tid, MC_Chunk* mc, SizeT rzB )
    mc->where = VG_(record_ExeContext) ( tid, 0/*first_ip_delta*/ );
    /* Put it out of harm's way for a while */
    add_to_freed_queue ( mc );
+   /* If the free list volume is bigger than MC_(clo_freelist_vol),
+      we wait till the next block allocation to release blocks.
+      This increase the chance to discover dangling pointer usage,
+      even for big blocks being freed by the client. */
 }
 
 void MC_(handle_free) ( ThreadId tid, Addr p, UInt rzB, MC_AllocKind kind )

memcheck/tests/Makefile.am

@@ -62,6 +62,7 @@ EXTRA_DIST = \
 	badloop.stderr.exp badloop.vgtest \
 	badpoll.stderr.exp badpoll.vgtest \
 	badrw.stderr.exp badrw.vgtest badrw.stderr.exp-s390x-mvc \
+	big_blocks_freed_list.stderr.exp big_blocks_freed_list.vgtest \
 	brk2.stderr.exp brk2.vgtest \
 	buflen_check.stderr.exp buflen_check.vgtest buflen_check.stderr.exp-kfail \
 	calloc-overflow.stderr.exp calloc-overflow.vgtest\
@@ -214,6 +215,7 @@ check_PROGRAMS = \
 	badloop \
 	badpoll \
 	badrw \
+	big_blocks_freed_list \
 	brk2 \
 	buflen_check \
 	calloc-overflow \

memcheck/tests/big_blocks_freed_list.c

@@ -0,0 +1,57 @@
+#include <stdlib.h>
+/* To be run with --freelist-vol=1000000 --freelist-big-blocks=50000 */
+static void jumped(void)
+{
+   ;
+}
+int main(int argc, char *argv[])
+{
+   char *semi_big = NULL;
+   char *big = NULL;
+   char *small = NULL;
+   char *other_small = NULL;
+   int i;
+   int j;
+
+   /* Verify that access via a dangling pointer to a big block bigger than
+      the free list is found by memcheck (still on the free list). */
+   semi_big = malloc (900000);
+   big = malloc (1000001);
+   free(semi_big);
+   free(big);
+   if (big[1000] > 0x0) jumped();
+   if (semi_big[1000] > 0x0) jumped();
+
+   /* Then verify that dangling pointers for small blocks is not hampered
+      by doing big alloc/free. */
+   small = malloc (10000);
+   free(small);
+
+   /* We should still have a nice error msg for the semi_big
+      but not for the big block, which has been removed from the free list
+      with the malloc of small above. */
+   if (big[2000] > 0x0) jumped();
+   if (semi_big[2000] > 0x0) jumped();
+
+   big = NULL;
+
+   {
+      big = malloc (1000001);
+      free(big);
+      if (small[10] > 0x0) jumped();
+      
+      /* Do not common up the below in a loop. We
+         want a different error/stack trace for each of
+         these. */
+      if (big[10] > 0x0) jumped();
+   }
+   
+   
+   for (i = 0; i < 100; i++) {
+      other_small = malloc(10000);
+      for (j = 0; j < 10000; j++)
+         other_small[j] = 0x1;
+   }
+   if (small[10] > 0x0) jumped();
+   return 0;
+}

memcheck/tests/big_blocks_freed_list.stderr.exp

@@ -0,0 +1,50 @@
+
+Invalid read of size 1
+   at 0x........: main (big_blocks_freed_list.c:22)
+ Address 0x........ is 1,000 bytes inside a block of size 1,000,001 free'd
+   at 0x........: free (vg_replace_malloc.c:...)
+   by 0x........: main (big_blocks_freed_list.c:21)
+
+Invalid read of size 1
+   at 0x........: main (big_blocks_freed_list.c:23)
+ Address 0x........ is 1,000 bytes inside a block of size 900,000 free'd
+   at 0x........: free (vg_replace_malloc.c:...)
+   by 0x........: main (big_blocks_freed_list.c:20)
+
+Invalid read of size 1
+   at 0x........: main (big_blocks_freed_list.c:33)
+ Address 0x........ is not stack'd, malloc'd or (recently) free'd
+
+Invalid read of size 1
+   at 0x........: main (big_blocks_freed_list.c:34)
+ Address 0x........ is 2,000 bytes inside a block of size 900,000 free'd
+   at 0x........: free (vg_replace_malloc.c:...)
+   by 0x........: main (big_blocks_freed_list.c:20)
+
+Invalid read of size 1
+   at 0x........: main (big_blocks_freed_list.c:41)
+ Address 0x........ is 10 bytes inside a block of size 10,000 free'd
+   at 0x........: free (vg_replace_malloc.c:...)
+   by 0x........: main (big_blocks_freed_list.c:28)
+
+Invalid read of size 1
+   at 0x........: main (big_blocks_freed_list.c:46)
+ Address 0x........ is 10 bytes inside a block of size 1,000,001 free'd
+   at 0x........: free (vg_replace_malloc.c:...)
+   by 0x........: main (big_blocks_freed_list.c:40)
+
+Invalid read of size 1
+   at 0x........: main (big_blocks_freed_list.c:55)
+ Address 0x........ is 10 bytes inside a block of size 10,000 free'd
+   at 0x........: free (vg_replace_malloc.c:...)
+   by 0x........: main (big_blocks_freed_list.c:28)
+
+
+HEAP SUMMARY:
+    in use at exit: 1,000,000 bytes in 100 blocks
+  total heap usage: 104 allocs, 4 frees, 3,910,002 bytes allocated
+
+For a detailed leak analysis, rerun with: --leak-check=full
+
+For counts of detected and suppressed errors, rerun with: -v
+ERROR SUMMARY: 7 errors from 7 contexts (suppressed: 0 from 0)

memcheck/tests/big_blocks_freed_list.vgtest

@@ -0,0 +1,2 @@
+prog: big_blocks_freed_list
+vgopts: --freelist-vol=1000000 --freelist-big-blocks=50000

memcheck/tests/memalign2.vgtest

@@ -1,2 +1,2 @@
 prog: memalign2
-vgopts: -q --freelist-vol=100000
+vgopts: -q --freelist-vol=100000 --freelist-big-blocks=0