Sorry for this rather long mail:
I have three small comments on the patch itself
(starting 80 lines below).
For those who want to try both new features, I attached a patch against
-current that merges the three parts of Daniel's diff (plus the trivial
two of the nits below) at the very end of this mail.
In addition to Daniel's test program, here's a small program that
in 1000 test runs on amd64 always triggered an abort at the second
free().
$ cat heap.c
#include <err.h>
#include <stdio.h>
#include <stdlib.h>
#include <strings.h>
int
main(void)
{
char *p;
p = malloc(1);
if (p == NULL)
err(1, "malloc");
p = strcpy(p, "1234567");
printf("%s\n", p);
free(p);
p = malloc(1);
p = strcpy(p, "12345678");
printf("%s\n", p);
free(p);
return 0;
}
$ cc -o heap heap.c
/tmp//ccHH1E1h.o: In function `main': heap.c:(.text+0x6b): warning: warning:
strcpy() is almost always
misused, please use strlcpy()
$ ./heap
1234567
12345678
heap(5992) in free(): error: chunk canary corrupted 0x1f4619f0e9c0
Abort trap (core dumped)
$
The performance impact of MALLOC_OPTIONS='CSV' is very noticeable,
but 'CV' is negligible and 'CJV' makes the system feel a bit less
snappy on my machine, but the actual effect seems almost negligible.
Here's a rough 'benchmark':
Building the base system on my i7 Thinkpad T420 with 4G RAM,
with the various malloc options each with a freshly booted system.
defaults: 31m04
CV: 33m16
CJV: 33m46
CSV: 53m32
Unfortunately, I have yet to hit an actual bug with this, one that isn't
triggered by simple test programs. Casual browsing with firefox, even
watching movies with mplayer is possible with MALLOC_OPTIONS='CSV'.
On Sun, Nov 01, 2015 at 02:56:11PM -0500, Daniel Micay wrote:
> @@ -560,6 +563,12 @@ omalloc_init(struct dir_info **dp)
> case 'J':
> mopts.malloc_junk = 2;
> break;
> + case 'v':
> + mopts.malloc_validate = 0;
> + break;
> + case 'V':
> + mopts.malloc_validate = 1;
> + break;
> case 'n':
> case 'N':
> break;
I'd keep the alphabetical order in the switch.
> @@ -608,6 +617,9 @@ omalloc_init(struct dir_info **dp)
> }
> }
>
> + if (!mopts.malloc_junk)
> + mopts.malloc_validate = 0;
> +
Wouldn't it make sense to just let 'V' imply 'J' (and 'j' imply 'v') as
is already done with 'F' and 'U', respectively?
> @@ -1190,6 +1208,35 @@ malloc(size_t size)
> /*DEF_STRONG(malloc);*/
>
> static void
> +validate_junk(void *p) {
> + struct region_info *r;
> + struct dir_info *pool = getpool();
> + size_t byte, sz;
> + if (p == NULL)
> + return;
> + r = find(pool, p);
> + if (r == NULL)
> + wrterror("bogus pointer in validate_junk", p);
> + REALSIZE(sz, r);
> + for (byte = 0; byte < sz; byte++) {
> + if (((char *)p)[byte] != SOME_FREEJUNK) {
This cast should be an (unsigned char *):
we have SOME_FREEJUNK == 0xdf, so '((char *)p)[byte] != SOME_FREEJUNK'
will always be true (this is pointed out when compiling with both clang
and gcc on amd64).
Index: stdlib/malloc.c
===================================================================
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.176
diff -u -p -r1.176 malloc.c
--- stdlib/malloc.c 13 Sep 2015 20:29:23 -0000 1.176
+++ stdlib/malloc.c 2 Nov 2015 09:52:32 -0000
@@ -182,15 +182,18 @@ struct malloc_readonly {
int malloc_freeunmap; /* mprotect free pages PROT_NONE? */
int malloc_hint; /* call madvice on free pages? */
int malloc_junk; /* junk fill? */
+ int malloc_validate; /* validate junk */
int malloc_move; /* move allocations to end of page? */
int malloc_realloc; /* always realloc? */
int malloc_xmalloc; /* xmalloc behaviour? */
+ size_t malloc_canaries; /* use canaries after chunks? */
size_t malloc_guard; /* use guard pages after allocations? */
u_int malloc_cache; /* free pages we cache */
#ifdef MALLOC_STATS
int malloc_stats; /* dump statistics at end */
#endif
u_int32_t malloc_canary; /* Matched against ones in malloc_pool
*/
+ uintptr_t malloc_chunk_canary;
};
/* This object is mapped PROT_READ after initialisation to prevent tampering */
@@ -218,6 +221,8 @@ static void malloc_exit(void);
#define CALLER NULL
#endif
+static void validate_delayed_chunks(void);
+
/* low bits of r->p determine size: 0 means >= page size and p->size holding
* real size, otherwise r->size is a shift count, or 1 for malloc(0)
*/
@@ -526,6 +531,12 @@ omalloc_init(struct dir_info **dp)
case 'A':
mopts.malloc_abort = 1;
break;
+ case 'c':
+ mopts.malloc_canaries = 0;
+ break;
+ case 'C':
+ mopts.malloc_canaries = sizeof(void *);
+ break;
#ifdef MALLOC_STATS
case 'd':
mopts.malloc_stats = 0;
@@ -592,6 +603,12 @@ omalloc_init(struct dir_info **dp)
case 'U':
mopts.malloc_freeunmap = 1;
break;
+ case 'v':
+ mopts.malloc_validate = 0;
+ break;
+ case 'V':
+ mopts.malloc_validate = 1;
+ break;
case 'x':
mopts.malloc_xmalloc = 0;
break;
@@ -608,6 +625,9 @@ omalloc_init(struct dir_info **dp)
}
}
+ if (!mopts.malloc_junk)
+ mopts.malloc_validate = 0;
+
#ifdef MALLOC_STATS
if (mopts.malloc_stats && (atexit(malloc_exit) == -1)) {
static const char q[] = "malloc() warning: atexit(2) failed."
@@ -616,9 +636,18 @@ omalloc_init(struct dir_info **dp)
}
#endif /* MALLOC_STATS */
+ if (mopts.malloc_validate && (atexit(validate_delayed_chunks) == -1)) {
+ static const char q[] = "malloc() warning: atexit(2) failed."
+ " Will not be able to check for use after free\n";
+ write(STDERR_FILENO, q, sizeof(q) - 1);
+ }
+
while ((mopts.malloc_canary = arc4random()) == 0)
;
+ arc4random_buf(&mopts.malloc_chunk_canary,
+ sizeof(mopts.malloc_chunk_canary));
+
/*
* Allocate dir_info with a guard page on either side. Also
* randomise offset inside the page at which the dir_info
@@ -984,8 +1013,15 @@ malloc_bytes(struct dir_info *d, size_t
k += (lp - bp->bits) * MALLOC_BITS;
k <<= bp->shift;
+ if (mopts.malloc_canaries && bp->size > 0) {
+ char *end = (char *)bp->page + k + bp->size;
+ uintptr_t *canary = (uintptr_t *)(end - mopts.malloc_canaries);
+ *canary = mopts.malloc_chunk_canary ^ hash(canary);
+ }
+
if (mopts.malloc_junk == 2 && bp->size > 0)
- memset((char *)bp->page + k, SOME_JUNK, bp->size);
+ memset((char *)bp->page + k, SOME_JUNK,
+ bp->size - mopts.malloc_canaries);
return ((char *)bp->page + k);
}
@@ -999,6 +1035,13 @@ find_chunknum(struct dir_info *d, struct
if (info->canary != d->canary1)
wrterror("chunk info corrupted", NULL);
+ if (mopts.malloc_canaries && info->size > 0) {
+ char *end = (char *)ptr + info->size;
+ uintptr_t *canary = (uintptr_t *)(end - mopts.malloc_canaries);
+ if (*canary != (mopts.malloc_chunk_canary ^ hash(canary)))
+ wrterror("chunk canary corrupted", ptr);
+ }
+
/* Find the chunk number on the page */
chunknum = ((uintptr_t)ptr & MALLOC_PAGEMASK) >> info->shift;
@@ -1121,7 +1164,7 @@ omalloc(size_t sz, int zero_fill, void *
/* takes care of SOME_JUNK */
p = malloc_bytes(pool, sz, f);
if (zero_fill && p != NULL && sz > 0)
- memset(p, 0, sz);
+ memset(p, 0, sz - mopts.malloc_canaries);
}
return p;
@@ -1176,6 +1219,8 @@ malloc(size_t size)
malloc_recurse();
return NULL;
}
+ if (size > 0 && size <= MALLOC_MAXCHUNK)
+ size += mopts.malloc_canaries;
r = omalloc(size, 0, CALLER);
malloc_active--;
_MALLOC_UNLOCK();
@@ -1190,6 +1235,37 @@ malloc(size_t size)
/*DEF_STRONG(malloc);*/
static void
+validate_junk(void *p) {
+ struct region_info *r;
+ struct dir_info *pool = getpool();
+ size_t byte, sz;
+ if (p == NULL)
+ return;
+ r = find(pool, p);
+ if (r == NULL)
+ wrterror("bogus pointer in validate_junk", p);
+ REALSIZE(sz, r);
+ if (sz > 0 && sz <= MALLOC_MAXCHUNK)
+ sz -= mopts.malloc_canaries;
+ for (byte = 0; byte < sz; byte++) {
+ if (((unsigned char *)p)[byte] != SOME_FREEJUNK) {
+ wrterror("use after free", p);
+ return;
+ }
+ }
+}
+
+static void
+validate_delayed_chunks(void) {
+ struct dir_info *pool = getpool();
+ int i;
+ if (pool == NULL)
+ return;
+ for (i = 0; i < MALLOC_DELAYED_CHUNK_MASK + 1; i++)
+ validate_junk(pool->delayed_chunks[i]);
+}
+
+static void
ofree(void *p)
{
struct dir_info *pool = getpool();
@@ -1242,7 +1318,7 @@ ofree(void *p)
int i;
if (mopts.malloc_junk && sz > 0)
- memset(p, SOME_FREEJUNK, sz);
+ memset(p, SOME_FREEJUNK, sz - mopts.malloc_canaries);
if (!mopts.malloc_freenow) {
if (find_chunknum(pool, r, p) == -1)
return;
@@ -1253,6 +1329,8 @@ ofree(void *p)
wrterror("double free", p);
return;
}
+ if (mopts.malloc_validate)
+ validate_junk(p);
pool->delayed_chunks[i] = tmp;
}
if (p != NULL) {
@@ -1386,16 +1464,25 @@ gotit:
}
}
if (newsz <= oldsz && newsz > oldsz / 2 && !mopts.malloc_realloc) {
- if (mopts.malloc_junk == 2 && newsz > 0)
- memset((char *)p + newsz, SOME_JUNK, oldsz - newsz);
+ if (mopts.malloc_junk == 2 && newsz > 0) {
+ size_t usable_oldsz = oldsz;
+ if (oldsz <= MALLOC_MAXCHUNK)
+ usable_oldsz -= mopts.malloc_canaries;
+ if (newsz < usable_oldsz)
+ memset((char *)p + newsz, SOME_JUNK,
usable_oldsz - newsz);
+ }
STATS_SETF(r, f);
return p;
} else if (newsz != oldsz || mopts.malloc_realloc) {
q = omalloc(newsz, 0, f);
if (q == NULL)
return NULL;
- if (newsz != 0 && oldsz != 0)
- memcpy(q, p, oldsz < newsz ? oldsz : newsz);
+ if (newsz != 0 && oldsz != 0) {
+ size_t copysz = oldsz < newsz ? oldsz : newsz;
+ if (copysz <= MALLOC_MAXCHUNK)
+ copysz -= mopts.malloc_canaries;
+ memcpy(q, p, copysz);
+ }
ofree(p);
return q;
} else {
@@ -1420,6 +1507,8 @@ realloc(void *ptr, size_t size)
malloc_recurse();
return NULL;
}
+ if (size > 0 && size <= MALLOC_MAXCHUNK)
+ size += mopts.malloc_canaries;
r = orealloc(ptr, size, CALLER);
malloc_active--;
@@ -1468,6 +1557,8 @@ calloc(size_t nmemb, size_t size)
}
size *= nmemb;
+ if (size > 0 && size <= MALLOC_MAXCHUNK)
+ size += mopts.malloc_canaries;
r = omalloc(size, 1, CALLER);
malloc_active--;
@@ -1595,6 +1686,8 @@ posix_memalign(void **memptr, size_t ali
malloc_recurse();
goto err;
}
+ if (size > 0 && size <= MALLOC_MAXCHUNK)
+ size += mopts.malloc_canaries;
r = omemalign(alignment, size, 0, CALLER);
malloc_active--;
_MALLOC_UNLOCK();
