On Fri, May 16, 2014 at 20:01, Otto Moerbeek wrote:
> 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 think you can return i in check_free_chunk() and use that in free_bytes().
Yes, but I think we should check for errors, too. I don't think
anyone actually disables abort, but since that's an option, we should
deal with it.
This rearranges a bit more, and makes the chunknum unsigned which may
help the compiler with the mod math.
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 16 May 2014 18:17:57 -0000
@@ -966,34 +966,47 @@ 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)
+static uint32_t
+find_chunknum(struct dir_info *d, struct region_info *r, void *ptr)
{
- struct chunk_head *mp;
struct chunk_info *info;
- int i, listnum;
+ uint32_t chunknum;
info = (struct chunk_info *)r->size;
if (info->canary != d->canary1)
wrterror("chunk info corrupted", NULL);
/* Find the chunk number on the page */
- i = ((uintptr_t)ptr & MALLOC_PAGEMASK) >> info->shift;
+ chunknum = ((uintptr_t)ptr & MALLOC_PAGEMASK) >> info->shift;
if ((uintptr_t)ptr & ((1U << (info->shift)) - 1)) {
wrterror("modified chunk-pointer", ptr);
- return;
+ return -1;
}
- if (info->bits[i / MALLOC_BITS] & (1U << (i % MALLOC_BITS))) {
+ if (info->bits[chunknum / MALLOC_BITS] &
+ (1U << (chunknum % MALLOC_BITS))) {
wrterror("chunk is already free", ptr);
- return;
+ return -1;
}
+ return chunknum;
+}
- info->bits[i / MALLOC_BITS] |= 1U << (i % MALLOC_BITS);
+/*
+ * 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;
+ uint32_t chunknum;
+ int listnum;
+
+ info = (struct chunk_info *)r->size;
+ if ((chunknum = find_chunknum(d, r, ptr)) == -1)
+ return;
+
+ info->bits[chunknum / MALLOC_BITS] |= 1U << (chunknum % MALLOC_BITS);
info->free++;
if (info->free == 1) {
@@ -1204,9 +1217,15 @@ ofree(void *p)
if (mopts.malloc_junk && sz > 0)
memset(p, SOME_FREEJUNK, sz);
if (!mopts.malloc_freenow) {
+ if (find_chunknum(g_pool, r, p) == -1)
+ return;
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) {
