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>
- Cc: Andreas Schwab <schwab at suse dot de>, Will Newton <will dot newton at linaro dot org>
- Date: Mon, 9 Feb 2015 10:04:31 -0500
- Subject: Re: [PATCH] fix to malloc checking
- Authentication-results: sourceware.org; auth=none
- References: <5462592E dot 9050301 at codesourcery dot com> <5474FA02 dot 6020701 at mentor dot com> <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>
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
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;
}
}