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