fix to malloc checking

Message ID 54762A68.5050801@codesourcery.com
State Committed
Headers

Commit Message

James Lemke Nov. 26, 2014, 7:30 p.m. UTC
  On 11/26/2014 04:40 AM, Will Newton wrote:
> This looks ok to me, although there are some minor style issues, e.g.:
>
> There should be a space after casts.
> There's a bracket on its own that should be moved onto the line above.
> A couple of if statements have some leading space that should not be there.

Thanks for the review.  This version fixes the style issues above.
Unless anyone has other issues I will commit this version.
Jim.
  

Comments

Siddhesh Poyarekar Nov. 29, 2014, 1:42 a.m. UTC | #1
On Wed, Nov 26, 2014 at 02:30:48PM -0500, James Lemke wrote:
> On 11/26/2014 04:40 AM, Will Newton wrote:
> >This looks ok to me, although there are some minor style issues, e.g.:
> >
> >There should be a space after casts.
> >There's a bracket on its own that should be moved onto the line above.
> >A couple of if statements have some leading space that should not be there.
> 
> Thanks for the review.  This version fixes the style issues above.
> Unless anyone has other issues I will commit this version.
> Jim.
> 
> -- 
> Jim Lemke, GNU Tools Sourcerer
> Mentor Graphics / CodeSourcery

