On Wed, May 14, 2014 at 03:28:02PM -0400, Ted Unangst wrote:
> As I learned the hard way not long ago, free() doesn't detect all
> errors because of the delay mechanism. We can make two improvements.
>
> 1. Perform the sanity checking from free_bytes before we insert
> something into the delay array. This detects many kinds of badness
> much sooner.
>
> 2. Check that the freed pointer isn't the same as the pointer in the
> delay array. Checking the entire array would be more complete, but
> slower. Randomly crashing immediately is a modest improvement.
I like both. I plan to test this diff this weekend, hoping real-life
does not interfere.
-Otto
>
> Index: malloc.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.163
> diff -u -p -r1.163 malloc.c
> --- malloc.c 12 May 2014 19:02:20 -0000 1.163
> +++ malloc.c 14 May 2014 19:21:15 -0000
> @@ -966,16 +966,11 @@ malloc_bytes(struct dir_info *d, size_t
> return ((char *)bp->page + k);
> }
>
> -
> -/*
> - * Free a chunk, and possibly the page it's on, if the page becomes empty.
> - */
> static void
> -free_bytes(struct dir_info *d, struct region_info *r, void *ptr)
> +check_free_chunk(struct dir_info *d, struct region_info *r, void *ptr)
> {
> - struct chunk_head *mp;
> struct chunk_info *info;
> - int i, listnum;
> + int i;
>
> info = (struct chunk_info *)r->size;
> if (info->canary != d->canary1)
> @@ -992,6 +987,24 @@ free_bytes(struct dir_info *d, struct re
> wrterror("chunk is already free", ptr);
> return;
> }
> +}
> +
> +/*
> + * Free a chunk, and possibly the page it's on, if the page becomes empty.
> + */
> +static void
> +free_bytes(struct dir_info *d, struct region_info *r, void *ptr)
> +{
> + struct chunk_head *mp;
> + struct chunk_info *info;
> + int i, listnum;
> +
> + check_free_chunk(d, r, ptr);
> +
> + info = (struct chunk_info *)r->size;
> +
> + /* Find the chunk number on the page */
> + i = ((uintptr_t)ptr & MALLOC_PAGEMASK) >> info->shift;
>
> info->bits[i / MALLOC_BITS] |= 1U << (i % MALLOC_BITS);
> info->free++;
> @@ -1204,9 +1217,14 @@ ofree(void *p)
> if (mopts.malloc_junk && sz > 0)
> memset(p, SOME_FREEJUNK, sz);
> if (!mopts.malloc_freenow) {
> + check_free_chunk(g_pool, r, p);
> i = getrbyte() & MALLOC_DELAYED_CHUNK_MASK;
> tmp = p;
> p = g_pool->delayed_chunks[i];
> + if (tmp == p) {
> + wrterror("double free", p);
> + return;
> + }
> g_pool->delayed_chunks[i] = tmp;
> }
> if (p != NULL) {
>
