diff mbox

fix to malloc checking

Message ID 54D26B54.9050707@codesourcery.com
State New
Headers show

Commit Message

James Lemke Feb. 4, 2015, 6:56 p.m. UTC 
> 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?

Comments

Mike Frysinger March 2, 2015, 3:08 a.m. UTC | #1 
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
James Lemke March 11, 2015, 6 p.m. UTC | #2 
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?
James Lemke March 31, 2015, 2:15 p.m. UTC | #3 
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?
Mike Frysinger April 5, 2015, 3:05 a.m. UTC | #4 
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
James Lemke April 7, 2015, 1:44 p.m. UTC | #5 
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.
Mike Frysinger April 8, 2015, 3:10 a.m. UTC | #6 
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
diff mbox

Patch

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;
         }
     }