This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] fix to malloc checking
- From: James Lemke <jwlemke at mentor dot com>
- To: libc-alpha <libc-alpha at sourceware dot org>, Mike Frysinger <vapier at gentoo dot org>
- Cc: Andreas Schwab <schwab at suse dot de>, Will Newton <will dot newton at gmail dot com>
- Date: Mon, 27 Apr 2015 09:23:14 -0400
- Subject: Re: [PATCH] fix to malloc checking
- Authentication-results: sourceware.org; auth=none
- References: <CANu=DmhJ3FqfxCKqoVyuS77UiJ92V8+a3ZuKo3xPX1pxppbxkw at mail dot gmail dot com> <54762A68 dot 5050801 at codesourcery dot com> <mvma92uqwhg dot fsf at hawking dot suse dot de> <54D26B54 dot 9050707 at codesourcery dot com> <20150302030859 dot GN19363 at vapier> <550082A3 dot 2010503 at codesourcery dot com> <20150405030555 dot GH16816 at vapier> <5523DF4B dot 9070407 at codesourcery dot com> <20150408031058 dot GT16816 at vapier> <55254C2E dot 4040504 at codesourcery dot com> <20150412080944 dot GE16816 at vapier> <552E7DE1 dot 60407 at codesourcery dot com>
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
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;
}
}