public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] fix to malloc checking
@ 2014-11-11 18:45 James Lemke
  2014-11-11 20:19 ` Andreas Schwab
                   ` (3 more replies)
  0 siblings, 4 replies; 29+ messages in thread
From: James Lemke @ 2014-11-11 18:45 UTC (permalink / raw)
  To: libc-alpha

[-- 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)

^ permalink raw reply	[flat|nested] 29+ messages in thread
end of thread, other threads:[~2015-05-16 10:06 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-11 18:45 [PATCH] fix to malloc checking James Lemke
2014-11-11 20:19 ` Andreas Schwab
2014-11-11 21:18   ` James Lemke
2014-11-11 21:31     ` Andreas Schwab
2014-11-11 21:43       ` James Lemke
2014-11-11 20:32 ` Joseph Myers
2014-11-11 21:07   ` James Lemke
2014-11-18 20:24 ` James Lemke
2014-11-25 21:52 ` James Lemke
2014-11-26  9:40   ` Will Newton
2014-11-26 19:30     ` James Lemke
2014-11-29  1:42       ` Siddhesh Poyarekar
2014-12-11 11:03       ` Andreas Schwab
2014-12-11 21:23         ` James Lemke
2015-02-04 18:56         ` James Lemke
2015-02-09 15:04           ` James Lemke
2015-02-17 16:22           ` James Lemke
2015-03-02  3:09           ` Mike Frysinger
2015-03-11 18:00             ` James Lemke
2015-03-31 14:15               ` James Lemke
2015-04-05  3:06               ` Mike Frysinger
2015-04-07 13:44                 ` James Lemke
2015-04-08  3:11                   ` Mike Frysinger
2015-04-08 15:41                     ` James Lemke
2015-04-12  8:09                       ` Mike Frysinger
2015-04-15 15:04                         ` James Lemke
2015-04-27 13:23                           ` James Lemke
2015-05-05 17:09                             ` James Lemke
2015-05-18  7:35                               ` Mike Frysinger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).