[PATCH] fix to malloc checking

[PATCH] fix to malloc checking

[James Lemke]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

glibc testing would sometimes generate this failure for me:
   FAIL: malloc/tst-malloc-usable.out
   malloc_usable_size: expected 7 but got 11

I found that the checking code fills the unused portion of a memory
chunk (end to front) with a chain of length-prefixed blocks
followed by a "magic" byte (a hash of the chunk's address).
Verification of the unused portion reads each block advancing via
the length bytes.  Sometimes a length byte would be interpreted as
the magic hash.  This caused the scan to terminate early and report a
larger usable size than actual.

My fix is to terminate the chain with a zero-length block.
The chain is still followed by the magic hash but there is now no ambiguity.

Tested with a mips-linux build where I noticed the problem.
Also tested with a x86_64 native make-check.  Results were unchanged.

OK to commit?

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia Ontario,  +1-613-963-1073

[-- Attachment #2: 14619-gli-1111a.diff --]
[-- Type: text/x-diff, Size: 5105 bytes --]

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

	* 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..ca487d8 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,17 @@ 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)

Re: [PATCH] fix to malloc checking

[Andreas Schwab]

James Lemke <jwlemke@codesourcery.com> writes:

> +/* 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)

If c > size then the difference wraps around.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] fix to malloc checking

[Joseph Myers]

As this bug is user-visible in past releases, it should be filed in 
Bugzilla if not already there (and the [BZ #N] notation used in the 
ChangeLog entry, and the bug added to the list of fixed bugs in NEWS when 
checking in the fix).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] fix to malloc checking

[James Lemke]

On 11/11/2014 03:32 PM, Joseph Myers wrote:
> As this bug is user-visible in past releases, it should be filed in
> Bugzilla if not already there (and the [BZ #N] notation used in the
> ChangeLog entry, and the bug added to the list of fixed bugs in NEWS when
> checking in the fix).

I have opened BZ 17581.
I'll mention it in ChangeLog & NEWS as you suggested.

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia Ontario,  +1-613-963-1073

Re: [PATCH] fix to malloc checking

[James Lemke]

On 11/11/2014 03:19 PM, Andreas Schwab wrote:
>> -      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 > size then the difference wraps around.

That would indicate memory corruption and the loop would terminate,
which it should.

However, if you think it's clearer, I can re-write the 3 occurrences of
this test as:
    if (size <= c + 2 * SIZE_SZ)

Otherwise OK?

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery

Re: [PATCH] fix to malloc checking

[Andreas Schwab]

James Lemke <jwlemke@codesourcery.com> writes:

> On 11/11/2014 03:19 PM, Andreas Schwab wrote:
>>> -      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 > size then the difference wraps around.
>
> That would indicate memory corruption and the loop would terminate,
> which it should.

This condition will not terminate it, and the next iteration will cause
size to wrap around.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] fix to malloc checking

[James Lemke]

On 11/11/2014 04:31 PM, Andreas Schwab wrote:
>>>> -      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 > size then the difference wraps around.
>> >
>> >That would indicate memory corruption and the loop would terminate,
>> >which it should.
> This condition will not terminate it, and the next iteration will cause
> size to wrap around.

Err, yes.  size is unsigned so you are correct.  Thanks for the input.

I will re-write the 3 instances of this test as:
     if (size <= c + 2 * SIZE_SZ)

Otherwise OK?

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia Ontario,  +1-613-963-1073

Re: [PATCH] fix to malloc checking

[James Lemke]

[-- Attachment #1: Type: text/plain, Size: 121 bytes --]

Ping..  OK to commit?

The updated patch is attached.

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery

[-- Attachment #2: 14619-gli-1111b.diff --]
[-- Type: text/x-diff, Size: 5118 bytes --]

2014-11-11  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..c18140d 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,17 @@ 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)

Re: [PATCH] fix to malloc checking

[James Lemke]

Ping!
Any opinions about committing this?
Cheers, Jim.

https://sourceware.org/ml/libc-alpha/2014-11/msg00219.html
https://sourceware.org/ml/libc-alpha/2014-11/msg00470.html

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery

Re: [PATCH] fix to malloc checking

[Will Newton]

On 25 November 2014 at 21:52, James Lemke <jwlemke@mentor.com> wrote:
> Ping!
> Any opinions about committing this?
> Cheers, Jim.
>
> https://sourceware.org/ml/libc-alpha/2014-11/msg00219.html
> https://sourceware.org/ml/libc-alpha/2014-11/msg00470.html

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.

-- 
Will Newton
Toolchain Working Group, Linaro

Re: [PATCH] fix to malloc checking

[James Lemke]

[-- Attachment #1: Type: text/plain, Size: 507 bytes --]

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

[-- Attachment #2: 14619-gli-1126a.diff --]
[-- Type: text/x-diff, Size: 5114 bytes --]

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)

Re: [PATCH] fix to malloc checking

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] fix to malloc checking

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] fix to malloc checking

[James Lemke]

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.

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia, Ontario

Re: [PATCH] fix to malloc checking

[James Lemke]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

> glibc testing would sometimes generate this failure for me:
>    FAIL: malloc/tst-malloc-usable.out
>    malloc_usable_size: expected 7 but got 11


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 ***


Sorry for the delay in returning to this.

My previous patch used a second checking byte (minimum 2, up from 1) that was
a problem in some circumstances.  This patch stays with a minimum of 1 byte.
It is a bit simpler.  Only the code to generate the checking bytes is
modified.  The verification code is essentially unchanged from the original.

This version solves the original problem, passes the iconvconfig test above
and my own testing.

OK to commit?

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia, Ontario

[-- Attachment #2: 14619-gli-0204a.diff --]
[-- Type: text/x-diff, Size: 4707 bytes --]

2015-02-04  James Lemke  <jwlemke@codesourcery.com>

	[BZ #17581]
	* malloc/hooks.c
	(magicbyte): Convert to a function and avoid returning 0x01.
	(mem2mem_check): Avoid using a length byte equal to the magic byte.
	(mem2chunk_check): Fix unsigned comparisons to zero.
	Hoist defs of sz and magic.

diff --git a/malloc/hooks.c b/malloc/hooks.c
index 0c4816f..191b1e2 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -88,11 +88,22 @@ __malloc_check_init (void)
    overruns.  The goal here is to avoid obscure crashes due to invalid
    usage, unlike in the MALLOC_DEBUG code. */
 
-#define MAGICBYTE(p) ((((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF)
+static unsigned char
+magicbyte (void *p)
+{
+  unsigned char magic;
+
+  magic = (((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF;
+  /* Do not return 1.  See the comment in mem2mem_check().  */
+  if (magic == 1)
+    ++magic;
+  return magic;
+}
+
 
-/* 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
+/* 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.  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.  */
@@ -101,7 +112,7 @@ malloc_check_get_size (mchunkptr p)
 {
   size_t size;
   unsigned char c;
-  unsigned char magic = MAGICBYTE (p);
+  unsigned char magic = magicbyte (p);
 
   assert (using_malloc_checking == 1);
 
@@ -122,32 +133,38 @@ malloc_check_get_size (mchunkptr p)
 }
 
 /* Instrument a chunk with overrun detector byte(s) and convert it
-   into a user pointer with requested size sz. */
+   into a user pointer with requested size req_sz. */
 
 static void *
 internal_function
-mem2mem_check (void *ptr, size_t sz)
+mem2mem_check (void *ptr, size_t req_sz)
 {
   mchunkptr p;
   unsigned char *m_ptr = ptr;
-  size_t i;
+  size_t max_sz, block_sz, i;
+  unsigned char magic;
 
   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)
+  magic = magicbyte (p);
+  max_sz = chunksize (p) - 2 * SIZE_SZ;
+  if (!chunk_is_mmapped (p))
+    max_sz += SIZE_SZ;
+  for (i = max_sz - 1; i > req_sz; i -= block_sz)
     {
-      if (i - sz < 0x100)
-        {
-          m_ptr[i] = (unsigned char) (i - sz);
-          break;
-        }
-      m_ptr[i] = 0xFF;
+      block_sz = i - req_sz;
+      if (block_sz > 0xff)
+        block_sz = 0xff;
+      /* Don't allow the magic byte to appear in the chain of length bytes.
+         For the following to work, magicbyte() cannot return 0x01.  */
+      if (block_sz == magic)
+        --block_sz;
+
+      m_ptr[i] = (unsigned char) block_sz;
     }
-  m_ptr[sz] = MAGICBYTE (p);
+  m_ptr[req_sz] = magic;
   return (void *) m_ptr;
 }
 
@@ -166,11 +183,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,10 +198,9 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }
@@ -201,13 +218,12 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }

Re: [PATCH] fix to malloc checking

[James Lemke]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

Ping..

On 02/04/2015 01:56 PM, James Lemke wrote:
>> glibc testing would sometimes generate this failure for me:
>>     FAIL: malloc/tst-malloc-usable.out
>>     malloc_usable_size: expected 7 but got 11
>
>
> 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 ***
>
>
> Sorry for the delay in returning to this.
>
> My previous patch used a second checking byte (minimum 2, up from 1) that was
> a problem in some circumstances.  This patch stays with a minimum of 1 byte.
> It is a bit simpler.  Only the code to generate the checking bytes is
> modified.  The verification code is essentially unchanged from the original.
>
> This version solves the original problem, passes the iconvconfig test above
> and my own testing.
>
> OK to commit?

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia, Ontario

[-- Attachment #2: 14619-gli-0204a.diff --]
[-- Type: text/x-diff, Size: 4707 bytes --]

2015-02-04  James Lemke  <jwlemke@codesourcery.com>

	[BZ #17581]
	* malloc/hooks.c
	(magicbyte): Convert to a function and avoid returning 0x01.
	(mem2mem_check): Avoid using a length byte equal to the magic byte.
	(mem2chunk_check): Fix unsigned comparisons to zero.
	Hoist defs of sz and magic.

diff --git a/malloc/hooks.c b/malloc/hooks.c
index 0c4816f..191b1e2 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -88,11 +88,22 @@ __malloc_check_init (void)
    overruns.  The goal here is to avoid obscure crashes due to invalid
    usage, unlike in the MALLOC_DEBUG code. */
 
-#define MAGICBYTE(p) ((((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF)
+static unsigned char
+magicbyte (void *p)
+{
+  unsigned char magic;
+
+  magic = (((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF;
+  /* Do not return 1.  See the comment in mem2mem_check().  */
+  if (magic == 1)
+    ++magic;
+  return magic;
+}
+
 
-/* 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
+/* 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.  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.  */
@@ -101,7 +112,7 @@ malloc_check_get_size (mchunkptr p)
 {
   size_t size;
   unsigned char c;
-  unsigned char magic = MAGICBYTE (p);
+  unsigned char magic = magicbyte (p);
 
   assert (using_malloc_checking == 1);
 
@@ -122,32 +133,38 @@ malloc_check_get_size (mchunkptr p)
 }
 
 /* Instrument a chunk with overrun detector byte(s) and convert it
-   into a user pointer with requested size sz. */
+   into a user pointer with requested size req_sz. */
 
 static void *
 internal_function
-mem2mem_check (void *ptr, size_t sz)
+mem2mem_check (void *ptr, size_t req_sz)
 {
   mchunkptr p;
   unsigned char *m_ptr = ptr;
-  size_t i;
+  size_t max_sz, block_sz, i;
+  unsigned char magic;
 
   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)
+  magic = magicbyte (p);
+  max_sz = chunksize (p) - 2 * SIZE_SZ;
+  if (!chunk_is_mmapped (p))
+    max_sz += SIZE_SZ;
+  for (i = max_sz - 1; i > req_sz; i -= block_sz)
     {
-      if (i - sz < 0x100)
-        {
-          m_ptr[i] = (unsigned char) (i - sz);
-          break;
-        }
-      m_ptr[i] = 0xFF;
+      block_sz = i - req_sz;
+      if (block_sz > 0xff)
+        block_sz = 0xff;
+      /* Don't allow the magic byte to appear in the chain of length bytes.
+         For the following to work, magicbyte() cannot return 0x01.  */
+      if (block_sz == magic)
+        --block_sz;
+
+      m_ptr[i] = (unsigned char) block_sz;
     }
-  m_ptr[sz] = MAGICBYTE (p);
+  m_ptr[req_sz] = magic;
   return (void *) m_ptr;
 }
 
@@ -166,11 +183,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,10 +198,9 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }
@@ -201,13 +218,12 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }

Re: [PATCH] fix to malloc checking

[James Lemke]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

Ping..  OK to commit?

On 02/04/2015 01:56 PM, James Lemke wrote:
>> glibc testing would sometimes generate this failure for me:
>>     FAIL: malloc/tst-malloc-usable.out
>>     malloc_usable_size: expected 7 but got 11
>
>
> 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 ***
:
> My previous patch used a second checking byte (minimum 2, up from 1) that was
> a problem in some circumstances.  This patch stays with a minimum of 1 byte.
> It is a bit simpler.  Only the code to generate the checking bytes is
> modified.  The verification code is essentially unchanged from the original.
>
> This version solves the original problem, passes the iconvconfig test above
> and my own testing.
>
> OK to commit?

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia, Ontario


[-- Attachment #2: 14619-gli-0204a.diff --]
[-- Type: text/x-diff, Size: 4708 bytes --]

2015-02-04  James Lemke  <jwlemke@codesourcery.com>

	[BZ #17581]
	* malloc/hooks.c
	(magicbyte): Convert to a function and avoid returning 0x01.
	(mem2mem_check): Avoid using a length byte equal to the magic byte.
	(mem2chunk_check): Fix unsigned comparisons to zero.
	Hoist defs of sz and magic.

diff --git a/malloc/hooks.c b/malloc/hooks.c
index 0c4816f..191b1e2 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -88,11 +88,22 @@ __malloc_check_init (void)
    overruns.  The goal here is to avoid obscure crashes due to invalid
    usage, unlike in the MALLOC_DEBUG code. */
 
-#define MAGICBYTE(p) ((((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF)
+static unsigned char
+magicbyte (void *p)
+{
+  unsigned char magic;
+
+  magic = (((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF;
+  /* Do not return 1.  See the comment in mem2mem_check().  */
+  if (magic == 1)
+    ++magic;
+  return magic;
+}
+
 
-/* 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
+/* 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.  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.  */
@@ -101,7 +112,7 @@ malloc_check_get_size (mchunkptr p)
 {
   size_t size;
   unsigned char c;
-  unsigned char magic = MAGICBYTE (p);
+  unsigned char magic = magicbyte (p);
 
   assert (using_malloc_checking == 1);
 
@@ -122,32 +133,38 @@ malloc_check_get_size (mchunkptr p)
 }
 
 /* Instrument a chunk with overrun detector byte(s) and convert it
-   into a user pointer with requested size sz. */
+   into a user pointer with requested size req_sz. */
 
 static void *
 internal_function
-mem2mem_check (void *ptr, size_t sz)
+mem2mem_check (void *ptr, size_t req_sz)
 {
   mchunkptr p;
   unsigned char *m_ptr = ptr;
-  size_t i;
+  size_t max_sz, block_sz, i;
+  unsigned char magic;
 
   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)
+  magic = magicbyte (p);
+  max_sz = chunksize (p) - 2 * SIZE_SZ;
+  if (!chunk_is_mmapped (p))
+    max_sz += SIZE_SZ;
+  for (i = max_sz - 1; i > req_sz; i -= block_sz)
     {
-      if (i - sz < 0x100)
-        {
-          m_ptr[i] = (unsigned char) (i - sz);
-          break;
-        }
-      m_ptr[i] = 0xFF;
+      block_sz = i - req_sz;
+      if (block_sz > 0xff)
+        block_sz = 0xff;
+      /* Don't allow the magic byte to appear in the chain of length bytes.
+         For the following to work, magicbyte() cannot return 0x01.  */
+      if (block_sz == magic)
+        --block_sz;
+
+      m_ptr[i] = (unsigned char) block_sz;
     }
-  m_ptr[sz] = MAGICBYTE (p);
+  m_ptr[req_sz] = magic;
   return (void *) m_ptr;
 }
 
@@ -166,11 +183,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,10 +198,9 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }
@@ -201,13 +218,12 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }

Re: [PATCH] fix to malloc checking

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

On 04 Feb 2015 13:56, James Lemke wrote:
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
>  
> +static unsigned char
> +magicbyte (void *p)

could be const

> +{
> +  unsigned char magic;
> +
> +  magic = (((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF;

shouldn't you use uintptr_t instead of size_t ?
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] fix to malloc checking

[James Lemke]

Sorry for the delay.  I was out of the office for two weeks.

On 03/01/2015 10:08 PM, Mike Frysinger wrote:
> On 04 Feb 2015 13:56, James Lemke wrote:
>> --- a/malloc/hooks.c
>> +++ b/malloc/hooks.c
>>
>> +static unsigned char
>> +magicbyte (void *p)
>
> could be const

I agree, it should be.  I have changed it to:
static unsigned char
magicbyte (const void *p)

>> +{
>> +  unsigned char magic;
>> +
>> +  magic = (((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF;
>
> shouldn't you use uintptr_t instead of size_t ?

It is size_t because that's what the previous macro implementation used.
I don't see a strong reason to change the casts to uinptr_t,
but if you do I'll change them.

Thanks for the comments Mike.
OK to commit?

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia, Ontario

Re: [PATCH] fix to malloc checking

[James Lemke]

Ping...

On 03/11/2015 02:00 PM, James Lemke wrote:
> Sorry for the delay.  I was out of the office for two weeks.
>
> On 03/01/2015 10:08 PM, Mike Frysinger wrote:
>> On 04 Feb 2015 13:56, James Lemke wrote:
>>> --- a/malloc/hooks.c
>>> +++ b/malloc/hooks.c
>>>
>>> +static unsigned char
>>> +magicbyte (void *p)
>>
>> could be const
>
> I agree, it should be.  I have changed it to:
> static unsigned char
> magicbyte (const void *p)
>
>>> +{
>>> +  unsigned char magic;
>>> +
>>> +  magic = (((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF;
>>
>> shouldn't you use uintptr_t instead of size_t ?
>
> It is size_t because that's what the previous macro implementation used.
> I don't see a strong reason to change the casts to uintptr_t,
> but if you do I'll change them.
>
> Thanks for the comments Mike.
> OK to commit?

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia, Ontario

Re: [PATCH] fix to malloc checking

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

On 11 Mar 2015 14:00, James Lemke wrote:
> On 03/01/2015 10:08 PM, Mike Frysinger wrote:
> > On 04 Feb 2015 13:56, James Lemke wrote:
> >> --- a/malloc/hooks.c
> >> +++ b/malloc/hooks.c
> >> +{
> >> +  unsigned char magic;
> >> +
> >> +  magic = (((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF;
> >
> > shouldn't you use uintptr_t instead of size_t ?
> 
> It is size_t because that's what the previous macro implementation used.
> I don't see a strong reason to change the casts to uinptr_t,
> but if you do I'll change them.

uintptr_t is the only type you can safely cast a pointer, but if the rest of the 
code is already using size_t, no need to change that in this commit
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] fix to malloc checking

[James Lemke]

On 04/04/2015 11:05 PM, Mike Frysinger wrote:
> On 11 Mar 2015 14:00, James Lemke wrote:
>> On 03/01/2015 10:08 PM, Mike Frysinger wrote:
>>> On 04 Feb 2015 13:56, James Lemke wrote:
>>>> --- a/malloc/hooks.c
>>>> +++ b/malloc/hooks.c
>>>> +{
>>>> +  unsigned char magic;
>>>> +
>>>> +  magic = (((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF;
>>>
>>> shouldn't you use uintptr_t instead of size_t ?
>>
>> It is size_t because that's what the previous macro implementation used.
>> I don't see a strong reason to change the casts to uinptr_t,
>> but if you do I'll change them.
>
> uintptr_t is the only type you can safely cast a pointer, but if the rest of the
> code is already using size_t, no need to change that in this commit

Hmm.  There is no other spot in the current code that uses (size_t).  I have
tested with (uintptr_t) and get the same results so I will switch to the
latter.

OK to commit with that change?
Jim.

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia, Ontario

Re: [PATCH] fix to malloc checking

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]

On 07 Apr 2015 09:44, James Lemke wrote:
> On 04/04/2015 11:05 PM, Mike Frysinger wrote:
> > On 11 Mar 2015 14:00, James Lemke wrote:
> >> On 03/01/2015 10:08 PM, Mike Frysinger wrote:
> >>> On 04 Feb 2015 13:56, James Lemke wrote:
> >>>> --- a/malloc/hooks.c
> >>>> +++ b/malloc/hooks.c
> >>>> +{
> >>>> +  unsigned char magic;
> >>>> +
> >>>> +  magic = (((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF;
> >>>
> >>> shouldn't you use uintptr_t instead of size_t ?
> >>
> >> It is size_t because that's what the previous macro implementation used.
> >> I don't see a strong reason to change the casts to uinptr_t,
> >> but if you do I'll change them.
> >
> > uintptr_t is the only type you can safely cast a pointer, but if the rest of the
> > code is already using size_t, no need to change that in this commit
> 
> Hmm.  There is no other spot in the current code that uses (size_t).  I have
> tested with (uintptr_t) and get the same results so I will switch to the
> latter.
> 
> OK to commit with that change?

probably ? :)  please post the latest version.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] fix to malloc checking

[James Lemke]

[-- Attachment #1: Type: text/plain, Size: 259 bytes --]

On 04/07/2015 11:10 PM, Mike Frysinger wrote:
>> OK to commit with that change?
> probably ? :)  please post the latest version.

The updated patch is attached.
OK to commit?

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia, Ontario

[-- Attachment #2: 14619-gli-0408a.diff --]
[-- Type: text/x-diff, Size: 4719 bytes --]

2015-04-08  James Lemke  <jwlemke@codesourcery.com>

	[BZ #17581]
	* malloc/hooks.c
	(magicbyte): Convert to a function and avoid returning 0x01.
	(mem2mem_check): Avoid using a length byte equal to the magic byte.
	(mem2chunk_check): Fix unsigned comparisons to zero.
	Hoist defs of sz and magic.

diff --git a/malloc/hooks.c b/malloc/hooks.c
index 0c4816f..d2c64e6 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -88,11 +88,22 @@ __malloc_check_init (void)
    overruns.  The goal here is to avoid obscure crashes due to invalid
    usage, unlike in the MALLOC_DEBUG code. */
 
-#define MAGICBYTE(p) ((((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF)
+static unsigned char
+magicbyte (const void *p)
+{
+  unsigned char magic;
+
+  magic = (((uintptr_t) p >> 3) ^ ((uintptr_t) p >> 11)) & 0xFF;
+  /* Do not return 1.  See the comment in mem2mem_check().  */
+  if (magic == 1)
+    ++magic;
+  return magic;
+}
+
 
-/* 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
+/* 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.  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.  */
@@ -101,7 +112,7 @@ malloc_check_get_size (mchunkptr p)
 {
   size_t size;
   unsigned char c;
-  unsigned char magic = MAGICBYTE (p);
+  unsigned char magic = magicbyte (p);
 
   assert (using_malloc_checking == 1);
 
@@ -122,32 +133,38 @@ malloc_check_get_size (mchunkptr p)
 }
 
 /* Instrument a chunk with overrun detector byte(s) and convert it
-   into a user pointer with requested size sz. */
+   into a user pointer with requested size req_sz. */
 
 static void *
 internal_function
-mem2mem_check (void *ptr, size_t sz)
+mem2mem_check (void *ptr, size_t req_sz)
 {
   mchunkptr p;
   unsigned char *m_ptr = ptr;
-  size_t i;
+  size_t max_sz, block_sz, i;
+  unsigned char magic;
 
   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)
+  magic = magicbyte (p);
+  max_sz = chunksize (p) - 2 * SIZE_SZ;
+  if (!chunk_is_mmapped (p))
+    max_sz += SIZE_SZ;
+  for (i = max_sz - 1; i > req_sz; i -= block_sz)
     {
-      if (i - sz < 0x100)
-        {
-          m_ptr[i] = (unsigned char) (i - sz);
-          break;
-        }
-      m_ptr[i] = 0xFF;
+      block_sz = i - req_sz;
+      if (block_sz > 0xff)
+        block_sz = 0xff;
+      /* Don't allow the magic byte to appear in the chain of length bytes.
+         For the following to work, magicbyte() cannot return 0x01.  */
+      if (block_sz == magic)
+        --block_sz;
+
+      m_ptr[i] = (unsigned char) block_sz;
     }
-  m_ptr[sz] = MAGICBYTE (p);
+  m_ptr[req_sz] = magic;
   return (void *) m_ptr;
 }
 
@@ -166,11 +183,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,10 +198,9 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }
@@ -201,13 +218,12 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }

Re: [PATCH] fix to malloc checking

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

On 08 Apr 2015 11:41, James Lemke wrote:
> +      block_sz = i - req_sz;
> +      if (block_sz > 0xff)
> +        block_sz = 0xff;

these three lines can be written:
	block_sz = min (i - req_sz, 0xff);

> +      /* Don't allow the magic byte to appear in the chain of length bytes.
> +         For the following to work, magicbyte() cannot return 0x01.  */

GNU style says to omit the () in comments

> +      m_ptr[i] = (unsigned char) block_sz;

i wonder if the cast is really needed ... they're both unsigned already

otherwise patch looks fine to me
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] fix to malloc checking

[James Lemke]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On 04/12/2015 04:09 AM, Mike Frysinger wrote:
> On 08 Apr 2015 11:41, James Lemke wrote:
>> +      block_sz = i - req_sz;
>> +      if (block_sz > 0xff)
>> +        block_sz = 0xff;
>
> these three lines can be written:
> 	block_sz = min (i - req_sz, 0xff);
>
>> +      /* Don't allow the magic byte to appear in the chain of length bytes.
>> +         For the following to work, magicbyte() cannot return 0x01.  */
>
> GNU style says to omit the () in comments
>
>> +      m_ptr[i] = (unsigned char) block_sz;
>
> i wonder if the cast is really needed ... they're both unsigned already
>
> otherwise patch looks fine to me

I've incorporated your three suggestions.  Tests are OK.
The updated patch is attached.  OK to commit?
Jim.

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia, Ontario

[-- Attachment #2: 14619-gli-0414a.diff --]
[-- Type: text/x-diff, Size: 4659 bytes --]

2015-04-15  James Lemke  <jwlemke@codesourcery.com>

	[BZ #17581]
	* malloc/hooks.c
	(magicbyte): Convert to a function and avoid returning 0x01.
	(mem2mem_check): Avoid using a length byte equal to the magic byte.
	(mem2chunk_check): Fix unsigned comparisons to zero.
	Hoist defs of sz and magic.

diff --git a/malloc/hooks.c b/malloc/hooks.c
index 0c4816f..715afae 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -88,11 +88,22 @@ __malloc_check_init (void)
    overruns.  The goal here is to avoid obscure crashes due to invalid
    usage, unlike in the MALLOC_DEBUG code. */
 
-#define MAGICBYTE(p) ((((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF)
+static unsigned char
+magicbyte (const void *p)
+{
+  unsigned char magic;
+
+  magic = (((uintptr_t) p >> 3) ^ ((uintptr_t) p >> 11)) & 0xFF;
+  /* Do not return 1.  See the comment in mem2mem_check().  */
+  if (magic == 1)
+    ++magic;
+  return magic;
+}
+
 
-/* 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
+/* 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.  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.  */
@@ -101,7 +112,7 @@ malloc_check_get_size (mchunkptr p)
 {
   size_t size;
   unsigned char c;
-  unsigned char magic = MAGICBYTE (p);
+  unsigned char magic = magicbyte (p);
 
   assert (using_malloc_checking == 1);
 
@@ -122,32 +133,36 @@ malloc_check_get_size (mchunkptr p)
 }
 
 /* Instrument a chunk with overrun detector byte(s) and convert it
-   into a user pointer with requested size sz. */
+   into a user pointer with requested size req_sz. */
 
 static void *
 internal_function
-mem2mem_check (void *ptr, size_t sz)
+mem2mem_check (void *ptr, size_t req_sz)
 {
   mchunkptr p;
   unsigned char *m_ptr = ptr;
-  size_t i;
+  size_t max_sz, block_sz, i;
+  unsigned char magic;
 
   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)
+  magic = magicbyte (p);
+  max_sz = chunksize (p) - 2 * SIZE_SZ;
+  if (!chunk_is_mmapped (p))
+    max_sz += SIZE_SZ;
+  for (i = max_sz - 1; i > req_sz; i -= block_sz)
     {
-      if (i - sz < 0x100)
-        {
-          m_ptr[i] = (unsigned char) (i - sz);
-          break;
-        }
-      m_ptr[i] = 0xFF;
+      block_sz = MIN (i - req_sz, 0xff);
+      /* Don't allow the magic byte to appear in the chain of length bytes.
+         For the following to work, magicbyte cannot return 0x01.  */
+      if (block_sz == magic)
+        --block_sz;
+
+      m_ptr[i] = block_sz;
     }
-  m_ptr[sz] = MAGICBYTE (p);
+  m_ptr[req_sz] = magic;
   return (void *) m_ptr;
 }
 
@@ -166,11 +181,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,10 +196,9 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }
@@ -201,13 +216,12 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }

Re: [PATCH] fix to malloc checking

[James Lemke]

[-- Attachment #1: Type: text/plain, Size: 200 bytes --]


> I've incorporated your three suggestions.  Tests are OK.
> The updated patch is attached.  OK to commit?

Ping..

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia, Ontario

[-- Attachment #2: 14619-gli-0414a.diff --]
[-- Type: text/x-diff, Size: 4659 bytes --]

2015-04-15  James Lemke  <jwlemke@codesourcery.com>

	[BZ #17581]
	* malloc/hooks.c
	(magicbyte): Convert to a function and avoid returning 0x01.
	(mem2mem_check): Avoid using a length byte equal to the magic byte.
	(mem2chunk_check): Fix unsigned comparisons to zero.
	Hoist defs of sz and magic.

diff --git a/malloc/hooks.c b/malloc/hooks.c
index 0c4816f..715afae 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -88,11 +88,22 @@ __malloc_check_init (void)
    overruns.  The goal here is to avoid obscure crashes due to invalid
    usage, unlike in the MALLOC_DEBUG code. */
 
-#define MAGICBYTE(p) ((((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF)
+static unsigned char
+magicbyte (const void *p)
+{
+  unsigned char magic;
+
+  magic = (((uintptr_t) p >> 3) ^ ((uintptr_t) p >> 11)) & 0xFF;
+  /* Do not return 1.  See the comment in mem2mem_check().  */
+  if (magic == 1)
+    ++magic;
+  return magic;
+}
+
 
-/* 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
+/* 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.  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.  */
@@ -101,7 +112,7 @@ malloc_check_get_size (mchunkptr p)
 {
   size_t size;
   unsigned char c;
-  unsigned char magic = MAGICBYTE (p);
+  unsigned char magic = magicbyte (p);
 
   assert (using_malloc_checking == 1);
 
@@ -122,32 +133,36 @@ malloc_check_get_size (mchunkptr p)
 }
 
 /* Instrument a chunk with overrun detector byte(s) and convert it
-   into a user pointer with requested size sz. */
+   into a user pointer with requested size req_sz. */
 
 static void *
 internal_function
-mem2mem_check (void *ptr, size_t sz)
+mem2mem_check (void *ptr, size_t req_sz)
 {
   mchunkptr p;
   unsigned char *m_ptr = ptr;
-  size_t i;
+  size_t max_sz, block_sz, i;
+  unsigned char magic;
 
   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)
+  magic = magicbyte (p);
+  max_sz = chunksize (p) - 2 * SIZE_SZ;
+  if (!chunk_is_mmapped (p))
+    max_sz += SIZE_SZ;
+  for (i = max_sz - 1; i > req_sz; i -= block_sz)
     {
-      if (i - sz < 0x100)
-        {
-          m_ptr[i] = (unsigned char) (i - sz);
-          break;
-        }
-      m_ptr[i] = 0xFF;
+      block_sz = MIN (i - req_sz, 0xff);
+      /* Don't allow the magic byte to appear in the chain of length bytes.
+         For the following to work, magicbyte cannot return 0x01.  */
+      if (block_sz == magic)
+        --block_sz;
+
+      m_ptr[i] = block_sz;
     }
-  m_ptr[sz] = MAGICBYTE (p);
+  m_ptr[req_sz] = magic;
   return (void *) m_ptr;
 }
 
@@ -166,11 +181,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,10 +196,9 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }
@@ -201,13 +216,12 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }

Re: [PATCH] fix to malloc checking

[James Lemke]

[-- Attachment #1: Type: text/plain, Size: 298 bytes --]

On 04/27/2015 09:23 AM, James Lemke wrote:
>> I've incorporated your three suggestions.  Tests are OK.
>> >The updated patch is attached.  OK to commit?
> Ping..

Ping..
Are there any other comments?
OK to commit?

-- 
Jim Lemke, GNU Tools Sourcerer
Mentor Graphics / CodeSourcery
Orillia, Ontario

[-- Attachment #2: 14619-gli-0414a.diff --]
[-- Type: text/x-diff, Size: 4659 bytes --]

2015-04-15  James Lemke  <jwlemke@codesourcery.com>

	[BZ #17581]
	* malloc/hooks.c
	(magicbyte): Convert to a function and avoid returning 0x01.
	(mem2mem_check): Avoid using a length byte equal to the magic byte.
	(mem2chunk_check): Fix unsigned comparisons to zero.
	Hoist defs of sz and magic.

diff --git a/malloc/hooks.c b/malloc/hooks.c
index 0c4816f..715afae 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -88,11 +88,22 @@ __malloc_check_init (void)
    overruns.  The goal here is to avoid obscure crashes due to invalid
    usage, unlike in the MALLOC_DEBUG code. */
 
-#define MAGICBYTE(p) ((((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF)
+static unsigned char
+magicbyte (const void *p)
+{
+  unsigned char magic;
+
+  magic = (((uintptr_t) p >> 3) ^ ((uintptr_t) p >> 11)) & 0xFF;
+  /* Do not return 1.  See the comment in mem2mem_check().  */
+  if (magic == 1)
+    ++magic;
+  return magic;
+}
+
 
-/* 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
+/* 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.  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.  */
@@ -101,7 +112,7 @@ malloc_check_get_size (mchunkptr p)
 {
   size_t size;
   unsigned char c;
-  unsigned char magic = MAGICBYTE (p);
+  unsigned char magic = magicbyte (p);
 
   assert (using_malloc_checking == 1);
 
@@ -122,32 +133,36 @@ malloc_check_get_size (mchunkptr p)
 }
 
 /* Instrument a chunk with overrun detector byte(s) and convert it
-   into a user pointer with requested size sz. */
+   into a user pointer with requested size req_sz. */
 
 static void *
 internal_function
-mem2mem_check (void *ptr, size_t sz)
+mem2mem_check (void *ptr, size_t req_sz)
 {
   mchunkptr p;
   unsigned char *m_ptr = ptr;
-  size_t i;
+  size_t max_sz, block_sz, i;
+  unsigned char magic;
 
   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)
+  magic = magicbyte (p);
+  max_sz = chunksize (p) - 2 * SIZE_SZ;
+  if (!chunk_is_mmapped (p))
+    max_sz += SIZE_SZ;
+  for (i = max_sz - 1; i > req_sz; i -= block_sz)
     {
-      if (i - sz < 0x100)
-        {
-          m_ptr[i] = (unsigned char) (i - sz);
-          break;
-        }
-      m_ptr[i] = 0xFF;
+      block_sz = MIN (i - req_sz, 0xff);
+      /* Don't allow the magic byte to appear in the chain of length bytes.
+         For the following to work, magicbyte cannot return 0x01.  */
+      if (block_sz == magic)
+        --block_sz;
+
+      m_ptr[i] = block_sz;
     }
-  m_ptr[sz] = MAGICBYTE (p);
+  m_ptr[req_sz] = magic;
   return (void *) m_ptr;
 }
 
@@ -166,11 +181,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,10 +196,9 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }
@@ -201,13 +216,12 @@ 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)
         {
-          if (c <= 0 || sz < (c + 2 * SIZE_SZ))
+          if (c == 0 || sz < (c + 2 * SIZE_SZ))
             return NULL;
         }
     }

Re: [PATCH] fix to malloc checking

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 293 bytes --]

On 05 May 2015 13:08, James Lemke wrote:
> On 04/27/2015 09:23 AM, James Lemke wrote:
> >> I've incorporated your three suggestions.  Tests are OK.
> >> >The updated patch is attached.  OK to commit?
> > Ping..
> 
> Ping..
> Are there any other comments?
> OK to commit?

OK
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]