> 2014-11-26  James Lemke  <jwlemke@codesourcery.com>
> 
> 	[BZ #17581]
> 	* malloc/hooks.c
> 	(mem2mem_check): Add a terminator to the chain of checking blocks.
> 	(malloc_check_get_size): Use it here.
> 	(mem2chunk_check): Ditto.
> 

Looks OK to me.

Siddhesh
  
Andreas Schwab Dec. 11, 2014, 11:03 a.m. UTC | #2
James Lemke <jwlemke@codesourcery.com> writes:

> On 11/26/2014 04:40 AM, Will Newton wrote:
>> This looks ok to me, although there are some minor style issues, e.g.:
>>
>> There should be a space after casts.
>> There's a bracket on its own that should be moved onto the line above.
>> A couple of if statements have some leading space that should not be there.
>
> Thanks for the review.  This version fixes the style issues above.
> Unless anyone has other issues I will commit this version.

This is broken, please revert.

$ MALLOC_CHECK_=3 iconv/iconvconfig --prefix=$PWD/root
*** Error in `iconv/iconvconfig': free(): invalid pointer: 0x09f15018 ***
======= Backtrace: =========
/suse/schwab/src/libc/i586/libc.so.6(+0x6cd73)[0xf7691d73]
/suse/schwab/src/libc/i586/libc.so.6(+0x72933)[0xf7697933]
/suse/schwab/src/libc/i586/libc.so.6(cfree+0x9b)[0xf769b31b]
/suse/schwab/src/libc/i586/libc.so.6(+0x2acea)[0xf764fcea]
/suse/schwab/src/libc/i586/libc.so.6(+0x23f35)[0xf7648f35]
/suse/schwab/src/libc/i586/libc.so.6(setlocale+0x2cf)[0xf76487af]
iconv/iconvconfig[0x8048dd5]
/suse/schwab/src/libc/i586/libc.so.6(__libc_start_main+0xf3)[0xf763e6f3]
iconv/iconvconfig[0x8049a96]
======= Memory map: ========
08048000-0804f000 r-xp 00000000 08:03 50072439                           /daten/src/libc/i586/iconv/iconvconfig
0804f000-08050000 r--p 00006000 08:03 50072439                           /daten/src/libc/i586/iconv/iconvconfig
08050000-08051000 rw-p 00007000 08:03 50072439                           /daten/src/libc/i586/iconv/iconvconfig
09f15000-09f36000 rw-p 00000000 00:00 0                                  [heap]
f75db000-f75f6000 r-xp 00000000 08:02 1319905                            /lib/libgcc_s.so.1
f75f6000-f75f7000 r--p 0001a000 08:02 1319905                            /lib/libgcc_s.so.1
f75f7000-f75f8000 rw-p 0001b000 08:02 1319905                            /lib/libgcc_s.so.1
f7623000-f7625000 rw-p 00000000 00:00 0 
f7625000-f7787000 r-xp 00000000 08:03 49819422                           /daten/src/libc/i586/libc.so
f7787000-f7789000 r--p 00161000 08:03 49819422                           /daten/src/libc/i586/libc.so
f7789000-f778a000 rw-p 00163000 08:03 49819422                           /daten/src/libc/i586/libc.so
f778a000-f778e000 rw-p 00000000 00:00 0 
f778e000-f778f000 r-xp 00000000 00:00 0                                  [vdso]
f778f000-f77af000 r-xp 00000000 08:03 50080584                           /daten/src/libc/i586/elf/ld.so
f77af000-f77b0000 r--p 0001f000 08:03 50080584                           /daten/src/libc/i586/elf/ld.so
f77b0000-f77b1000 rw-p 00020000 08:03 50080584                           /daten/src/libc/i586/elf/ld.so
ff8e7000-ff909000 rw-p 00000000 00:00 0                                  [stack]
Aborted

Andreas.
  
James Lemke Dec. 11, 2014, 9:22 p.m. UTC | #3
On 12/11/2014 06:03 AM, Andreas Schwab wrote:
> This is broken, please revert.
>
> $ MALLOC_CHECK_=3 iconv/iconvconfig --prefix=$PWD/root
> *** Error in `iconv/iconvconfig': free(): invalid pointer: 0x09f15018 ***

I will revert it today and then investigate.
  

Patch

2014-11-26  James Lemke  <jwlemke@codesourcery.com>

	[BZ #17581]
	* malloc/hooks.c
	(mem2mem_check): Add a terminator to the chain of checking blocks.
	(malloc_check_get_size): Use it here.
	(mem2chunk_check): Ditto.

diff --git a/malloc/hooks.c b/malloc/hooks.c
index 00ee6be..996111a 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -90,31 +90,35 @@  __malloc_check_init (void)
 
 #define MAGICBYTE(p) ((((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF)
 
-/* Visualize the chunk as being partitioned into blocks of 256 bytes from the
-   highest address of the chunk, downwards.  The beginning of each block tells
-   us the size of the previous block, up to the actual size of the requested
-   memory.  Our magic byte is right at the end of the requested size, so we
-   must reach it with this iteration, otherwise we have witnessed a memory
-   corruption.  */
+/* Visualize the chunk as being partitioned into blocks of 255 bytes from the
+   highest address of the chunk, downwards.  The end of each block tells us
+   the size of that block, up to the actual size of the requested memory.
+   The last block has a length of zero and is followed by the magic byte.
+   Our magic byte is right at the end of the requested size.  If we don't
+   reach it with this iteration we have witnessed a memory corruption.  */
 static size_t
 malloc_check_get_size (mchunkptr p)
 {
-  size_t size;
+  size_t total_sz, size;
   unsigned char c;
   unsigned char magic = MAGICBYTE (p);
 
   assert (using_malloc_checking == 1);
 
-  for (size = chunksize (p) - 1 + (chunk_is_mmapped (p) ? 0 : SIZE_SZ);
-       (c = ((unsigned char *) p)[size]) != magic;
+  /* Validate the length-byte chain.  */
+  total_sz = chunksize (p) + (chunk_is_mmapped (p) ? 0 : SIZE_SZ);
+  for (size = total_sz - 1;
+       (c = ((unsigned char *) p)[size]) != 0;
        size -= c)
     {
-      if (c <= 0 || size < (c + 2 * SIZE_SZ))
-        {
-          malloc_printerr (check_action, "malloc_check_get_size: memory corruption",
-                           chunk2mem (p));
-          return 0;
-        }
+      if (size <= c + 2 * SIZE_SZ)
+	break;
+    }
+  if (c != 0 || ((unsigned char *) p)[--size] != magic)
+    {
+      malloc_printerr (check_action, "malloc_check_get_size: memory corruption",
+		       chunk2mem (p));
+      return 0;
     }
 
   /* chunk2mem size.  */
@@ -130,22 +134,24 @@  mem2mem_check (void *ptr, size_t sz)
 {
   mchunkptr p;
   unsigned char *m_ptr = ptr;
-  size_t i;
+  size_t user_sz, block_sz, i;
 
   if (!ptr)
     return ptr;
 
   p = mem2chunk (ptr);
-  for (i = chunksize (p) - (chunk_is_mmapped (p) ? 2 * SIZE_SZ + 1 : SIZE_SZ + 1);
-       i > sz;
-       i -= 0xFF)
+  user_sz = chunksize (p) + (chunk_is_mmapped (p) ? 0 : SIZE_SZ);
+  user_sz -= 2 * SIZE_SZ;
+  for (i = user_sz - 1; i > sz; i -= block_sz)
     {
-      if (i - sz < 0x100)
-        {
-          m_ptr[i] = (unsigned char) (i - sz);
-          break;
-        }
-      m_ptr[i] = 0xFF;
+      block_sz = i - (sz + 1);
+      if (block_sz > 0xff)
+	block_sz = 0xff;
+
+      m_ptr[i] = (unsigned char) block_sz;
+
+      if (block_sz == 0)
+	break;
     }
   m_ptr[sz] = MAGICBYTE (p);
   return (void *) m_ptr;
@@ -166,11 +172,12 @@  mem2chunk_check (void *mem, unsigned char **magic_p)
     return NULL;
 
   p = mem2chunk (mem);
+  sz = chunksize (p);
+  magic = MAGICBYTE (p);
   if (!chunk_is_mmapped (p))
     {
       /* Must be a chunk in conventional heap memory. */
       int contig = contiguous (&main_arena);
-      sz = chunksize (p);
       if ((contig &&
            ((char *) p < mp_.sbrk_base ||
             ((char *) p + sz) >= (mp_.sbrk_base + main_arena.system_mem))) ||
@@ -180,12 +187,13 @@  mem2chunk_check (void *mem, unsigned char **magic_p)
                                next_chunk (prev_chunk (p)) != p)))
         return NULL;
 
-      magic = MAGICBYTE (p);
-      for (sz += SIZE_SZ - 1; (c = ((unsigned char *) p)[sz]) != magic; sz -= c)
+      for (sz += SIZE_SZ - 1; (c = ((unsigned char *) p)[sz]) != 0; sz -= c)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
-            return NULL;
+	  if (sz <= c + 2 * SIZE_SZ)
+	    break;
         }
+      if (c != 0 || ((unsigned char *) p)[--sz] != magic)
+	return NULL;
     }
   else
     {
@@ -201,15 +209,16 @@  mem2chunk_check (void *mem, unsigned char **magic_p)
            offset < 0x2000) ||
           !chunk_is_mmapped (p) || (p->size & PREV_INUSE) ||
           ((((unsigned long) p - p->prev_size) & page_mask) != 0) ||
-          ((sz = chunksize (p)), ((p->prev_size + sz) & page_mask) != 0))
+          ((p->prev_size + sz) & page_mask) != 0)
         return NULL;
 
-      magic = MAGICBYTE (p);
-      for (sz -= 1; (c = ((unsigned char *) p)[sz]) != magic; sz -= c)
+      for (sz -= 1; (c = ((unsigned char *) p)[sz]) != 0; sz -= c)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
-            return NULL;
+	  if (sz <= c + 2 * SIZE_SZ)
+	    break;
         }
+      if (c != 0 || ((unsigned char *) p)[--sz] != magic)
+	return NULL;
     }
   ((unsigned char *) p)[sz] ^= 0xFF;
   if (magic_p)